-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-14545][SQL] Improve LikeSimplification by adding a%b rule
#12312
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
|
Test build #55555 has finished for PR 12312 at commit
|
| StartsWith(l, Literal(pattern)) | ||
| case endsWith(pattern) => | ||
| EndsWith(l, Literal(pattern)) | ||
| case startsAndEndsWith(prefix, postfix) if !prefix.endsWith("\\") => |
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 think you should have some comments explaining this rewrite, in particular the greaterthanorequal part is not as straightforward as the other ones.
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 for review. I'll add comments here.
|
Hi, @rxin . Sorry for late response. |
|
Test build #55618 has finished for PR 12312 at commit
|
|
Hi, @rxin . |
| EndsWith(l, Literal(pattern)) | ||
| // 'a%a' pattern is basically same with 'a%' && '%a'. | ||
| // However, the additional `Length` condition is required to prevent 'a' match 'a%a'. | ||
| case startsAndEndsWith(prefix, postfix) if !prefix.endsWith("\\") => |
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.
while you are at it, can you rename "pattern" in the startsWith case to prefix, and endsWith to suffix?
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.
also rename utf to "pattern"
and just compute the value of GreaterThanOrEqual(Length(l), Literal(prefix.size + postfix.size)) directly since it is a literal
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.
Sure. I will renames those parameters.
By the way, l of Length(l) is not literal here. It could be column.
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 for spending time here. I know your are very busy.
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.
ah ok - let's leave the l one there
|
I updated to use |
| Contains(l, Literal(pattern)) | ||
| case equalTo(pattern) => | ||
| EqualTo(l, Literal(pattern)) | ||
| case Like(l, Literal(pattern, StringType)) => |
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.
sorry to be pedantic. do you mind changing l to something like input? "l" is too short and unclear what it means, and it also just looks like one.
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.
You're welcome. :) I'll fix right now!
|
Test build #55826 has finished for PR 12312 at commit
|
|
Test build #55830 has finished for PR 12312 at commit
|
|
Thanks - merging in master. |
|
Thank you for merging, @rxin ! |
What changes were proposed in this pull request?
Current
LikeSimplificationhandles the following four rules.This PR adds the following rule.
Here, 2 is statically calculated from "a".size + "b".size.
Before
After
How was this patch tested?
Pass the Jenkins tests (including new testcase).