Repository navigation
Hive: Parse and correctly display SKEWED BY clauses - #2395
wenxiaojie1 wants to merge 5 commits into
Conversation
| fn display_create_table_with_skewed_by() { | ||
| let column = |name| ColumnDef { | ||
| name: Ident::new(name), | ||
| data_type: DataType::String(None), | ||
| options: vec![], | ||
| }; | ||
| let stmt = CreateTableBuilder::new(ObjectName::from(vec![Ident::new("test")])) | ||
| .columns(vec![column("id")]) | ||
| .hive_distribution(HiveDistributionStyle::SKEWED { | ||
| columns: vec![column("id")], | ||
| on: vec![column("id")], | ||
| stored_as_directories: true, | ||
| }) | ||
| .build(); | ||
|
|
||
| assert_eq!( | ||
| stmt.to_string(), | ||
| "CREATE TABLE test (id STRING) SKEWED BY (id STRING) ON (id STRING) STORED AS DIRECTORIES" | ||
| ); |
There was a problem hiding this comment.
can we simplify the test to use the format here for example?
https://github.com/wenxiaojie1/datafusion-sqlparser-rs/blob/30b2a5a686135af090cf7e757070143f36cdff84/tests/sqlparser_hive.rs#L122-L125
There was a problem hiding this comment.
Thanks. I tried the verified_stmt form, but the current Hive parser rejects this syntax with Expected: end of statement, found: SKEWED. Issue #1499 explicitly notes that SKEWED BY cannot currently be parsed. I kept the explicit AST construction so the test remains scoped to the reported display bug rather than adding parser support in this PR. The focused test still passes.
There was a problem hiding this comment.
not sure I follow, if the parser doesn't accept the syntax then sounds like something more fundamental is wrong if the test was valid?
There was a problem hiding this comment.
That separation is intentional for this issue. #1499 explicitly notes that SKEWED BY cannot currently be parsed and reports the extra ) in the existing CreateTable display path. HiveDistributionStyle::SKEWED is nevertheless a public AST variant that callers can construct programmatically (including through CreateTableBuilder), so its malformed SQL output is independently observable.
The AST-construction test exercises that exact display regression without expanding this small fix into parser support; parse_hive_distribution still has the separate TODO: Support parsing for SKEWED distribution style. If parser support is intended to be a prerequisite for fixing the formatter, I can revise the scope, but I read #1499 as deliberately limited to stringification.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2395 +/- ##
==========================================
+ Coverage 81.24% 81.30% +0.06%
==========================================
Files 42 42
Lines 33901 34003 +102
Branches 33901 34003 +102
==========================================
+ Hits 27543 27647 +104
+ Misses 2792 2783 -9
- Partials 3566 3573 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
LucaCappelletti94
left a comment
LucaCappelletti94
left a comment
There was a problem hiding this comment.
Removing the extra ) is correct, but the new test can't catch a wrong rendering, and that is why CI is currently not red.
It builds the AST by hand and compares to_string() with a hand-written string that matches what the printer emits, and since the parser has no SKEWED BY support at this time, nothing reads that string back. That is why CI is green even though the expected string is invalid Hive. Hive 4.0.1 rejects CREATE TABLE test (id STRING) SKEWED BY (id STRING) ON (id STRING) STORED AS DIRECTORIES with ParseException line 1:42 extraneous input 'STRING' expecting ) near '(' in skewed value element.
The cause is the AST shape. Hive's SKEWED BY takes bare column names and its ON takes constant values such as ON ('a', 'b'), while HiveDistributionStyle::SKEWED stores both as Vec<ColumnDef>. No value of the current variant renders a valid ON clause, and even SKEWED BY (id) ON (id) is rejected. This is the more fundamental problem @iffyio pointed at.
Could you change SKEWED to hold the column identifiers and the skewed value tuples, parse it in parse_hive_distribution, and test it with hive().verified_stmt(...)? Hive also expects SKEWED BY after CLUSTERED BY ... INTO n BUCKETS, while the display writes it before, so the round trip should cover that order too.
LucaCappelletti94
added
enhancement
wenxiaojie1
changed the title
4 participants
Fixes #1499.
Example:
CREATE TABLE t (id STRING) SKEWED BY (id) ON ('a', 'b') STORED AS DIRECTORIES.PARTITIONED BY->CLUSTERED BY->SKEWED BYorder, including combined partitioned/skewed tables.Syntax checked against the official Hive DDL and Hive 4.0.1 grammar.
Checks:
cargo test --all-features(1707 passed),cargo fmt --all -- --check,cargo +nightly clippy --all-targets --all-features -- -D warnings.Local fuzzing is blocked by Windows/MSVC libFuzzer linker errors; no iterations completed. Upstream CI requires maintainer approval.
AI assistance was used for implementation and tests.