fix(spark-sql): reject creating a table that pairs a non partitioned key generator with partition columns - #19505
Conversation
…key generator with partition columns A non partitioned key generator never produces a partition path, so pairing one with PARTITIONED BY describes a table that cannot exist. CREATE TABLE accepted it and persisted a table config that disagrees with itself: the partition fields came from the partition columns while the key generator came from the table properties, and nothing cross checked the two. Every subsequent write to such a table is rejected for a partition path conflict that names neither the key generator nor the partition columns, so the table is unusable from the moment it is created. Reject the combination while the statement that introduced it is still in hand. The check applies only to a table being created, so an existing table already in this state stays readable and can still be registered.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR rejects CREATE TABLE statements that pair a non-partitioned key generator with partition columns, failing fast with a message naming both instead of persisting a self-contradictory table config that no write can succeed against. I traced the validation path (it's on the shared (_, false) create arm covering both managed and external tables), confirmed resolvePartitionColumns is a behavior-preserving extraction, and checked that the guard only fires on the genuinely contradictory case (non-empty partition columns + NON_PARTITION/NON_PARTITION_AVRO keygen resolved from either class name or type), so there are no false positives. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. One small Scala style nit below; the logic and naming are otherwise clean.
cc @yihua
| val partitionColumns = resolvePartitionColumns(tableConfigs) | ||
| if (!StringUtils.isNullOrEmpty(partitionColumns)) { | ||
| val keyGenerator = KeyGeneratorType.getKeyGeneratorClassName(tableConfigs.asJava) | ||
| if (KeyGeneratorType.NON_PARTITION.getClassName.equals(keyGenerator) |
There was a problem hiding this comment.
🤖 nit: could you use Scala's == here instead of .equals()? KeyGeneratorType.NON_PARTITION.getClassName == keyGenerator || KeyGeneratorType.NON_PARTITION_AVRO.getClassName == keyGenerator reads more naturally in Scala and is equivalent for String.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19505 +/- ##
============================================
- Coverage 77.03% 76.99% -0.04%
- Complexity 33863 33883 +20
============================================
Files 2575 2575
Lines 143379 143409 +30
Branches 17574 17581 +7
============================================
- Hits 110451 110424 -27
- Misses 24666 24730 +64
+ Partials 8262 8255 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
5 participants
Describe the issue this Pull Request addresses
Closes #15589 / HUDI-5263.
Spark SQL accepts a
CREATE TABLEthat declares partition columns and, at the sametime, configures a key generator that never produces a partition path:
The statement succeeds and persists a table config that disagrees with itself. The
partition fields are taken from the partition columns in
initHoodieTable, while thekey generator is taken from the table properties, which override the value
extraTableConfiginferred. Nothing cross checks the two.The resulting table cannot be written to at all. Running against master, every write
path is rejected identically:
So the behaviour has moved on from the original report: the null partition column is
no longer reachable, because a later guard blocks the write. What remains is that the
broken table is still created, and the eventual failure names neither the key
generator nor the partition columns, which are the two things actually in conflict.
Summary and Changelog
Creating a table with a non partitioned key generator and partition columns now fails
immediately, with a message that names both, instead of producing a table that cannot
be written to.
validateKeyGeneratorForPartitionColumnstoHoodieCatalogTable, invoked fromparseSchemaAndConfigson the path that creates a table.resolvePartitionColumnsfrominitHoodieTableso the validation and thevalue that gets persisted are derived the same way and cannot drift apart.
NonpartitionedKeyGeneratorandNonpartitionedAvroKeyGenerator, resolvedthrough
KeyGeneratorType.getKeyGeneratorClassNameso the key generator class andkey generator type properties are both honoured.
in this state stays readable and can still be registered in the catalog, so nobody is
locked out of data they already have.
TestCreateTable: the rejection, for both non partitioned keygenerator classes, and a companion test asserting a genuinely non partitioned table
is still created and still writable, so the guard cannot over fire.
The new error reads:
Impact
A
CREATE TABLEthat previously succeeded now fails. This breaks no working workflow:a table created that way is rejected on every subsequent write, as shown above, so the
statement only ever produced an unusable table. The failure simply moves to the
statement that causes it.
Confined to the Spark SQL table creation path in
HoodieCatalogTable. No change to theread path, the write path, or the datasource API. Tables that already exist in this
state are untouched.
Risk Level
low
The validation runs only while creating a table, and only fires on a combination that
is provably unusable. Verified that no existing test pairs a non partitioned key
generator with partition columns; the whole
org.apache.spark.sql.hudi.ddlpackagepasses.
Documentation Update
None. No new config, no public API change.
Contributor's checklist