Skip to content

feat: Support ANY/ALL operators#477

Merged
alamb merged 2 commits intoapache:mainfrom
cube-js:any-all-operators
May 6, 2022
Merged

feat: Support ANY/ALL operators#477
alamb merged 2 commits intoapache:mainfrom
cube-js:any-all-operators

Conversation

@ovr
Copy link
Copy Markdown
Contributor

@ovr ovr commented Apr 28, 2022

Hello!

Ref issue: #476

I want to support a = ANY(expr) in DF, but after reviewing I come to decision that it requires changes in sql-parser to simplify code (because hacking when the right side is a function is a super tricky)

Thanks

@ovr ovr force-pushed the any-all-operators branch from 8d812e3 to 15f3444 Compare April 28, 2022 14:41
@ovr ovr marked this pull request as ready for review April 28, 2022 14:42
@ovr
Copy link
Copy Markdown
Contributor Author

ovr commented Apr 28, 2022

@alamb WDYT? Should I create a separate issue for supporting AllOperatorSubquery & AnyOperatorSubQuery?

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 28, 2022

Pull Request Test Coverage Report for Build 2277494621

  • 31 of 33 (93.94%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 90.444%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser.rs 15 17 88.24%
Totals Coverage Status
Change from base Build 2273021751: 0.02%
Covered Lines: 8177
Relevant Lines: 9041

💛 - Coveralls

Copy link
Copy Markdown
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ho @ovr --- sounds like an exiting feature!

Looking at the postgres documentation, https://www.postgresql.org/docs/current/functions-subquery.html it seems to me like ANY / ALL can be combined with any operator that results in a boolean, not just <, <=, =, !=, >=, >

For example, with ~:

alamb=# select x from test where x ~ ANY (select x from test);
  x  
-----
 foo
(1 row)

What would you think about adding something like

 /// `ANY()`
    Any(Box<Expr>),

This might be more in keeping with the spirit of sql-parser-rs to parse the query faithfully without applying any implicit semantics to it.

@alamb WDYT? Should I create a separate issue for supporting AllOperatorSubquery & AnyOperatorSubQuery?

I don't think separate issues are necessary

@ovr
Copy link
Copy Markdown
Contributor Author

ovr commented Apr 29, 2022

@alamb Oh, I see, but anyway what about LIKE/AND/OR operators which are supported in Binary expression but is not supported for ANY & ALL operators? Another question is, whether it will lead to a nonimplicit pattern machine on the DF side?

If it's okey, I will update a PR to reflect your idea. Big thanks to you!

@alamb
Copy link
Copy Markdown
Contributor

alamb commented Apr 29, 2022

@alamb Oh, I see, but anyway what about LIKE/AND/OR operators which are supported in Binary expression but is not supported for ANY & ALL operators? Another question is, whether it will lead to a nonimplicit pattern machine on the DF side?

I am not quite sure what you mean about "nonimplicit pattern match on the DF side"

Maybe you could do something like the following with a match guard (totally untested):

sqlparser::Expr::BinaryExpr {
  left, 
  op
  right
} if is_any_expr(&right) => {...}
  }
...

and

/// returns true of `expr` represents an `Any` clause
fn is_any_expr(expr: sqlparser::Expr) ->bool)  {
...
}

I found this style worked pretty well in simplify_expressions:

https://github.com/apache/arrow-datafusion/blob/master/datafusion/core/src/optimizer/simplify_expressions.rs#L498-L508

@ovr ovr force-pushed the any-all-operators branch 2 times, most recently from 37d0338 to 69eceab Compare May 5, 2022 18:15
@ovr ovr force-pushed the any-all-operators branch from 69eceab to 773294c Compare May 5, 2022 18:16
@ovr
Copy link
Copy Markdown
Contributor Author

ovr commented May 5, 2022

@alamb Oh, I see your idea about simplifying it and wrapping it on the right side of the BinaryExpression expression. I've updated the PR as you suggested. Thanks

Copy link
Copy Markdown
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me @ovr 👍

Comment thread src/ast/mod.rs
right: Box<Expr>,
},
/// Any operation e.g. `1 ANY (1)` or `foo > ANY(bar)`, It will be wrapped in the right side of BinaryExpr
AnyOp(Box<Expr>),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment thread tests/sqlparser_common.rs
SelectItem::UnnamedExpr(Expr::BinaryOp {
left: Box::new(Expr::Identifier(Ident::new("a"))),
op: BinaryOperator::Eq,
right: Box::new(Expr::AnyOp(Box::new(Expr::Identifier(Ident::new("b"))))),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looks good to me

@alamb alamb merged commit 8ef5fc8 into apache:main May 6, 2022
ovr added a commit to cube-js/sqlparser-rs that referenced this pull request May 12, 2022
* feat: Support ANY/ALL operators

* fix lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants