Skip to content

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

Merged
alamb merged 1 commit into
apache:mainfrom
alamb:alamb/fix_tuple_parsing
Feb 8, 2022
Merged

Add Expr::Tuple and parsing support, remove special case in DISTINCT clause#414
alamb merged 1 commit into
apache:mainfrom
alamb:alamb/fix_tuple_parsing

Conversation

@alamb

@alamb alamb commented Feb 8, 2022

Copy link
Copy Markdown
Contributor

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
Comment thread tests/sqlparser_common.rs
);

// Tuples can also be in the set
one_statement_parses_to(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@coveralls

coveralls commented Feb 8, 2022

Copy link
Copy Markdown

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

Comment thread src/parser.rs
None
};

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

Copy link
Copy Markdown
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

@e-dard e-dard left a comment

Copy link
Copy Markdown

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
Copy Markdown
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