fix(bulk-insert): add write config constructors to all sort mode partitioners - #19509
Conversation
…itioners A partitioner named through hoodie.bulkinsert.user.defined.partitioner.class is instantiated by reflection with only the write config, so it needs a public constructor taking a single HoodieWriteConfig. Eight of the twelve partitioners that a BulkInsertSortMode maps to did not have one. NonSortPartitioner, NonSortPartitionerWithRows, JavaNonSortPartitioner and JavaGlobalSortPartitioner take nothing configurable, so the new constructor behaves as the default one does. The two java client partitioners relied on the implicit no-arg constructor, which declaring another one removes, so that is now declared explicitly to keep JavaBulkInsertInternalPartitionerFactory working. The partition path repartition partitioners take the table partitioned flag from the table when built by the factory. The new constructor derives it from the configured partition path field, which is what governs whether records end up with a non-empty partition path. Partitioners not reachable from a sort mode are left alone: the spatial curve partitioners are clustering layout partitioners, and JavaCustomColumnsSortPartitioner is not selected by any sort mode.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR adds (HoodieWriteConfig) constructors to the bulk-insert sort-mode partitioners so each can be named through hoodie.bulkinsert.user.defined.partitioner.class, plus a shared BulkInsertPartitioner.isTablePartitioned(config) helper for the partition-path variants. The additive constructors delegate to existing ones and preserve the factory's no-arg instantiation path, and I traced the isTablePartitioned derivation against the factory's table.isPartitioned() source and it's a sound, well-documented fallback for the config-only reflection path. 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. A few naming and simplification suggestions below — mostly the copy-pasted constructor Javadoc across six files; everything else looks clean.
cc @yihua
| this(false); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
🤖 nit: the same Javadoc block ("Constructor taking the write config, so this partitioner can also be named through…") is copy-pasted verbatim across six or so constructors. Could you shorten these to a one-liner like // Required for reflection-based instantiation via BULKINSERT_USER_DEFINED_PARTITIONER_CLASS_NAME and leave the full rationale in the BulkInsertPartitioner.isTablePartitioned Javadoc (or a single shared location), so there's only one place to update if the explanation ever changes?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19509 +/- ##
============================================
- Coverage 77.03% 76.99% -0.05%
- Complexity 33863 33882 +19
============================================
Files 2575 2575
Lines 143379 143417 +38
Branches 17574 17579 +5
============================================
- Hits 110451 110419 -32
- Misses 24666 24735 +69
- Partials 8262 8263 +1
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 #16423 / HUDI-7526.
A partitioner named through
hoodie.bulkinsert.user.defined.partitioner.classisinstantiated by reflection:
ReflectionUtils.loadClass(clazz, Object...)infers the constructor argument types fromthe instances it is handed, so it resolves
getConstructor(HoodieWriteConfig.class). Aclass is therefore usable as a user defined partitioner only if it exposes a public
constructor taking exactly one
HoodieWriteConfig.Twelve partitioners back an out of the box
BulkInsertSortMode. Eight of them did nothave such a constructor, and failed with
Unable to instantiate class ...:NONENonSortPartitioner(fixed)NonSortPartitionerWithRows(fixed)JavaNonSortPartitioner(fixed)GLOBAL_SORTGlobalSortPartitionerGlobalSortPartitionerWithRowsJavaGlobalSortPartitioner(fixed)PARTITION_SORTRDDPartitionSortPartitionerPartitionSortPartitionerWithRowsPARTITION_PATH_REPARTITIONPartitionPathRepartitionPartitioner(fixed)PartitionPathRepartitionPartitionerWithRows(fixed)PARTITION_PATH_REPARTITION_AND_SORTPartitionPathRepartitionAndSortPartitioner(fixed)PartitionPathRepartitionAndSortPartitionerWithRows(fixed)NonSortPartitioner, backingBulkInsertSortMode.NONE, is the example named in theticket.
Summary and Changelog
Every partitioner that an out of the box bulk insert sort mode maps to can now be named
through
hoodie.bulkinsert.user.defined.partitioner.class.(HoodieWriteConfig)constructor to the eight partitioners marked above.NonSortPartitioner,NonSortPartitionerWithRows,JavaNonSortPartitionerandJavaGlobalSortPartitionerhave nothing configurable, so the new constructor behavesas the default one does. The value used for
enforceNumOutputPartitionsisfalse,matching what
BulkInsertInternalPartitionerFactory.get(table, config)andBulkInsertInternalPartitionerWithRowsFactory.get(config, isTablePartitioned)pass onthe bulk insert write path.
any constructor removes it, and
JavaBulkInsertInternalPartitionerFactorycalls it, sothe no-arg constructor is now declared explicitly alongside the new one.
HoodieTablewhen built by the factory, which reflection cannot supply. The newconstructor derives it from the configured partition path field, added as
BulkInsertPartitioner.isTablePartitioned. Those partitioners branch on whetherrecords carry a non-empty partition path, and the write side partition path field is
what governs that.
is affected.
Left alone, because no sort mode maps to them:
RDDSpatialCurveSortPartitionerandRowSpatialCurveSortPartitionerare clusteringlayout partitioners, not sort mode partitioners. The RDD one also needs a
HoodieSparkEngineContextthat cannot be derived from a write config.JavaCustomColumnsSortPartitioneris not selected by any sort mode.Tests:
TestDataSourceUtilsgains a parameterized test over all thirteen constructible sparkpartitioners, asserting each loads through both
createUserDefinedBulkInsertPartitionerandcreateUserDefinedBulkInsertPartitionerWithRows. It covers the ones that alreadyworked, so the whole contract is pinned rather than only the classes being fixed. It
also gains a test pinning
isTablePartitionedfor a configured partition path field,an explicitly empty one, and an unset one.
TestJavaBulkInsertInternalPartitionerFactorygains a test that both java client sortmode partitioners load from a write config, and a test that both still expose a no-arg
constructor, which guards the implicit constructor removal described above.
Reverting only the new constructors makes those tests fail for exactly the classes
concerned and nothing else.
Impact
None for existing users. The change only adds constructors, so every current call site
resolves as before, and the factories are unaffected. What changes is that eight built in
partitioners can now be named as a user defined partitioner where that previously failed
at instantiation.
Risk Level
low
Additive only, with no signature changed or removed. The one piece of new logic,
isTablePartitioned, is used solely by the new constructors; the existing(boolean, HoodieWriteConfig)constructors still take the flag from the caller. Verifiedend to end by running a bulk insert with each fixed partitioner named as the user defined
partitioner, over both the RDD and row writer paths, and on both a partitioned and a non
partitioned table so both branches of the derived flag are exercised.
Documentation Update
None. No new config, and no change to an existing config's meaning or default.
Contributor's checklist