danny0405 commented on code in PR #19727:
URL: https://github.com/apache/hudi/pull/19727#discussion_r3861784555
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/execution/bulkinsert/BucketIndexBulkInsertPartitionerWithRows.java:
##########
@@ -36,20 +36,32 @@ public class BucketIndexBulkInsertPartitionerWithRows
implements BulkInsertParti
private final String indexKeyFields;
private final NumBucketsFunction numBucketsFunction;
private final HoodieWriteConfig writeConfig;
+ private final boolean sortByRecordKey;
private FileSystemViewStorageConfig viewConfig;
- public BucketIndexBulkInsertPartitionerWithRows(String indexKeyFields,
HoodieWriteConfig writeConfig) {
- this(writeConfig, NumBucketsFunction.fromWriteConfig(writeConfig),
indexKeyFields);
+ public BucketIndexBulkInsertPartitionerWithRows(String indexKeyFields,
+ HoodieWriteConfig
writeConfig,
+ boolean sortByRecordKey) {
Review Comment:
[P1] Please retain the two existing public constructor overloads. This
change replaces `(String, HoodieWriteConfig)` and `(HoodieWriteConfig, String,
String, int)` with new signatures, so downstream code compiled against the
current `hudi-spark-client` will fail with `NoSuchMethodError` (and source
users will no longer compile), even for non-LSM tables. Delegating overloads
can preserve compatibility while the new boolean-aware constructors serve the
new call sites.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestLSMDataSource.scala:
##########
@@ -175,6 +176,57 @@ class TestLSMDataSource extends
SparkClientFunctionalTestHarness {
"Ａ-row-p2" -> "v1"))
}
+ @ParameterizedTest
+ @MethodSource(Array("bucketBulkInsertParams"))
+ def testBucketIndexBulkInsert(
+ tableType: HoodieTableType,
+ bucketEngineType: HoodieIndex.BucketIndexEngineType,
+ enableRowWriter: Boolean): Unit = {
+ val tablePath =
s"${basePath}_${tableType.name.toLowerCase}_${bucketEngineType.name.toLowerCase}_$enableRowWriter"
+ val options = baseOptions(tableType) ++ Map(
+ DataSourceWriteOptions.ENABLE_ROW_WRITER.key -> enableRowWriter.toString,
+ HoodieWriteConfig.BULK_INSERT_SORT_MODE.key ->
BulkInsertSortMode.NONE.name,
+ HoodieIndexConfig.INDEX_TYPE.key -> HoodieIndex.IndexType.BUCKET.name,
+ HoodieIndexConfig.BUCKET_INDEX_ENGINE_TYPE.key -> bucketEngineType.name,
+ HoodieIndexConfig.BUCKET_INDEX_HASH_FIELD.key -> "id",
+ HoodieIndexConfig.BUCKET_INDEX_NUM_BUCKETS.key -> "1")
Review Comment:
[P2] Could this coverage use at least two buckets and assert the expected
record-to-bucket/file-id mapping? With `NUM_BUCKETS = 1`, every key is forced
through the same bucket, so the test only verifies partition-path routing; an
implementation that sorts correctly but changes/ignores bucket routing would
still pass. The PR’s central invariant is preserving bucket/file-group routing
while adding the secondary record-key ordering, so exercising multiple buckets
would protect that behavior for both RDD and Row parameters.
--
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]
