-
Notifications
You must be signed in to change notification settings - Fork 1.8k
include some BinaryOperator from sqlparser #15327
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
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
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 @waynexia -- I think the idea is great. However, I think there should be sql level tests (sqllogitests) that run these operators. I think it will make extending operators easier for other systems built on DataFusion
The tests can / should probably error with unsupported
It might also be a good idea to include some documentation in the operators themselves that DataFusion doesn't have default implementations
| AtArrow | ArrowAt | Arrow | LongArrow | HashArrow | HashLongArrow | AtAt | ||
| | HashMinus | AtQuestion | Question | QuestionAnd | QuestionPipe | ||
| | IntegerDivide => { | ||
| unreachable!("These operators should be rewritten to functions") |
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 dont think this code is unreachable -- I got it to panic:
DataFusion CLI v46.0.1
> select 'foo' @> 'bar';
thread 'main' panicked at datafusion/physical-expr/src/expressions/binary.rs:799:17:
internal error: entered unreachable code: These operators should be rewritten to functions
note: run with `RUST_BACKTRACE=1` environment variable to display a backtraceThere 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 just realized <@ and @> are only supported for lists 🤣
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
That's a good idea! I think I know them much better after writing some SQLs (though none of them can run... lol) |
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Added in 5828cba 👍 |
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.
THanks @waynexia
|
Thank you @alamb |
* include some BinaryOperator from sqlparser Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * slt for new operators Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * add document about availability Signed-off-by: Ruihang Xia <waynestxia@gmail.com> --------- Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
* include some BinaryOperator from sqlparser Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * slt for new operators Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * add document about availability Signed-off-by: Ruihang Xia <waynestxia@gmail.com> --------- Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
* include some BinaryOperator from sqlparser Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * slt for new operators Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * add document about availability Signed-off-by: Ruihang Xia <waynestxia@gmail.com> --------- Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Which issue does this PR close?
Rationale for this change
Follows #15326, this patch includes
Arrow,LongArrow,HashArrow,HashLongArrow,AtAt,IntegerDivide,HashMinus,AtQuestion,Question,QuestionAndandQuestionPipebinary operators.What changes are included in this PR?
New operators. Only
XORis actually implemented (functional) among them, by mapping to existingBitwiseXor.Are these changes tested?
Compiler tests it
Are there any user-facing changes?
New APIs