You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "solontsev (via GitHub)" <gi...@apache.org> on 2025/07/06 08:26:33 UTC
[PR] Fix for Postgres regex and like binary operators [datafusion-sqlparser-rs]
solontsev opened a new pull request, #1928:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1928
Closes #1776
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix for Postgres regex and like binary operators [datafusion-sqlparser-rs]
Posted by "solontsev (via GitHub)" <gi...@apache.org>.
solontsev commented on code in PR #1928:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1928#discussion_r2207480227
##########
tests/sqlparser_postgres.rs:
##########
@@ -2207,19 +2223,31 @@ fn parse_pg_like_match_ops() {
];
for (str_op, op) in pg_like_match_ops {
- let select = pg().verified_only_select(&format!("SELECT 'abc' {} 'a_c%'", &str_op));
+ // Basic binary operator usage
+ let select = pg().verified_only_select(&format!("SELECT 'abc' {str_op} 'a_c%'"));
assert_eq!(
SelectItem::UnnamedExpr(Expr::BinaryOp {
- left: Box::new(Expr::Value(
- (Value::SingleQuotedString("abc".into())).with_empty_span()
- )),
+ left: Box::new(Expr::Value(single_quoted_string("abc").with_empty_span(),)),
op: op.clone(),
- right: Box::new(Expr::Value(
- (Value::SingleQuotedString("a_c%".into())).with_empty_span()
- )),
+ right: Box::new(Expr::Value(single_quoted_string("a_c%").with_empty_span(),)),
}),
select.projection[0]
);
+
+ // Binary operator with ALL operator
+ let select =
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix for Postgres regex and like binary operators [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1928:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1928#discussion_r2192773155
##########
tests/sqlparser_postgres.rs:
##########
@@ -2207,19 +2223,31 @@ fn parse_pg_like_match_ops() {
];
for (str_op, op) in pg_like_match_ops {
- let select = pg().verified_only_select(&format!("SELECT 'abc' {} 'a_c%'", &str_op));
+ // Basic binary operator usage
+ let select = pg().verified_only_select(&format!("SELECT 'abc' {str_op} 'a_c%'"));
assert_eq!(
SelectItem::UnnamedExpr(Expr::BinaryOp {
- left: Box::new(Expr::Value(
- (Value::SingleQuotedString("abc".into())).with_empty_span()
- )),
+ left: Box::new(Expr::Value(single_quoted_string("abc").with_empty_span(),)),
op: op.clone(),
- right: Box::new(Expr::Value(
- (Value::SingleQuotedString("a_c%".into())).with_empty_span()
- )),
+ right: Box::new(Expr::Value(single_quoted_string("a_c%").with_empty_span(),)),
}),
select.projection[0]
);
+
+ // Binary operator with ALL operator
+ let select =
Review Comment:
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](https://github.com/apache/datafusion-sqlparser-rs/blob/60acdc5bb67383ba0e4b3a6291ca6701ab2ec752/tests/sqlparser_postgres.rs#L2218-L2223) being tested so that its not clear to me why this test fails without the fix in this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix for Postgres regex and like binary operators [datafusion-sqlparser-rs]
Posted by "solontsev (via GitHub)" <gi...@apache.org>.
solontsev commented on code in PR #1928:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1928#discussion_r2193231211
##########
tests/sqlparser_postgres.rs:
##########
@@ -2207,19 +2223,31 @@ fn parse_pg_like_match_ops() {
];
for (str_op, op) in pg_like_match_ops {
- let select = pg().verified_only_select(&format!("SELECT 'abc' {} 'a_c%'", &str_op));
+ // Basic binary operator usage
+ let select = pg().verified_only_select(&format!("SELECT 'abc' {str_op} 'a_c%'"));
assert_eq!(
SelectItem::UnnamedExpr(Expr::BinaryOp {
- left: Box::new(Expr::Value(
- (Value::SingleQuotedString("abc".into())).with_empty_span()
- )),
+ left: Box::new(Expr::Value(single_quoted_string("abc").with_empty_span(),)),
op: op.clone(),
- right: Box::new(Expr::Value(
- (Value::SingleQuotedString("a_c%".into())).with_empty_span()
- )),
+ right: Box::new(Expr::Value(single_quoted_string("a_c%").with_empty_span(),)),
}),
select.projection[0]
);
+
+ // Binary operator with ALL operator
+ let select =
Review Comment:
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 + allowed 8 more operators before the ALL and ANY functions in the `parse_infix` function.
https://github.com/apache/datafusion-sqlparser-rs/blob/60acdc5bb67383ba0e4b3a6291ca6701ab2ec752/src/parser/mod.rs#L3481-L3488
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix for Postgres regex and like binary operators [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio merged PR #1928:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1928
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix for Postgres regex and like binary operators [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1928:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1928#discussion_r2198022760
##########
tests/sqlparser_postgres.rs:
##########
@@ -2207,19 +2223,31 @@ fn parse_pg_like_match_ops() {
];
for (str_op, op) in pg_like_match_ops {
- let select = pg().verified_only_select(&format!("SELECT 'abc' {} 'a_c%'", &str_op));
+ // Basic binary operator usage
+ let select = pg().verified_only_select(&format!("SELECT 'abc' {str_op} 'a_c%'"));
assert_eq!(
SelectItem::UnnamedExpr(Expr::BinaryOp {
- left: Box::new(Expr::Value(
- (Value::SingleQuotedString("abc".into())).with_empty_span()
- )),
+ left: Box::new(Expr::Value(single_quoted_string("abc").with_empty_span(),)),
op: op.clone(),
- right: Box::new(Expr::Value(
- (Value::SingleQuotedString("a_c%".into())).with_empty_span()
- )),
+ right: Box::new(Expr::Value(single_quoted_string("a_c%").with_empty_span(),)),
}),
select.projection[0]
);
+
+ // Binary operator with ALL operator
+ let select =
Review Comment:
> + 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix for Postgres regex and like binary operators [datafusion-sqlparser-rs]
Posted by "solontsev (via GitHub)" <gi...@apache.org>.
solontsev commented on code in PR #1928:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1928#discussion_r2193231211
##########
tests/sqlparser_postgres.rs:
##########
@@ -2207,19 +2223,31 @@ fn parse_pg_like_match_ops() {
];
for (str_op, op) in pg_like_match_ops {
- let select = pg().verified_only_select(&format!("SELECT 'abc' {} 'a_c%'", &str_op));
+ // Basic binary operator usage
+ let select = pg().verified_only_select(&format!("SELECT 'abc' {str_op} 'a_c%'"));
assert_eq!(
SelectItem::UnnamedExpr(Expr::BinaryOp {
- left: Box::new(Expr::Value(
- (Value::SingleQuotedString("abc".into())).with_empty_span()
- )),
+ left: Box::new(Expr::Value(single_quoted_string("abc").with_empty_span(),)),
op: op.clone(),
- right: Box::new(Expr::Value(
- (Value::SingleQuotedString("a_c%".into())).with_empty_span()
- )),
+ right: Box::new(Expr::Value(single_quoted_string("a_c%").with_empty_span(),)),
}),
select.projection[0]
);
+
+ // Binary operator with ALL operator
+ let select =
Review Comment:
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.
https://github.com/apache/datafusion-sqlparser-rs/blob/60acdc5bb67383ba0e4b3a6291ca6701ab2ec752/src/parser/mod.rs#L3481-L3488
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org