wombatu-kun commented on code in PR #19236:
URL: https://github.com/apache/hudi/pull/19236#discussion_r3560189938
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestLanceDataSource.scala:
##########
@@ -1422,11 +1441,30 @@ class TestLanceDataSource extends
HoodieSparkClientTestBase {
s"DESCRIPTOR default should populate reference on plain read (id=$id)")
}
- // read_blob() under CONTENT mode is what we use to verify the
post-compaction bytes
- // because read_blob() on INLINE rows throws under the DESCRIPTOR default.
The bytes can
- // only come back if HoodieSparkLanceReader's CONTENT pin held during the
compactor's
- // base-file read — otherwise untouched ids 3..5 would have been rewritten
with null
- // `data` and CONTENT-mode read would surface that.
+ // Plain projection under CONTENT first: on a regression this fails with
an explicit
Review Comment:
This describes a failure mode the PR itself makes unreachable. A
DESCRIPTOR-leaked row is `{INLINE, data=null, reference!=null}`, and every
Lance write goes through `HoodieSparkLanceWriter`, whose new `validateBlobRow`
rejects exactly that shape. If the CONTENT pin regresses, the inline compaction
aborts inside the rewrite with a `HoodieException` and `writeDataframe` fails
before this CONTENT projection ever runs.
The projection still earns its place, but against a different shape:
`{INLINE, null, null}`, which the guard deliberately allows. Suggest rewording
to that, and pointing at `TestSparkReaderContextFactory` as what actually pins
the config. The same comment is duplicated on
`testBlobInlineClusteringRoundTrip`.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestLanceDataSource.scala:
##########
@@ -1443,6 +1481,150 @@ class TestLanceDataSource extends
HoodieSparkClientTestBase {
}
}
+ /**
+ * Clustering must preserve INLINE blob bytes under the DESCRIPTOR default.
Clustering rewrites
+ * ALL rows through
MultipleSparkJobExecutionStrategy.readRecordsForGroupAsRow, which reads base
+ * files through the same internal write-side reader context as compaction
+ * (SparkReaderContextFactory -> SparkFileFormatInternalRowReaderContext ->
SparkLanceReaderBase).
+ * Before SparkReaderContextFactory pinned {@code
hoodie.read.blob.inline.mode=CONTENT} for
+ * internal reads, the first clustering of a Lance table with INLINE blobs
read null {@code data}
+ * (the DESCRIPTOR default) and rewrote every row's blob with null bytes,
silently losing all
+ * blob content (#19232). This test bulk-inserts INLINE blobs, triggers
inline clustering,
+ * asserts a replacecommit actually completed, and verifies every row's
bytes survived.
+ */
+ @Test
+ def testBlobInlineClusteringRoundTrip(): Unit = {
Review Comment:
`SparkReaderContextFactory` and `HoodieSparkLanceReader` both name "merge"
as a path the CONTENT pin protects, but no blob test exercises it. Every blob
test writes via `bulk_insert` or `insert`; the only `upsert` is the MOR one in
`testBlobInlineCompactionRoundTrip`, which reaches the reader through
`HoodieCompactor`. A CoW upsert that rewrites an existing base file through
`FileGroupReaderBasedMergeHandle` is never covered.
That path is not just a third caller of the same code: it resolves the
factory via `getReaderContextFactoryForWrite`, which branches to
`AvroReaderContextFactory` for AVRO record types, unlike the
`getReaderContextFactory` used by compaction and clustering. A `COPY_ON_WRITE`
round-trip that bulk-inserts INLINE blobs and then upserts a subset would close
it.
--
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]
