Allow declaring partition columns in `PARTITION BY` clause, backwards compatible #9599

MohamedAbdeen21 · 2024-03-13T14:35:09Z

Which issue does this PR close?

Closes #9465.

Rationale for this change

Allow HiveQL syntax when creating external tables with partition columns only defined inside the PARTITION BY clause, while maintaining original syntax (backwards compatible).

What changes are included in this PR?

SQL Parser now parses:

CREATE EXTERNAL TABLE test(trace_id varchar) 
STORED AS parquet
PARTITIONED BY (partition varchar)
LOCATION '/tmp/test/';

as if it was:

CREATE EXTERNAL TABLE test(trace_id varchar, partition varchar) 
STORED AS parquet
PARTITIONED BY (partition)
LOCATION '/tmp/test/';

This means that queries re-defining columns in PARTITIONED BY are rejected with Schema error

Are these changes tested?

Yes

Are there any user-facing changes?

User is now able to use HiveQL syntax when creating partitioned external tables.

MohamedAbdeen21 · 2024-03-13T14:40:31Z

datafusion/sql/src/parser.rs

@@ -679,7 +679,25 @@ impl<'a> DFParser<'a> {
                    Keyword::PARTITIONED => {
                        self.parser.expect_keyword(Keyword::BY)?;
                        ensure_not_set(&builder.table_partition_cols, "PARTITIONED BY")?;
-                        builder.table_partition_cols = Some(self.parse_partitions()?);
+                        let peeked = self.parser.peek_nth_token(2);
+                        if peeked == Token::Comma || peeked == Token::RParen {


Works fine but feels hacky. Considering replacing this if with a more robust function that tries to apply a parsing rule and falls back (undo consumed tokens) when rule fails.

Yeah, it is not immediately clear why this logic works. I think at a minimum we should add a comment explaining the reasoning of this condition.

I'm not super familiar with sqlparser crate, but I don't think it allows rewinding tokens, we will have to implement a parsing rule that only uses peeks, which sounds really unnecessary. Will add a comment for now and maybe can find a better way.

I think you can use the poorly named consume_tokens API:

https://docs.rs/sqlparser/latest/sqlparser/parser/struct.Parser.html#method.consume_tokens

That looks promising. I'll open a follow up PR if it works out. Thanks for pointing it out.

Edit: misread the docs, we need a version of consume_tokens that returns true without actually consuming the tokens, because we need to parse the tokens later.

Also, would be perfect if it can match a pattern to catch mixing syntax in the clause.

devinjdangelo · 2024-03-14T12:41:29Z

Thank you @MohamedAbdeen21! This looks good. I haven't had a chance to dig into this deeply yet, but I plan to sometime over the next few days when I get some time.

I think one thing we will definitely want prior to merging this is test cases exploring possible edge cases: e.g. validating it isn't possible to mix the two syntaxes in any way to lead to undesired behavior.

devinjdangelo

Thank you @MohamedAbdeen21 this is looking very close to ready (and certainly ready for wider review/discussion). I left a few comments. Would you mind marking this "ready for review" to increase visibility for other reviewers?

cc @alamb @metesynnada

devinjdangelo · 2024-03-23T12:31:28Z

datafusion/sql/src/parser.rs

@@ -679,7 +679,25 @@ impl<'a> DFParser<'a> {
                    Keyword::PARTITIONED => {
                        self.parser.expect_keyword(Keyword::BY)?;
                        ensure_not_set(&builder.table_partition_cols, "PARTITIONED BY")?;
-                        builder.table_partition_cols = Some(self.parse_partitions()?);
+                        let peeked = self.parser.peek_nth_token(2);
+                        if peeked == Token::Comma || peeked == Token::RParen {


Yeah, it is not immediately clear why this logic works. I think at a minimum we should add a comment explaining the reasoning of this condition.

devinjdangelo · 2024-03-23T12:31:40Z

datafusion/sql/src/parser.rs

@@ -175,7 +175,7 @@ pub(crate) type LexOrdering = Vec<OrderByExpr>;
 /// [ WITH HEADER ROW ]
 /// [ DELIMITER <char> ]
 /// [ COMPRESSION TYPE <GZIP | BZIP2 | XZ | ZSTD> ]
-/// [ PARTITIONED BY (<column list>) ]
+/// [ PARTITIONED BY (<column_definition list> | <column list>) ]


datafusion/sqllogictest/test_files/create_external_table.slt

alamb

Thank you @MohamedAbdeen21 -- I am sorry for the delay in reviewing this PR. It looks really nice to me.

Thank you @devinjdangelo for the review.

Is there any chance you can merge up from main so we can make sure there are no conflicts? If not I can handle it too

alamb · 2024-03-30T20:14:48Z

Thanks again @MohamedAbdeen21

… compatible (apache#9599) * Draft allow both syntaxes * suppress unused code error * prevent constraints in partition clauses * fix clippy * More tests * comment and prevent constraints on partition columns * trailing whitespaces * End-to-End test of new Hive syntax --------- Co-authored-by: Mohamed Abdeen <[email protected]>

Mohamed Abdeen added 3 commits March 13, 2024 16:07

Draft allow both syntaxes

e064f73

suppress unused code error

133de66

prevent constraints in partition clauses

8f75d96

github-actions bot added the sql SQL Planner label Mar 13, 2024

fix clippy

7639ca1

MohamedAbdeen21 commented Mar 13, 2024

View reviewed changes

More tests

8de8a8b

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Mar 14, 2024

MohamedAbdeen21 added 2 commits March 14, 2024 21:19

Merge branch 'main' into extern-partition-syntax

693248f

Merge branch 'main' into extern-partition-syntax

6e7048c

devinjdangelo reviewed Mar 23, 2024

View reviewed changes

comment and prevent constraints on partition columns

70a0fec

MohamedAbdeen21 marked this pull request as ready for review March 23, 2024 13:30

Mohamed Abdeen added 2 commits March 23, 2024 16:18

trailing whitespaces

e60f4b4

End-to-End test of new Hive syntax

87aa4cf

alamb mentioned this pull request Mar 24, 2024

DataFusion weekly project plan (Andrew Lamb) - March 18, 2024 #9675

Closed

7 tasks

devinjdangelo mentioned this pull request Mar 24, 2024

[BUG] Panic when querying table with wrong partition columns order #9785

Closed

alamb mentioned this pull request Mar 25, 2024

DataFusion weekly project plan (Andrew Lamb) - March 25, 2024 #9796

Closed

6 tasks

alamb approved these changes Mar 29, 2024

View reviewed changes

Merge branch 'apache:main' into extern-partition-syntax

e93fea5

alamb merged commit ab88220 into apache:main Mar 30, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow declaring partition columns in `PARTITION BY` clause, backwards compatible #9599

Allow declaring partition columns in `PARTITION BY` clause, backwards compatible #9599

MohamedAbdeen21 commented Mar 13, 2024 •

edited

Loading

MohamedAbdeen21 Mar 13, 2024 •

edited

Loading

devinjdangelo Mar 23, 2024

MohamedAbdeen21 Mar 23, 2024

alamb Mar 29, 2024

MohamedAbdeen21 Mar 29, 2024 •

edited

Loading

devinjdangelo commented Mar 14, 2024

devinjdangelo left a comment

devinjdangelo Mar 23, 2024

devinjdangelo Mar 23, 2024

alamb left a comment

alamb commented Mar 30, 2024

Allow declaring partition columns in PARTITION BY clause, backwards compatible #9599

Allow declaring partition columns in PARTITION BY clause, backwards compatible #9599

Conversation

MohamedAbdeen21 commented Mar 13, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

MohamedAbdeen21 Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

devinjdangelo Mar 23, 2024

Choose a reason for hiding this comment

MohamedAbdeen21 Mar 23, 2024

Choose a reason for hiding this comment

alamb Mar 29, 2024

Choose a reason for hiding this comment

MohamedAbdeen21 Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

devinjdangelo commented Mar 14, 2024

devinjdangelo left a comment

Choose a reason for hiding this comment

devinjdangelo Mar 23, 2024

Choose a reason for hiding this comment

devinjdangelo Mar 23, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb commented Mar 30, 2024

Allow declaring partition columns in `PARTITION BY` clause, backwards compatible #9599

Allow declaring partition columns in `PARTITION BY` clause, backwards compatible #9599

MohamedAbdeen21 commented Mar 13, 2024 •

edited

Loading

MohamedAbdeen21 Mar 13, 2024 •

edited

Loading

MohamedAbdeen21 Mar 29, 2024 •

edited

Loading