Add support for quantified comparison predicates (ALL/ANY/SOME) #1459

yoavcloud · 2024-10-05T19:36:20Z

Allows comparisons against subquery results: https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html#_8_9_quantified_comparison_predicate

For example:

SELECT 42 >= SOME (SELECT 41 UNION ALL SELECT 42 UNION ALL SELECT 43); -- true

…L/SOME)

iffyio · 2024-10-07T18:47:14Z

src/parser/mod.rs

+                let right = if self.is_query_ahead() {
+                    // We have a subquery ahead (SELECT\WITH ...) need to rewind and
+                    // use the parenthesis for parsing the subquery as an expression.
+                    self.prev_token(); // SELECT


Would it make sense to call the self.prev_token() as part of the is_query_ahead() logic? Currently with the split it feels a bit disconnected that the method is abstracted but the caller here still has to know the details and manually rewind the token stream which seems like an arbitrary split of responsibility? So that if it works the same then the caller can only rely on the flag returned without knowing the details of how the check is done

Yes, good idea, implemented in the new commit.

iffyio · 2024-10-07T18:50:06Z

src/parser/mod.rs

+                    // use the parenthesis for parsing the subquery as an expression.
+                    self.prev_token(); // SELECT
+                    self.prev_token(); // LParen
+                    self.parse_subexpr(precedence)?


would it make sense to call self.try_parse_expr_sub_query() here already? it seems like it could let us avoid most of the if block here

I tried to make that work in the new commit, but since we need to rewind the LParen first it got a bit messy

Ah right that makes sense!

iffyio · 2024-10-07T18:52:26Z

src/parser/mod.rs

@@ -12141,6 +12144,12 @@ impl<'a> Parser<'a> {
    pub fn into_tokens(self) -> Vec<TokenWithLocation> {
        self.tokens
    }
+
+    /// Checks if the next keyword indicates a query, i.e. SELECT or WITH
+    pub fn is_query_ahead(&mut self) -> bool {


Suggested change

pub fn is_query_ahead(&mut self) -> bool {

fn peek_sub_query(&mut self) -> bool {

also the function was made public, assuming that was unintentional?

Fixed in new commit

iffyio · 2024-10-07T18:53:40Z

src/parser/mod.rs

@@ -12141,6 +12144,12 @@ impl<'a> Parser<'a> {
    pub fn into_tokens(self) -> Vec<TokenWithLocation> {
        self.tokens
    }
+
+    /// Checks if the next keyword indicates a query, i.e. SELECT or WITH


Suggested change

/// Checks if the next keyword indicates a query, i.e. SELECT or WITH

/// Returns true if the next keyword indicates a sub query, i.e. SELECT or WITH

iffyio · 2024-10-07T19:02:50Z

src/ast/mod.rs

@@ -650,6 +650,8 @@ pub enum Expr {
        left: Box<Expr>,
        compare_op: BinaryOperator,
        right: Box<Expr>,
+        // ANY and SOME are synonymous


Is there any one of the supported dialects supporting the SOME variant?

I believe hive does: https://docs.cloudera.com/cdw-runtime/cloud/using-hiveql/topics/hive_comparison_predicates.html?

Ah cool! Can we include the link in the comment here as well? It could help later on in case someone comes across this and wonders the same

iffyio

Thanks @yoavcloud! Left a couple minor comment regarding doc links, other than that I think this looks good

iffyio · 2024-10-08T16:26:52Z

src/ast/mod.rs

@@ -650,6 +650,8 @@ pub enum Expr {
        left: Box<Expr>,
        compare_op: BinaryOperator,
        right: Box<Expr>,
+        // ANY and SOME are synonymous


Ah cool! Can we include the link in the comment here as well? It could help later on in case someone comes across this and wonders the same

iffyio · 2024-10-08T16:30:27Z

src/ast/mod.rs

@@ -646,12 +646,16 @@ pub enum Expr {
        regexp: bool,
    },
    /// `ANY` operation e.g. `foo > ANY(bar)`, comparison operator is one of `[=, >, <, =>, =<, !=]`
+    /// https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html#_8_9_quantified_comparison_predicate


I'm thinking we can link to snowflake instead? Since that's a supported dialect that has the subquery functionality. The current link doesnt seem to refer to any specific dialect so it could add confusion later on when trying to figure what features in the parser to maintain

iffyio · 2024-10-08T16:32:29Z

src/parser/mod.rs

+                    // use the parenthesis for parsing the subquery as an expression.
+                    self.prev_token(); // SELECT
+                    self.prev_token(); // LParen
+                    self.parse_subexpr(precedence)?


Ah right that makes sense!

yoavcloud · 2024-10-08T16:40:43Z

Pushed doc changes as recommended.

iffyio

LGTM! cc @alamb

alamb

🚀 -- thanks @iffyio and @yoavcloud

coveralls · 2024-10-09T17:17:02Z

Pull Request Test Coverage Report for Build 11261716771

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

34 of 40 (85.0%) changed or added relevant lines in 3 files are covered.
1099 unchanged lines in 9 files lost coverage.
Overall coverage increased (+0.007%) to 89.315%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser/mod.rs	19	21	90.48%
src/ast/mod.rs	10	14	71.43%

Files with Coverage Reduction	New Missed Lines	%
tests/sqlparser_snowflake.rs	1	98.01%
src/dialect/sqlite.rs	1	80.0%
src/dialect/generic.rs	1	62.22%
tests/sqlparser_sqlite.rs	5	98.23%
src/dialect/mod.rs	12	67.51%
src/ast/ddl.rs	67	86.74%
src/parser/mod.rs	264	93.38%
src/ast/mod.rs	306	82.4%
tests/sqlparser_common.rs	442	89.57%

Totals
Change from base Build 11186281805:	0.007%
Covered Lines:	30143
Relevant Lines:	33749

💛 - Coveralls

yoavcloud · 2024-10-09T19:27:02Z

@alamb can you please trigger the workflow again? Fixed an issue with the URLs in the docs

alamb · 2024-10-09T21:47:17Z

Thanks again @yoavcloud

yoavcloud added 2 commits October 5, 2024 07:39

Add support for subqueries in quantified comparison predicates(ANY/AL…

cad0742

…L/SOME)

Code reuse and more testing

76dbe35

iffyio reviewed Oct 7, 2024

View reviewed changes

yoavcloud added 2 commits October 8, 2024 09:01

Code review comments

3860cb5

Code review comments

e4def24

yoavcloud requested a review from iffyio October 8, 2024 09:04

iffyio reviewed Oct 8, 2024

View reviewed changes

Improve docs

704fa97

yoavcloud requested a review from iffyio October 8, 2024 16:40

iffyio approved these changes Oct 8, 2024

View reviewed changes

alamb approved these changes Oct 9, 2024

View reviewed changes

Fix links in docs

2219f8e

alamb merged commit a4fa9e0 into apache:main Oct 9, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for quantified comparison predicates (ALL/ANY/SOME) #1459

Add support for quantified comparison predicates (ALL/ANY/SOME) #1459

yoavcloud commented Oct 5, 2024

iffyio Oct 7, 2024

yoavcloud Oct 8, 2024

iffyio Oct 7, 2024

yoavcloud Oct 8, 2024

iffyio Oct 8, 2024

iffyio Oct 7, 2024

yoavcloud Oct 8, 2024

iffyio Oct 7, 2024

iffyio Oct 7, 2024

yoavcloud Oct 8, 2024

iffyio Oct 8, 2024

iffyio left a comment

iffyio Oct 8, 2024

iffyio Oct 8, 2024

iffyio Oct 8, 2024

yoavcloud commented Oct 8, 2024

iffyio left a comment

alamb left a comment

coveralls commented Oct 9, 2024 •

edited

Loading

yoavcloud commented Oct 9, 2024

alamb commented Oct 9, 2024

	pub fn is_query_ahead(&mut self) -> bool {
	fn peek_sub_query(&mut self) -> bool {

	/// Checks if the next keyword indicates a query, i.e. SELECT or WITH
	/// Returns true if the next keyword indicates a sub query, i.e. SELECT or WITH

Add support for quantified comparison predicates (ALL/ANY/SOME) #1459

Add support for quantified comparison predicates (ALL/ANY/SOME) #1459

Conversation

yoavcloud commented Oct 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iffyio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoavcloud commented Oct 8, 2024

iffyio left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

coveralls commented Oct 9, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11261716771

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

yoavcloud commented Oct 9, 2024

alamb commented Oct 9, 2024

coveralls commented Oct 9, 2024 •

edited

Loading