-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: simplify regex wildcard pattern #15299
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>
|
|
||
| if let Expr::Literal(ScalarValue::Utf8(Some(pattern))) = right.as_ref() { | ||
| // Handle the special case for ".*" pattern | ||
| if pattern == ".*" { |
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.
We can make this a const similar to COUNT_STAR_EXPANSION
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
| right: empty_lit, | ||
| }) | ||
| } else { | ||
| // always true |
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 this needs to handle nulls too as null ~ '.*' is not true (it is null)
> create or replace table foo(x varchar) as values (1), (2), (null);
0 row(s) fetched.
Elapsed 0.004 seconds.
> select x ~ '.*' from foo;
+--------------------+
| foo.x ~ Utf8(".*") |
+--------------------+
| true |
| true |
| NULL |
+--------------------+
3 row(s) fetched.
Elapsed 0.016 seconds.So maybe instead of lit(true) it is x.is_not_null() 🤔
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.
Good catch! I added two cases about null in simplify_expr.slt, it should work as expected now.
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
| 40 | ||
|
|
||
| query I | ||
| SELECT * FROM v1; |
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.
If this result is not deterministic, we need rowsort for it
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.
Added in 95848ef
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.
Looks good to me -- thank you @waynexia
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
|
Thank you for reviewing @jayzhan211 @alamb ❤️ |
Which issue does this PR close?
Rationale for this change
Simplify dump regex cases like
~ '.*'or!~ '.*'.What changes are included in this PR?
Handle special wildcard regex pattern in expr_simplifier rule
Are these changes tested?
Yes, via sqllogictests and unit tests
Are there any user-facing changes?
no