hudi-agent commented on code in PR #19505:
URL: https://github.com/apache/hudi/pull/19505#discussion_r3869557732
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/catalyst/catalog/HoodieCatalogTable.scala:
##########
@@ -290,6 +287,42 @@ class HoodieCatalogTable(val spark: SparkSession, var
table: CatalogTable) exten
(finalSchema, tableConfigs)
}
+ /**
+ * Resolves the partition fields to persist in the table config, preferring
an explicitly configured
+ * partition path field over the columns of the PARTITIONED BY clause.
+ */
+ private def resolvePartitionColumns(tableConfigs: Map[String, String]):
String = {
+ if (SparkConfigUtils.containsConfigProperty(tableConfigs,
KeyGeneratorOptions.PARTITIONPATH_FIELD_NAME)) {
+ SparkConfigUtils.getStringWithAltKeys(tableConfigs,
KeyGeneratorOptions.PARTITIONPATH_FIELD_NAME)
+ } else if (table.partitionColumnNames.isEmpty) {
+ null
+ } else {
+ table.partitionColumnNames.mkString(",")
+ }
+ }
+
+ /**
+ * A non partitioned key generator never produces a partition path, so
pairing one with partition
+ * columns describes a table that cannot exist. Creating it anyway persists
a table config that
+ * disagrees with itself, and every subsequent write is rejected for a
partition path conflict that
+ * names neither the key generator nor the partition columns. Reject it
while the statement that
+ * introduced it is still in hand.
+ */
+ private def validateKeyGeneratorForPartitionColumns(tableConfigs:
Map[String, String]): Unit = {
+ val partitionColumns = resolvePartitionColumns(tableConfigs)
+ if (!StringUtils.isNullOrEmpty(partitionColumns)) {
+ val keyGenerator =
KeyGeneratorType.getKeyGeneratorClassName(tableConfigs.asJava)
+ if (KeyGeneratorType.NON_PARTITION.getClassName == keyGenerator
Review Comment:
🤖 `GlobalDeleteKeyGenerator`/`GlobalAvroDeleteKeyGenerator` also return no
partition path fields, so pairing them with partition columns produces the same
self-disagreeing config this guard is meant to catch. Was leaving them out
intentional (to keep scope tight), or should they be included in this check too?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/catalyst/catalog/HoodieCatalogTable.scala:
##########
@@ -290,6 +287,42 @@ class HoodieCatalogTable(val spark: SparkSession, var
table: CatalogTable) exten
(finalSchema, tableConfigs)
}
+ /**
+ * Resolves the partition fields to persist in the table config, preferring
an explicitly configured
+ * partition path field over the columns of the PARTITIONED BY clause.
+ */
+ private def resolvePartitionColumns(tableConfigs: Map[String, String]):
String = {
+ if (SparkConfigUtils.containsConfigProperty(tableConfigs,
KeyGeneratorOptions.PARTITIONPATH_FIELD_NAME)) {
+ SparkConfigUtils.getStringWithAltKeys(tableConfigs,
KeyGeneratorOptions.PARTITIONPATH_FIELD_NAME)
+ } else if (table.partitionColumnNames.isEmpty) {
+ null
+ } else {
+ table.partitionColumnNames.mkString(",")
+ }
+ }
+
+ /**
+ * A non partitioned key generator never produces a partition path, so
pairing one with partition
+ * columns describes a table that cannot exist. Creating it anyway persists
a table config that
+ * disagrees with itself, and every subsequent write is rejected for a
partition path conflict that
+ * names neither the key generator nor the partition columns. Reject it
while the statement that
+ * introduced it is still in hand.
+ */
+ private def validateKeyGeneratorForPartitionColumns(tableConfigs:
Map[String, String]): Unit = {
+ val partitionColumns = resolvePartitionColumns(tableConfigs)
+ if (!StringUtils.isNullOrEmpty(partitionColumns)) {
+ val keyGenerator =
KeyGeneratorType.getKeyGeneratorClassName(tableConfigs.asJava)
+ if (KeyGeneratorType.NON_PARTITION.getClassName == keyGenerator
+ || KeyGeneratorType.NON_PARTITION_AVRO.getClassName == keyGenerator) {
+ throw new HoodieAnalysisException(
+ s"Cannot create table '$catalogTableName' partitioned by
'$partitionColumns' using key generator"
+ + s" '$keyGenerator', which does not generate a partition path.
Either drop the partition"
+ + s" columns, or configure a partitioned key generator through"
+ + s" '${HoodieTableConfig.KEY_GENERATOR_CLASS_NAME.key}'.")
Review Comment:
🤖 nit: Yoda-style comparisons (`constant == variable`) are idiomatic Java
but feel a little off in Scala — could you flip these to `keyGenerator ==
KeyGeneratorType.NON_PARTITION.getClassName || keyGenerator ==
KeyGeneratorType.NON_PARTITION_AVRO.getClassName`?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
