Re: [I] Add bootstrap read testing to TestHoodieFileGroupReaderBase [hudi]

["rangareddy (via GitHub)" <gi...@apache.org>]

rangareddy commented on issue #17268:
URL: https://github.com/apache/hudi/issues/17268#issuecomment-5508894667

   Followed up on this against `master` at `a2788eef922e`. The TODO in the description is **not** stale, and I can now say exactly why it is still unreachable and what is needed to reach it.
   
   ### The TODO path is confirmed dead, by measurement
   
   I put a print statement inside the bootstrap branch of `shouldValidatePartialRead` and ran the existing bootstrap test:
   
   ```
   mvn test -pl hudi-spark-datasource/hudi-spark \
     -Dtest='TestHoodieFileGroupReaderOnSpark#testReadFileGroupInBootstrapMergeOnReadTable'
   
   Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
   branch-reached hits: 0
   ```
   
   The test passes and the branch is entered zero times, so the comment is accurate rather than left-over.
   
   ### Why the bootstrap test does not reach it
   
   https://github.com/apache/hudi/blob/a2788eef922e2375432ead3fe50af7fe5f55faf7/hudi-common/src/test/java/org/apache/hudi/common/table/read/TestHoodieFileGroupReaderBase.java#L1122-L1133
   
   The log-file check comes first and returns `true`. A **Merge-on-Read** file slice always has log files, so for `testReadFileGroupInBootstrapMergeOnReadTable` (`:771`, driven by the `file-group-reader/bootstrap_data.zip` fixture) the method short-circuits on the first `if` and the bootstrap branch below it is never evaluated. The bootstrap read test that landed under HUDI-8632 is therefore necessary but not sufficient for the second half of this ticket.
   
   ### What would actually exercise it
   
   A **bootstrap Copy-on-Write** read test: a file slice with a bootstrap base file present and no log files, which is the only shape that falls through the first check into the bootstrap branch. That likely means a COW bootstrap fixture alongside the existing MOR `bootstrap_data.zip`, since the current one is MOR.
   
   So the remaining scope on this ticket is one specific missing test rather than a general "add bootstrap coverage", which should make it considerably cheaper to finish than it looks. Worth noting for whoever picks it up that the assertion guarded by that branch (`assertThrows(IllegalArgumentException.class, ...)` at `:1025`) has consequently never run for a bootstrap slice, so it is unverified rather than known-good.
   
   Read and measured on Spark 3.5 / Scala 2.12; the instrumentation was reverted afterwards.
   


-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org