-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Allow Logical expression ScalarVariable to represent an extension type or metadata #18243
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
base: main
Are you sure you want to change the base?
Conversation
|
@paleolimbot can you review this sire |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
@alamb kindly added a helper to make the DataType -> FieldRef cleaner, which I think will be useful here.
I think this also needs at least one test to ensure that the metadata and nullability of a variable are inferred correctly by Expr::to_field()!
I don't have the power to make the CI run but perhaps a committer who comes across this can hit the button 🙂
datafusion/core/"/var/folders/xf/78z9wmtx4r9g_9pmytgm2ysm0000gn/T/.tmpU3MvU3/target.csv"
Outdated
Show resolved
Hide resolved
|
@batmnnn thank you for this PR. Do you have time to address @paleolimbot 's comments on this PR? |
Co-authored-by: Dewey Dunnington <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
…n/T/.tmpU3MvU3/target.csv"
|
@alamb sorry for the delay got caught up in exams, just addressed them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
This needs a committer to run CI...provided it's green, this looks good to me!
Which issue does this PR close?
Rationale for this change
Add richer variable metadata by switching Expr::ScalarVariable to store an Arrow Field, allowing planners to retain nullability and metadata when handling @var expressions.
What changes are included in this PR?
This PR updates ScalarVariable to use FieldRef so it can represent extension types and metadata in logical expressions.
Are these changes tested?
Yes
Are there any user-facing changes?
When planning queries with variables (e.g., @foo), the resulting logical expressions carry full field metadata instead of only a data type. This can affect downstream components that inspect nullability or custom metadata.