-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support Dictionary
and List
types in scalar_to_sql
#14346
Conversation
Thanks @cetra3 -- I think this PR needs to be merged up from main -- the PR currently has a fialure due to a logical conflict |
scalar_to_sql
Dictionary
types in scalar_to_sql
Dictionary
types in scalar_to_sql
Dictionary
and List
types in scalar_to_sql
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.
Looks good to me -- thank you @cetra3
FYI @phillipleblanc
"'foo'", | ||
), | ||
( | ||
Expr::Literal(ScalarValue::List(Arc::new( |
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.
Can you please also add a test for LargeList
?
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.
Done
a8a934a
to
a1caa5d
Compare
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.
Thanks!
🚀 |
Thanks again @cetra3 and @phillipleblanc |
Rationale for this change
Adds more support for
ScalarValue
variants inscalar_to_sql
What changes are included in this PR?
Includes support for lists and dictionary type scalars.
Are these changes tested?
Tests have been added for the
ScalarValue::Dictionary
andScalarValue::List
variantsAre there any user-facing changes?
There are no breaking/user-facing changes