hudi-agent commented on code in PR #19509:
URL: https://github.com/apache/hudi/pull/19509#discussion_r3915687302
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/TestDataSourceUtils.java:
##########
@@ -247,6 +262,65 @@ public void
testCreateRDDCustomColumnsSortPartitionerWithValidPartitioner() thro
assertThat(partitioner.isPresent(), is(true));
}
+ /**
+ * Every out of the box bulk insert partitioner has to be usable as a user
defined partitioner.
+ * One is instantiated by reflection with only the write config, so each has
to expose a
+ * constructor taking only a {@link HoodieWriteConfig}. See HUDI-7526.
+ */
+ @ParameterizedTest
+ @ValueSource(classes = {
+ NonSortPartitioner.class,
+ GlobalSortPartitioner.class,
+ RDDPartitionSortPartitioner.class,
+ RDDCustomColumnsSortPartitioner.class,
+ PartitionPathRepartitionPartitioner.class,
+ PartitionPathRepartitionAndSortPartitioner.class,
+ NonSortPartitionerWithRows.class,
+ GlobalSortPartitionerWithRows.class,
+ PartitionSortPartitionerWithRows.class,
+ RowCustomColumnsSortPartitioner.class,
+ RowSpatialCurveSortPartitioner.class,
+ PartitionPathRepartitionPartitionerWithRows.class,
+ PartitionPathRepartitionAndSortPartitionerWithRows.class
+ })
+ public void
testBuiltInPartitionersAreUsableAsUserDefinedPartitioners(Class<?>
partitionerClass) {
+ Map<String, String> props = new HashMap<>();
+ // required by the spatial curve partitioner, ignored by the rest
+ props.put(HoodieClusteringConfig.PLAN_STRATEGY_SORT_COLUMNS.key(),
"column1,column2");
+ config = HoodieWriteConfig.newBuilder()
+ .withPath("/")
+ .withUserDefinedBulkInsertPartitionerClass(partitionerClass.getName())
+ .withUserDefinedBulkInsertPartitionerSortColumns("column1,column2")
+ .withSchema(avroSchemaString)
+ .withProps(props)
+ .build();
+
+
assertThat(DataSourceUtils.createUserDefinedBulkInsertPartitioner(config).isPresent(),
is(true));
+
assertThat(DataSourceUtils.createUserDefinedBulkInsertPartitionerWithRows(config).isPresent(),
is(true));
+ }
+
+ /**
+ * The partition path partitioners take the flag from the table when built
by the factory, so
+ * check the write config only constructor derives it from the configured
partition path field.
Review Comment:
🤖 nit: the method name
`testPartitionPathRepartitionPartitionerDerivesIsTablePartitioned` points to a
concrete class, but the body is really exercising
`BulkInsertPartitioner.isTablePartitioned()` — could you rename it to something
like `testIsTablePartitionedDerivesFromPartitionPathField` so it stays accurate
if the helper ever moves?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/BulkInsertPartitioner.java:
##########
@@ -71,4 +74,22 @@ default String getFileIdPfx(int partitionId) {
default Option<WriteHandleFactory> getWriteHandleFactory(int partitionId) {
return Option.empty();
}
+
+ /**
+ * Whether the records being written carry a partition path, derived from
the write config alone.
+ * <p>
+ * A partitioner named through
+ * {@code HoodieWriteConfig.BULKINSERT_USER_DEFINED_PARTITIONER_CLASS_NAME}
is instantiated by
+ * reflection with only the write config, so an implementation that
otherwise takes the flag from
+ * the {@link HoodieTable} has nothing else to derive it from. The write
side partition path field
+ * governs whether records end up with a non-empty partition path, which is
what those
+ * implementations branch on.
+ *
+ * @param config Write config.
+ * @return {@code true} if a partition path field is configured; {@code
false} otherwise.
+ */
+ static boolean isTablePartitioned(HoodieWriteConfig config) {
+ return !StringUtils.isNullOrEmpty(
Review Comment:
🤖 Confirmed the two sources genuinely differ: the factory path resolves
`table.isPartitioned()` → `HoodieTableConfig.isTablePartitioned()` (backed by
`hoodie.table.partition.fields`), whereas this new method reads
`hoodie.datasource.write.partitionpath.field`. One thing worth noting on
severity: the built-in sort-mode path is untouched — it still threads
`table.isPartitioned()` through the two-arg constructor — so the divergence
only affects the new reflection path, and since each record already carries its
own partition path, a wrong `false` here degrades `partitionBy` to `coalesce()`
(a shuffle/layout + parallelism difference, not data loss). That said,
reconciling or at least documenting the intended precedence still seems
worthwhile, since a custom key generator can produce non-empty partition paths
without `partitionpath.field` being set, and there the reflection path would
repartition differently from the built-in sort mode.
--
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]
