Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update duckdb version #134

Merged
merged 5 commits into from
Jan 7, 2025

Conversation

srikrishnak
Copy link
Contributor

No description provided.

@CLAassistant
Copy link

CLAassistant commented Dec 28, 2024

CLA assistant check
All committers have signed the CLA.

@jacques-n
Copy link
Contributor

Can you update to tip of duckdb instead of this version?

EpsilonPrime
EpsilonPrime previously approved these changes Dec 30, 2024
Copy link
Member

@EpsilonPrime EpsilonPrime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a new table filter in the latest version?

EpsilonPrime
EpsilonPrime previously approved these changes Jan 3, 2025
@srikrishnak
Copy link
Contributor Author

Is there a new table filter in the latest version?

I figured out that the errors are coming for filter type OPTIONAL_FILTER

error: unsupported table filter type OPTIONAL_FILTER

@srikrishnak
Copy link
Contributor Author

srikrishnak commented Jan 7, 2025

Is there a new table filter in the latest version?

I figured out that the errors are coming for filter type OPTIONAL_FILTER

error: unsupported table filter type OPTIONAL_FILTER

This is introduced a while back. The test as I understand, does a get_substrait(query) and use the plan. Since, get_substrait() is provided by duckdb, I am checking the changes around that area as well.

Also, make sure filter type constant comparison always returns a boolean
@@ -698,7 +698,7 @@ substrait::Expression *DuckDBToSubstrait::TransformConstantComparisonFilter(uint
auto s_expr = new substrait::Expression();
auto s_scalar = s_expr->mutable_scalar_function();
auto &constant_filter = dfilter.Cast<ConstantFilter>();
*s_scalar->mutable_output_type() = DuckToSubstraitType(return_type);
*s_scalar->mutable_output_type() = DuckToSubstraitType(LogicalTypeId::BOOLEAN);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this theoretically return a different type for ConjunctionAndFilter and ConstantComparisonFilter since we'er using return_type below? If so, should we use replace the use of return_type as well (or only set this to boolean for optional_filter)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type for any of these comparison is always Boolean.

The return for other filters is not changed.

@EpsilonPrime EpsilonPrime merged commit d0b5597 into substrait-io:main Jan 7, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants