wombatu-kun commented on code in PR #19236:
URL: https://github.com/apache/hudi/pull/19236#discussion_r3552385471
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/client/common/SparkReaderContextFactory.java:
##########
@@ -89,6 +90,10 @@ public SparkReaderContextFactory(HoodieSparkEngineContext
hoodieSparkEngineConte
// Broadcast: Configuration.
Configuration configs = getHadoopConfiguration(jsc.hadoopConfiguration());
schemaEvolutionConfigs.forEach(configs::set);
+ // Internal write-side reads (compaction, clustering, merge) must
materialize INLINE blob bytes
+ // so they can be rewritten into the new base file. DESCRIPTOR is a
query-only optimization; if
+ // it leaked onto this path the rewrite would persist null blob data and
silently drop the bytes.
+ configs.set(HoodieReaderConfig.BLOB_INLINE_READ_MODE.key(),
HoodieReaderConfig.BLOB_INLINE_READ_MODE_CONTENT);
Review Comment:
`TestSparkReaderContextFactory#testGetSchemaEvolutionConfigurations` already
captures this exact `Configuration` and asserts a handful of keys on it, so a
one-line `assertEquals(BLOB_INLINE_READ_MODE_CONTENT,
createdConfig.get(BLOB_INLINE_READ_MODE.key()))` would guard this pin directly.
Today only the heavyweight Lance functional tests cover it, and they would not
run if someone dropped this line while touching the factory for an unrelated
reason.
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/io/storage/HoodieSparkLanceWriter.java:
##########
@@ -446,12 +467,43 @@ protected void updateRecordMetadata(InternalRow row,
@AllArgsConstructor(staticName = "of")
private static class SparkArrowWriter implements ArrowWriter<InternalRow> {
private final LanceArrowWriter lanceArrowWriter;
+ // Parallel arrays of top-level BLOB column ordinals and names for the
per-row guard.
+ private final int[] blobFieldOrdinals;
+ private final String[] blobFieldNames;
@Override
public void write(InternalRow row) {
+ validateBlobRow(row);
lanceArrowWriter.write(row);
}
+ /**
+ * Reject descriptor-shaped BLOB rows before they are persisted. A row is
descriptor-shaped when
+ * a top-level BLOB struct is non-null, its type is INLINE, its data is
null, and its reference
+ * is non-null; this only arises when a DESCRIPTOR-mode read leaks onto a
write path, and writing
+ * it would silently drop the inline bytes. The legitimate empty-inline
shape {INLINE, null, null}
+ * stays writable.
+ */
+ private void validateBlobRow(InternalRow row) {
+ for (int i = 0; i < blobFieldOrdinals.length; i++) {
+ int ordinal = blobFieldOrdinals[i];
+ if (row.isNullAt(ordinal)) {
+ continue;
+ }
+ InternalRow blob = row.getStruct(ordinal, 3);
Review Comment:
`BlobDescriptorTransform`, in this package, already names these:
`TYPE_IDX`/`DATA_IDX`/`REF_IDX` for the child ordinals, and `INLINE_UTF8` for
the precomputed token that `INLINE_TYPE_TOKEN` duplicates. The struct arity is
available as `HoodieSchema.Blob.getFieldCount()`, which `BatchedBlobReader`
already uses for exactly this.
Suggest reusing those (or hoisting them into a small shared blob-layout
holder) rather than re-deriving `0`/`1`/`2`/`3` and a second INLINE token, so
the two classes that decode this struct cannot drift apart.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/execution/datasources/lance/SparkLanceReaderBase.scala:
##########
@@ -131,10 +131,10 @@ class SparkLanceReaderBase(enableVectorizedReader:
Boolean) extends SparkColumna
null
}
- // Honor `hoodie.read.blob.inline.mode`. CONTENT (default)
materializes INLINE bytes in
- // the `data` column; DESCRIPTOR surfaces per-row {position, size}
which the descriptor
- // iterator rewrites into Hudi OUT_OF_LINE references. Non-blob Lance
columns ignore
- // the option regardless.
+ // Honor `hoodie.read.blob.inline.mode`. DESCRIPTOR (default) surfaces
per-row
+ // {position, size} which the descriptor iterator rewrites into Hudi
OUT_OF_LINE
Review Comment:
`BlobDescriptorTransform` keeps `type = INLINE` on the rewritten row and
only synthesizes the `reference` sub-struct pointing at the current `.lance`
file; it never emits `OUT_OF_LINE`. Wording it as "rewrites into Hudi
OUT_OF_LINE references" is the mental model that made #19232 hard to spot,
since the row that actually reaches the writer is `{INLINE, data=null,
reference}`.
Suggest "...which the descriptor iterator turns into a synthesized
`reference` while leaving `type = INLINE`".
--
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]
