-
Notifications
You must be signed in to change notification settings - Fork 622
Fix for Postgres regex and like binary operators #1928
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
}), | ||
select.projection[0] | ||
); | ||
|
||
// Binary operator with ALL operator | ||
let select = |
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'm not sure I fully understood the fix, is the issue specific to the ALL
and ANY
functions? the current test doesn't seem to make changes to the list of operators being tested so that its not clear to me why this test fails without the fix in this PR
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.
Currently, when parsing expressions with regex and like operators, only literals are supported, while syntax like where a ~ any(array['x'])
is also valid (matching against an array of values using ALL or ANY). I've added additional more complex expressions to the tests (fails with the current version) + allowed 8 more operators before the ALL and ANY functions in the parse_infix
function.
datafusion-sqlparser-rs/src/parser/mod.rs
Lines 3481 to 3488 in 60acdc5
| BinaryOperator::PGRegexMatch | |
| BinaryOperator::PGRegexIMatch | |
| BinaryOperator::PGRegexNotMatch | |
| BinaryOperator::PGRegexNotIMatch | |
| BinaryOperator::PGLikeMatch | |
| BinaryOperator::PGILikeMatch | |
| BinaryOperator::PGNotLikeMatch | |
| BinaryOperator::PGNotILikeMatch |
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.
- allowed 8 more operators before the ALL and ANY functions in the parse_infix function.
Are the added operators covered by the tests? if not could we add tests covering them?
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.
Are the added operators covered by the tests? if not could we add tests covering them?
Yeah, I've added the tests as well. Maybe it was not the best to test 2 cases during each cycle iteration, I've modified it a bit: a previous match against a single value, and added one more to test against an array of values. Does it look better now?
Also, to wrap up: The problem now, is that before ANY, ALL or SOME functions, parser allows only 6 basic binary operators (gt, lt, gteq, lteq, eq, noteq). But those 8 additional are also valid. So the fix basically just adds them to the allowlist and tests for match and like operators are modified to make sure, parser doesn't return an error in a valid cases. I haven't added exhaustive test cases, just chose ANY function for match tests and ALL function for like 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.
LGTM! Thanks @solontsev!
cc @alamb
Closes #1776
Currently, when parsing expressions with regex and like operators, only literals are supported, while the following syntax is also valid (matching against an array of values using ALL or ANY):
or