-
Notifications
You must be signed in to change notification settings - Fork 1.9k
add support for unary and binary values in values list, update docs #1172
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
Conversation
alamb
left a comment
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.
Very cool @jimexist 👍
| - [x] nullif | ||
| - Approximation functions | ||
| - [ ] approx_distinct | ||
| - [x] approx_distinct |
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.
❤️
| Ok(n) => Ok(lit(n)), | ||
| Err(_) => Ok(lit(n.parse::<f64>().unwrap())), | ||
| }, | ||
| SQLExpr::Value(Value::Number(n, _)) => parse_sql_number(n), |
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.
I wonder if we could just call sql_to_logical_expr here (rather than repeating part of the parsing) so that any expression in the values list could be supported
That would allow things like CASE .. WHEN .. END type expressions in the VALUES lists as well
Although if we went with that approach, we probably would have to then do a post creation check for unsupported expressions (like Column, Aggregate, etc)
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.
i'd like to keep this list small for now unless really needed in future.
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.
the key assumption here is that for all the expression in the values list they should be evaluated to a scalar or singleton array, but there's no way to test for that during planning time?
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.
i'd like to keep this list small for now unless really needed in future.
Makes sense to me
the key assumption here is that for all the expression in the values list they should be evaluated to a scalar or singleton array, but there's no way to test for that during planning time?
I think in general most Exprs have the property that they can be compiled into a PhysicalExpr and that PhysicalExpr can be evaluated to produce a ArrayRef. There are some exceptions like Expr::Aggregrate etc that don't compile directly to a PhysicalExpr but are special cased
…etrics (apache#1172) * Sync with main. * Spark History server sems to appead the word total at the end, so I removed the redundant word at the beginning of the new metrics. * Fix typo.
Which issue does this PR close?
add support for unary and binary values in values list
Closes #1171
Rationale for this change
What changes are included in this PR?
this is a follow up for #1165
Are there any user-facing changes?