Support `DISTINCT` for SetOperator #689

unvalley · 2022-10-26T06:38:59Z

closes #688

This supports following for GenericDialect ~~BigQuery and MySql dialects~~.

UNION DISTINCT
INTERSECT DISTINCT
EXCEPT DISTINCT

cargo fmt Fix parse_set_operator_option after next_token

tests/sqlparser_common.rs

unvalley · 2022-10-26T09:21:55Z

src/ast/query.rs

@@ -78,7 +95,7 @@ pub enum SetExpr {
    /// UNION/EXCEPT/INTERSECT of two queries
    SetOperation {
        op: SetOperator,
-        all: bool,
+        op_option: Option<SetOperatorOption>,


This change makes users change their code to use Option<SetOperatorOption>.

@unvalley I'd name it set_quantifier, as it is the name used in ANSI (1).

[1] : https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html#set-quantifier

I agree set_quantifier sounds good (along with SetOperatorOption --> SetQuantifier)

I fiexed this at ~~f6e8305~~ 826dcc5

AugustoFKL · 2022-10-29T21:38:12Z

src/ast/query.rs

@@ -64,6 +64,23 @@ impl fmt::Display for Query {
    }
 }

+/// Options for SetOperator


Would be nice having more description about the structure. Maybe a reference of which big dialects use it...

AugustoFKL · 2022-10-29T21:39:19Z

src/ast/query.rs

@@ -78,7 +95,7 @@ pub enum SetExpr {
    /// UNION/EXCEPT/INTERSECT of two queries
    SetOperation {
        op: SetOperator,
-        all: bool,
+        op_option: Option<SetOperatorOption>,


@unvalley I'd name it set_quantifier, as it is the name used in ANSI (1).

[1] : https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html#set-quantifier

AugustoFKL · 2022-10-29T21:40:10Z

src/ast/query.rs

+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            SetOperatorOption::All => write!(f, " ALL"),
+            SetOperatorOption::Distinct => write!(f, " DISTINCT"),


Printing spaces inside the enum structure doesn't make much sense.

If there's a space or not, the structure that holds it should handle it (in this case, the SetExpr).

I fixed this at f6e8305

AugustoFKL · 2022-10-29T21:42:29Z

src/parser.rs

@@ -4193,6 +4194,40 @@ impl<'a> Parser<'a> {
        }
    }

+    pub fn parse_set_operator_option(


I'd be cautious about 2 things:

Are you really sure that the ALL and DISTINCT change between operations?

You're being restrictive here. Please add GenericDialect and be aware of one thing: unless you're REALLY sure that only those 2 dialects support that feature, please be permissive and do not validate dialects.

I found that at least ANSI supports this. Please, or be permissive or validate perfectly (which is really hard)

I think ALL / DISTINCT are ANSI SQL and thus should be handled by all dialects (unless we have specific evidence that some dialects don't support them there is no reason to prevent them being parsed)

Thank you for commenting.

Are you really sure that the ALL and DISTINCT change between operations?

sorry, could you elaborate this?

You're being restrictive here. Please add GenericDialect and be aware of one thing: unless you're REALLY sure that only those 2 dialects support that feature, please be permissive and do not validate dialects.

I think ALL / DISTINCT are ANSI SQL and thus should be handled by all dialects (unless we have specific evidence that some dialects don't support them there is no reason to prevent them being parsed)

OK, I'll remove the specific dialects handling (bq, mysql) and make it parse by GenericDialect.

@unvalley you're parsing them differently. Union is being parsed with both ALL and DISTINCT, while Except and Intersect not.

Is there any reason why you can't do something like:

match op { Some(SetOperator::Union) => { if self.parse_keyword(Keyword::ALL) { Some(SetOperatorOption::All) } else if self.parse_keyword(Keyword::DISTINCT) && dialect_of!(self is MySqlDialect | BigQueryDialect) { Some(SetOperatorOption::Distinct) } else { None } } Some(SetOperator::Except) | Some(SetOperator::Intersect) => { if self.parse_keyword(Keyword::ALL) { Some(SetOperatorOption::All) } else { None } } _ => None, }

Also, FYI (not saying you must do it in this PR, besides the second one):

ANSI supports both (1).

The GenericDialect should be added to parse it as well. Unless the syntax is REALLY different between dialects, I'd say to add every syntax to the generic because, as the name states, it tries to be as generic as possible.

MySQL also accepts it in INTERSECT (2) and EXCEPT (3).

BigQuery, following your implementation, support both only for UNION (4)

[1] : https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html#query-expression-body
[2] : https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html#query-expression-body
[3] : https://dev.mysql.com/doc/refman/8.0/en/except.html
[4] : https://dev.mysql.com/doc/refman/8.0/en/except.html

@AugustoFKL Thanks, I got it. I should implement ALL and DISTINCT for INTERSECT and EXCEPT as I do for UNION. And, I should focus mainly on GenericDialect.
In this pr, I'll only implement the [1] and [2] for Generic Dialect.

I thought the BigQuery dialect should not allow ALL to be attached to INTERSECT and EXCEPT, but I see that is not correct. Because ANSI supports it, there is no trouble with parsing being done. And it is difficult to test that it cannot be parsed as you said.

@unvalley one option was to parse it conditionally for all of them, the same you do for UNION. But IMHO I think this can be scoped in another PR.

Your PR is to support UNION DISTINCT for both dialects and, parsing with Generic, you can have it.

If you want the EXACT same support for a dialect (being it permissive or restrictive), I'd say to do it separately, working from a generic scope to a restrict one. Trying to do all at once make the code confusing, and requires reading all documentations at once.

coveralls · 2022-10-31T19:02:37Z

Pull Request Test Coverage Report for Build 3393519226

27 of 29 (93.1%) changed or added relevant lines in 2 files are covered.
794 unchanged lines in 9 files lost coverage.
Overall coverage increased (+0.02%) to 85.969%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/ast/query.rs	12	13	92.31%
src/parser.rs	15	16	93.75%

Files with Coverage Reduction	New Missed Lines	%
tests/sqlparser_snowflake.rs	1	96.83%
src/lib.rs	2	10.0%
src/ast/value.rs	8	87.67%
src/ast/data_type.rs	13	88.62%
src/ast/ddl.rs	20	82.29%
tests/sqlparser_postgres.rs	23	97.81%
tests/sqlparser_common.rs	54	96.85%
src/ast/mod.rs	283	77.07%
src/parser.rs	390	83.72%

Totals
Change from base Build 3292360592:	0.02%
Covered Lines:	11053
Relevant Lines:	12857

💛 - Coveralls

alamb

Thank you @unvalley -- this is looking pretty close!

alamb · 2022-10-31T19:00:13Z

src/ast/query.rs

@@ -78,7 +95,7 @@ pub enum SetExpr {
    /// UNION/EXCEPT/INTERSECT of two queries
    SetOperation {
        op: SetOperator,
-        all: bool,
+        op_option: Option<SetOperatorOption>,


I agree set_quantifier sounds good (along with SetOperatorOption --> SetQuantifier)

alamb · 2022-10-31T19:02:09Z

src/ast/query.rs

+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+pub enum SetOperatorOption {
+    All,


In general it should be possible to round trip structures in sqlparser -- so it should be possible to distinguish between ALL, DISTINCT or nothing was specified in the query

I'll add the None to the enum SetOperatorOption, and remove Option<T> for SetOperatorOption.

I fixed this at ac41a8e

alamb · 2022-10-31T19:03:41Z

src/parser.rs

@@ -4193,6 +4194,40 @@ impl<'a> Parser<'a> {
        }
    }

+    pub fn parse_set_operator_option(


I think ALL / DISTINCT are ANSI SQL and thus should be handled by all dialects (unless we have specific evidence that some dialects don't support them there is no reason to prevent them being parsed)

src/ast/query.rs

unvalley · 2022-11-04T11:53:46Z

src/ast/query.rs

+/// A quantifier for [SetOperator].
+// TODO: Restrict parsing specific SetQuantifier in some specific dialects.
+// For example, BigQuery does not support `DISTINCT` for `EXCEPT` and `INTERSECT`


I added some comments at ae6fb81 , but I wasn't quite sure how to describe this enum🥲

unvalley · 2022-11-04T11:54:57Z

sorry for the notification.. I didn't know I can set one reviewer only.

alamb · 2022-11-04T19:18:57Z

sorry for the notification.. I didn't know I can set one reviewer only.

No worries -- without these notifications it takes me a while to find PRs to review!

alamb

I think this looks great @unvalley -- thank you!

Any concerns @AugustoFKL ?

AugustoFKL

LGTM

alamb · 2022-11-07T12:06:08Z

Thanks again @unvalley !

unvalley added 3 commits October 26, 2022 14:33

Update SetOperation field all to op_option

55b1d2d

Implement parse_set_operator_option

e424471

cargo fmt Fix parse_set_operator_option after next_token

Add test for parsing union distinct

0ac391c

unvalley marked this pull request as ready for review October 26, 2022 09:16

unvalley commented Oct 26, 2022

View reviewed changes

tests/sqlparser_common.rs Outdated Show resolved Hide resolved

unvalley commented Oct 26, 2022

View reviewed changes

AugustoFKL suggested changes Oct 29, 2022

View reviewed changes

Rename to SetQualifier and fix fmt space

f6e8305

alamb reviewed Oct 31, 2022

View reviewed changes

unvalley added 5 commits November 4, 2022 19:20

Add None to SetQualifier

ac41a8e

Update parse method

0337257

Rename to SetQuantifier

826dcc5

Rename parse_set_operator_option parse_set_operator

03e4950

Fix test to parse union, except, intersect

e52e567

unvalley changed the title ~~Support UNION DISTINCT~~ Support DISTINCT for SetOperator Nov 4, 2022

unvalley commented Nov 4, 2022

View reviewed changes

src/ast/query.rs Show resolved Hide resolved

unvalley added 2 commits November 4, 2022 20:40

Add some comments to SetQuantifier

ae6fb81

Fix comment

f336342

unvalley requested review from AugustoFKL and alamb and removed request for AugustoFKL November 4, 2022 11:52

unvalley commented Nov 4, 2022

View reviewed changes

unvalley requested review from AugustoFKL and removed request for alamb November 4, 2022 11:54

alamb approved these changes Nov 4, 2022

View reviewed changes

AugustoFKL approved these changes Nov 7, 2022

View reviewed changes

alamb merged commit f7817bc into sqlparser-rs:main Nov 7, 2022

unvalley deleted the union-distinct branch November 7, 2022 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `DISTINCT` for SetOperator #689

Support `DISTINCT` for SetOperator #689

unvalley commented Oct 26, 2022 •

edited

unvalley Oct 26, 2022 •

edited

AugustoFKL Oct 29, 2022

alamb Oct 31, 2022

unvalley Nov 4, 2022 •

edited

AugustoFKL Oct 29, 2022

AugustoFKL Oct 29, 2022

AugustoFKL Oct 29, 2022

unvalley Nov 4, 2022 •

edited

AugustoFKL Oct 29, 2022

alamb Oct 31, 2022

unvalley Nov 1, 2022 •

edited

AugustoFKL Nov 1, 2022

unvalley Nov 1, 2022 •

edited

AugustoFKL Nov 1, 2022

coveralls commented Oct 31, 2022 •

edited

alamb left a comment •

edited

alamb Oct 31, 2022

alamb Oct 31, 2022

unvalley Nov 1, 2022

unvalley Nov 4, 2022

alamb Oct 31, 2022

unvalley Nov 4, 2022 •

edited

unvalley commented Nov 4, 2022 •

edited

alamb commented Nov 4, 2022

alamb left a comment

AugustoFKL left a comment

alamb commented Nov 7, 2022

Support DISTINCT for SetOperator #689

Support DISTINCT for SetOperator #689

Conversation

unvalley commented Oct 26, 2022 • edited

unvalley Oct 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unvalley Nov 4, 2022 • edited

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

unvalley Nov 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unvalley Nov 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unvalley Nov 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 31, 2022 • edited

Pull Request Test Coverage Report for Build 3393519226

💛 - Coveralls

alamb left a comment • edited

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

unvalley Nov 4, 2022 • edited

Choose a reason for hiding this comment

unvalley commented Nov 4, 2022 • edited

alamb commented Nov 4, 2022

alamb left a comment

Choose a reason for hiding this comment

AugustoFKL left a comment

Choose a reason for hiding this comment

alamb commented Nov 7, 2022

Support `DISTINCT` for SetOperator #689

Support `DISTINCT` for SetOperator #689

unvalley commented Oct 26, 2022 •

edited

unvalley Oct 26, 2022 •

edited

unvalley Nov 4, 2022 •

edited

unvalley Nov 4, 2022 •

edited

unvalley Nov 1, 2022 •

edited

unvalley Nov 1, 2022 •

edited

coveralls commented Oct 31, 2022 •

edited

alamb left a comment •

edited

unvalley Nov 4, 2022 •

edited

unvalley commented Nov 4, 2022 •

edited