Skip to content

Add Expr::Tuple and parsing support, remove special case in DISTINCT clause #414

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

Merged
merged 1 commit into from
Feb 8, 2022

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Feb 8, 2022

Rationale:

@yuval-illumex is trying to support parenthesized expressions like (a, b) in various places (see #390, and #391)

I believe these are more commonly called tuple or row types in postgres or trino. Rather than adding special case handling for parens in various places, it would be better to simply support parsing tuples in general

Changes

  1. Support parsing tuple expressions in select statement
  2. Tests for same
  3. Remove special case added in Allow parentheses in GROUP BY #390 for distinct clause
  4. Add tests in Support parentheses in DISTINCT clause, CURRENT_TIMESTAMP, CURRENT_TIME, and CURRENT_DATE #391 demonstrating this code can parse paren'd expressions in the group by

Reference:

This syntax is supported by postgres:

alamb=# select (1, 'foo');
   row   
---------
 (1,foo)
(1 row)

It is not supported by mysql:

mysql> select (1, 2);
ERROR 1241 (21000): Operand should contain 1 column(s)

@alamb alamb force-pushed the alamb/fix_tuple_parsing branch from 9c4697a to 068c40d Compare February 8, 2022 14:32
@@ -1121,6 +1171,12 @@ fn parse_select_group_by() {
],
select.group_by
);

// Tuples can also be in the set
one_statement_parses_to(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@coveralls
Copy link

coveralls commented Feb 8, 2022

Pull Request Test Coverage Report for Build 1812838772

  • 38 of 39 (97.44%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 90.524%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser.rs 4 5 80.0%
Totals Coverage Status
Change from base Build 1807150916: 0.04%
Covered Lines: 6897
Relevant Lines: 7619

💛 - Coveralls

@@ -2680,19 +2685,8 @@ impl<'a> Parser<'a> {
None
};

// Not Sure if Top should be cheked here as well. Trino doesn't support TOP.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was added in #390 but is now covered by the general purpose Tuple parsing

Copy link

@e-dard e-dard left a comment

Choose a reason for hiding this comment

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

I took a whizz through this and it made sense to me 👍

@yuval-illumex
Copy link
Contributor

@alamb Much appreciated!!! Looks great :)

@alamb alamb merged commit ff558ee into apache:main Feb 8, 2022
@alamb alamb deleted the alamb/fix_tuple_parsing branch February 8, 2022 15:54
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.

4 participants