-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Rewrite array @> array
and array <@ array
in sql_expr_to_logical_expr
#11155
Conversation
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@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 @jayzhan211 -- this looks good to me
Can you remind me why this method is preferrable to the FunctionRewrite idea we previously had?
I feel this PR and changes are related to the one from @samuelcolvin on #11137 -- where the idea is to support SQL syntax with JSON operators without putting the implementation into SQL
@@ -1225,22 +1225,6 @@ fn select_binary_expr_nested() { | |||
quick_test(sql, expected); | |||
} | |||
|
|||
#[test] | |||
fn select_at_arrow_operator() { |
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.
Should we add an explain test?
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 don't think it is needed since we don't really care about how it looks in plan.
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.
Maybe we can make sure array_has_all
is applied 🤔
Here is an alternate idea #11168 |
From your comment here :) #10534 (comment) |
@@ -145,36 +145,3 @@ fn compare_op_for_nested( | |||
Ok(BooleanArray::new(values, nulls)) | |||
} | |||
} | |||
|
|||
#[cfg(test)] | |||
mod tests { |
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.
forgot to remove in previous PR
…expr (apache#11155) * rewrite at arrow Signed-off-by: jayzhan211 <jayzhan211@gmail.com> * rm useless test Signed-off-by: jayzhan211 <jayzhan211@gmail.com> * add test Signed-off-by: jayzhan211 <jayzhan211@gmail.com> * rm test Signed-off-by: jayzhan211 <jayzhan211@gmail.com> --------- Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Which issue does this PR close?
Part of #8506 #10534
Rationale for this change
Move the rewrite from optimizer to sql crate
I keep the function rewrite trait even it is not used anywhere in datafusion now.
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?