deepakpanda93 commented on code in PR #19509:
URL: https://github.com/apache/hudi/pull/19509#discussion_r3916698376
##########
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:
Good catch — this is a real divergence and I have reconciled it rather than
only documenting it.
Confirmed the two sources first: the factory path threads
`HoodieTable#isPartitioned()`, which is
`getMetaClient().getTableConfig().isTablePartitioned()` and resolves to
`getPartitionFields()` over `hoodie.table.partition.fields`. The method added
here read `hoodie.datasource.write.partitionpath.field`. Different keys, so
they can disagree exactly as described.
The fix prefers the property the factory uses, and evaluates it with Hudi's
own logic rather than a second implementation of it — `HoodieWriteConfig
extends HoodieConfig`, so `HoodieTableConfig.getPartitionFields` takes the
write config directly:
```java
static boolean isTablePartitioned(HoodieWriteConfig config) {
Option<String[]> partitionFields =
HoodieTableConfig.getPartitionFields(config);
if (partitionFields.isPresent()) {
return partitionFields.get().length > 0;
}
return !StringUtils.isNullOrEmpty(
config.getProps().getProperty(KeyGeneratorOptions.PARTITIONPATH_FIELD_NAME.key()));
}
```
So the reflection path now resolves identically to the built-in sort mode
whenever the table properties reached the write config, including the
empty-value case, since `getPartitionFields` filters empty entries and `length
> 0` then gives `false`. The write side field is used only when
`hoodie.table.partition.fields` is absent, which is the case the fallback
exists for — a write config assembled without the table's properties. The
javadoc now states that precedence explicitly.
Your follow-up note on severity matches what I found, and it is worth
recording that the fallback is still a heuristic: a custom key generator can
produce non-empty partition paths with no `partitionpath.field` set, and with
no table property present there is nothing better to read. That case now
behaves as before rather than getting worse.
Covered by `testIsTablePartitionedPrefersTableConfigOverPartitionPathField`,
which includes the case where the two keys disagree. Negative control: with the
previous one-line body restored, that test fails (`but: was <false>`), so it
detects the bug rather than merely passing. The companion
`testIsTablePartitionedFallsBackToPartitionPathField` passes either way and is
a regression guard, not a bug detector.
Verified on the rebased branch: checkstyle clean on the three client
modules, `TestDataSourceUtils` 26/26,
`TestJavaBulkInsertInternalPartitionerFactory` 7/7.
##########
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:
Renamed, though not to exactly that name, because the reconciliation in the
other thread changed what the method reads.
`isTablePartitioned` now prefers `hoodie.table.partition.fields` and falls
back to `hoodie.datasource.write.partitionpath.field`, so
`testIsTablePartitionedDerivesFromPartitionPathField` would only describe half
of it. Split into two tests, both dropping the concrete class name, which was
your point:
- `testIsTablePartitionedPrefersTableConfigOverPartitionPathField` — the
table property wins when present, including when the two keys disagree
- `testIsTablePartitionedFallsBackToPartitionPathField` — the original three
cases, now scoped to the fallback
The class-level name was misleading in the way you describe: the body never
constructed a `PartitionPathRepartitionPartitioner`, it only called the static
helper.
--
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]
