deepakpanda93 commented on code in PR #19471:
URL: https://github.com/apache/hudi/pull/19471#discussion_r3712054174
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestPartitionBucketIndexSupport.scala:
##########
@@ -173,32 +172,31 @@ class TestPartitionBucketIndexSupport extends
TestBucketIndexSupport {
def exprFilePathAnswerCheck(bucketIndexSupport: PartitionBucketIndexSupport,
exprRaw: String, expectResult: Set[String],
allFileStatus: Set[String]): Unit = {
- if (!HoodieSparkUtils.gteqSpark4_0) { // TODO (HUDI-9403)
- val resolveExpr = HoodieCatalystExpressionUtils.resolveExpr(spark,
exprRaw, structSchema)
- val optimizerPlan =
spark.sessionState.optimizer.execute(DummyExpressionHolder(Seq(resolveExpr)))
- val optimizerExpr =
optimizerPlan.asInstanceOf[DummyExpressionHolder].exprs.head
-
- // split input files into different partitions
- val partitionPath1 = DEFAULT_PARTITION_PATH(0)
- val allFileSlices1: Seq[FileSlice] = allFileStatus.slice(0,
3).map(fileName => {
- val slice = new FileSlice(partitionPath1, "00000000000000000",
FSUtils.getFileId(fileName))
- slice.setBaseFile(new HoodieBaseFile(new StoragePathInfo(new
StoragePath(fileName), 0L, false, 0, 0, 0)))
- slice
- }).toSeq
-
- val partitionPath2 = DEFAULT_PARTITION_PATH(1)
- val allFileSlices2: Seq[FileSlice] = allFileStatus.slice(3,
5).map(fileName => {
- val slice = new FileSlice(partitionPath1, "00000000000000000",
FSUtils.getFileId(fileName))
- slice.setBaseFile(new HoodieBaseFile(new StoragePathInfo(new
StoragePath(fileName), 0L, false, 0, 0, 0)))
- slice
- }).toSeq
-
- val input = Seq((Option.apply(new
BaseHoodieTableFileIndex.PartitionPath(partitionPath1, Array())),
allFileSlices1),
- (Option.apply(new
BaseHoodieTableFileIndex.PartitionPath(partitionPath2, Array())),
allFileSlices2))
- val candidate = bucketIndexSupport.computeCandidateFileNames(fileIndex,
splitConjunctivePredicates(optimizerExpr),
- Seq(), input, false)
-
- assert(candidate.get.equals(expectResult))
- }
+ val resolveExpr = HoodieCatalystExpressionUtils.resolveExpr(spark,
exprRaw, structSchema)
+ val dummyExpressionHolder = HoodieDummyExpressionHolder(Seq(resolveExpr),
resolveExpr.references.toSeq)
+ val optimizerPlan =
spark.sessionState.optimizer.execute(dummyExpressionHolder)
+ val optimizerExpr =
optimizerPlan.asInstanceOf[HoodieDummyExpressionHolder].exprs.head
+
+ // split input files into different partitions
+ val partitionPath1 = DEFAULT_PARTITION_PATH(0)
+ val allFileSlices1: Seq[FileSlice] = allFileStatus.slice(0,
3).map(fileName => {
+ val slice = new FileSlice(partitionPath1, "00000000000000000",
FSUtils.getFileId(fileName))
+ slice.setBaseFile(new HoodieBaseFile(new StoragePathInfo(new
StoragePath(fileName), 0L, false, 0, 0, 0)))
+ slice
+ }).toSeq
+
+ val partitionPath2 = DEFAULT_PARTITION_PATH(1)
+ val allFileSlices2: Seq[FileSlice] = allFileStatus.slice(3,
5).map(fileName => {
Review Comment:
This is the most useful part of the review — thank you. The
`computeNumBuckets` hardcoded to `return 19` framing makes the gap unarguable,
and your table is right: both `DEFAULT_PARTITION_PATH` entries match
`\d{4}\-(06\-(01|17|18)|11\-(01|10|11))` under `Matcher.find()`, so every
partition in the suite carried 19 buckets and the per-partition mapping was
never exercised.
Added `testCandidateFilesUsePerPartitionBucketCount`, built along the lines
you suggested:
- `2025-06-17` matches the expression, so 19 buckets; `2025-07-01` does not,
so it falls back to the table default of 10.
- Each partition gets explicitly named files rather than `slice`.
- The default partition also holds a **decoy** carrying the bucket id from
the *19* bucket hashing. Same bucket id, different file name, since
`newBucketFileIdPrefix` mints a fresh uuid per call.
- The test asserts a precondition that the two bucket ids for the record
actually differ, so the check cannot pass vacuously.
If `computeNumBuckets` returned the wrong count for the non-matching
partition, the decoy becomes a candidate and the expected file does not, so it
fails from both directions.
Verified it discriminates rather than assuming so: pointing the non-matching
partition at `2025-06-01`, so both partitions match the expression, fails
**exactly** this test and leaves the other four passing.
On the random split — confirmed and fixed. The file names are ordered before
slicing now, so the file to partition assignment is stable run to run.
On the fallback gap: deliberately not adding it here. Asserting
`bucketSet.isEmpty` fails today because of #19487, and asserting the current
`Some(Set())` would bake that bug into the suite. It belongs in the change that
fixes #19487, as its regression test. Happy to pick that up next if nobody has
started.
--
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]
