wombatu-kun commented on code in PR #18961:
URL: https://github.com/apache/hudi/pull/18961#discussion_r3826901772
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/io/storage/row/HoodieRowParquetWriteSupport.java:
##########
@@ -902,16 +908,25 @@ private Type convertField(HoodieSchema fieldSchema,
StructField structField, Typ
.named(MAP_REPEATED_NAME))
.named(structField.name());
} else if (dataType instanceof StructType) {
+ StructType nestedStruct = (StructType) dataType;
Types.GroupBuilder<GroupType> groupBuilder =
Types.buildGroup(repetition);
- Arrays.stream(((StructType) dataType).fields()).forEach(field -> {
+ Arrays.stream(nestedStruct.fields()).forEach(field -> {
// Note: Cannot use HoodieSchemaField::schema method reference due to
Java 17 compilation ambiguity
HoodieSchema nestedFieldSchema = Option.ofNullable(resolvedSchema)
.flatMap(s -> s.getField(field.name()))
.map(f -> f.schema())
.orElse(null);
groupBuilder.addField(convertField(nestedFieldSchema, field));
});
- return groupBuilder.named(structField.name());
+ // A shredded variant column reaches here as a marked struct (the
VariantType was replaced by
+ // its {metadata, value, typed_value} shredding schema). Tag the parquet
group with the VARIANT
+ // logical type so external readers recognize it as a Variant, matching
Spark's native shredded
+ // parquet schema. No-op on Spark 4.0/3.x (the annotation only exists in
parquet 1.16+).
+ Types.GroupBuilder<GroupType> taggedBuilder =
Review Comment:
This tags shredded variant groups on the row path, which master's struct
branch did not, so a table whose write schema already declares `typed_value`
gets a changed footer with the new config off. The Impact section attributes
the annotation change to the AVRO path only and says it matches what the
row-writer path already produces - worth covering this leg there too?
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/io/storage/HoodieSparkFileWriterFactory.java:
##########
@@ -56,6 +62,43 @@ public HoodieSparkFileWriterFactory(HoodieStorage storage) {
protected HoodieFileWriter newParquetFileWriter(
String instantTime, StoragePath path, HoodieConfig config, HoodieSchema
schema,
TaskContextSupplier taskContextSupplier) throws IOException {
+ // The row write support resolves its HoodieSchema from the config
(hoodie.write.schema /
+ // hoodie.avro.schema), not the schema argument, so inferable columns are
detected on that
+ // config schema (the one the splice below targets) intersected with the
schema argument's
+ // (the shape of the rows being written, which the samples come from). The
argument is
+ // checked first because it is already parsed: native log, delete and CDC
writers share this
+ // factory with schemas that have no top-level variant, and they must not
pay a config-schema
Review Comment:
`HoodieNativeLogFormatWriter.ensureDataFileWriter` hands this factory the
full record schema, so the native-log data writer is the one case here that
does carry a top-level variant - which is what makes the config's "native
parquet log files ... each infers its own schema" true. Dropping "native log"
would leave the delete and CDC writers, which do fit.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/dml/schema/TestVariantDataType.scala:
##########
@@ -1192,6 +1377,38 @@ class TestVariantDataType extends HoodieSparkSqlTestBase
{
}
}
+ /**
+ * Pins an INFERRED shredding layout of `column` across every data parquet
file of the table:
+ * see [[assertInferredTypedValueIn]].
+ */
+ private def assertInferredTypedValue(tablePath: String, column: String, leg:
String,
+ present: Seq[String], absent:
Seq[String] = Seq.empty): Unit = {
+ assertInferredTypedValueIn(listDataParquetFiles(tablePath), column, leg,
present, absent)
+ }
+
+ /**
+ * Pins an INFERRED shredding layout of `column` in the given parquet files:
the variant group is
+ * shredded, carries the VARIANT logical type (the inferrer only exists on
Spark 4.1+, whose
+ * parquet ships the annotation) and its typed_value has every `present`
member and none of
+ * the `absent` ones (e.g. an avro-illegal key the inferrer dropped).
+ */
+ private def assertInferredTypedValueIn(files: Seq[String], column: String,
leg: String,
+ present: Seq[String], absent:
Seq[String] = Seq.empty): Unit = {
+ assert(files.nonEmpty, s"[$leg] should have at least one data parquet
file")
+ files.foreach { filePath =>
+ val variantGroup = getFieldAsGroup(readParquetSchema(filePath), column)
+ assert(variantGroup.containsField("typed_value"),
+ s"[$leg] $column should be shredded with an inferred typed_value.
File: $filePath Schema:\n$variantGroup")
+
assert(Option(variantGroup.getLogicalTypeAnnotation).exists(_.toString.contains("VARIANT")),
Review Comment:
These assertions read only the footer schema, so a file where every row fell
back to the residual `value` with `typed_value` written all-null passes every
inference test here. Could this also assert a non-null count on a `typed_value`
leaf from the block column statistics?
##########
hudi-common/src/main/java/org/apache/hudi/core/io/storage/VariantShreddingInferenceFileWriter.java:
##########
@@ -0,0 +1,328 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.core.io.storage;
+
+import org.apache.hudi.common.avro.VariantShreddingSchemaInferrer;
+import
org.apache.hudi.common.avro.VariantShreddingSchemaInferrer.VariantSample;
+import org.apache.hudi.common.model.HoodieKey;
+import org.apache.hudi.common.model.HoodieRecord;
+import org.apache.hudi.common.schema.HoodieSchema;
+import org.apache.hudi.common.util.CloseableUtils;
+import org.apache.hudi.common.util.DefaultSizeEstimator;
+import org.apache.hudi.common.util.SizeEstimator;
+import org.apache.hudi.exception.HoodieIOException;
+
+import lombok.extern.slf4j.Slf4j;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Properties;
+
+/**
+ * A {@link HoodieFileWriter} decorator that infers a per-file variant
shredding schema from the
+ * first records before opening the real parquet writer.
+ *
+ * <p>Records are buffered (and their variant binaries sampled) until a
threshold is reached or
+ * the writer closes, the sampled binaries are fed to a {@link
VariantShreddingSchemaInferrer},
+ * the real writer is created against the schema with the inferred typed_value
spliced in, and
+ * the buffer is replayed in arrival order. Replay reproduces each call
exactly (write vs
+ * writeWithMetadata), so commit seqnos, bloom filters and min/max record keys
come out
+ * identical to a non-buffered write. Buffering thresholds mirror Spark's
+ * {@code ParquetOutputWriterWithVariantShredding} (4096 rows / 64MB).
+ *
+ * <p>Buffered records are {@link HoodieRecord#copy() copied} because Spark
iterators reuse row
+ * instances, then handed to {@link VariantSampleExtractor#prepare}, which
serves two purposes:
+ * an extractor that has to materialize the record to sample it (the Avro one
deserializes
+ * payload-backed records) returns the materialized form for buffering so the
replay does not
+ * repeat that work, and (since copy() returns the caller's own wrapper for
every record type
+ * today) both shipped extractors return a wrapper the caller does not hold,
so a handle that
+ * deflates its record right after the write call cannot blank a buffered one.
Records with
+ * nothing to materialize (delete payloads) are buffered as they are, so
replay still relies on
+ * writer-level records being freshly allocated per record, which holds today;
variant samples
+ * are extracted eagerly into immutable byte arrays so inference itself never
depends on it.
+ *
+ * <p>Inference failures never fail the write: the file falls back to
unshredded variants. This
+ * deliberately diverges from Spark (which propagates inference failures)
because a throwing
+ * inference would fail compaction. Writer-creation or replay failures,
however, are latched and
+ * rethrown from every subsequent call including {@link #close()}, so a task
cannot silently
+ * drop buffered records that the handle already counted as written.
+ *
+ * <p>{@link #writeRow} carries neither a {@link HoodieRecord} nor a schema to
sample from, so the
+ * first such call materializes the real writer with whatever has been sampled
so far (unshredded
+ * when nothing has) and passes the row straight through. Footer metadata
added before
+ * materialization is queued and handed to the real writer once it exists;
parquet only consumes
+ * it at close, so nothing is lost.
+ *
+ * <p>Single-threaded by contract, same as the writers it wraps.
+ *
+ * <p>See https://github.com/apache/hudi/issues/18937.</p>
+ *
+ * @param <T> the engine-native record type of the wrapped writer
+ */
+@Slf4j
+public class VariantShreddingInferenceFileWriter<T> implements
HoodieFileWriter<T> {
+
+ /** Buffer caps mirroring Spark's ParquetOutputWriterWithVariantShredding. */
+ public static final int MAX_BUFFERED_RECORDS = 4096;
+ public static final long MAX_BUFFERED_BYTES = 64L * 1024 * 1024;
+ private static final int SIZE_ESTIMATE_INTERVAL = 100;
+
+ /**
+ * Extracts the variant binaries of the inferable columns from a record.
Bound to the writer
+ * schema and column set by the creating factory; must defensively copy the
bytes.
+ */
+ @FunctionalInterface
+ public interface VariantSampleExtractor {
+ VariantSample[] extract(HoodieRecord record, HoodieSchema schema,
Properties props) throws IOException;
+
+ /**
+ * Returns the record to buffer for replay; {@link #extract} is then
called with that record.
+ * An extractor that must materialize the record to sample it returns the
materialized form,
+ * so the replay does not redo the work. Defaults to the record itself.
+ */
+ default HoodieRecord prepare(HoodieRecord record, HoodieSchema schema,
Properties props) throws IOException {
+ return record;
+ }
+
+ /**
+ * Bytes of state that every buffered record references but that is shared
across them, so a
+ * deep object-size walk of one record counts it in full: the Avro {@code
Schema} graph of an
+ * Avro record, the {@code StructType} of a Spark row. Subtracted from
each record's size
+ * estimate so the byte cap budgets record payload rather than the schema
times the record
+ * count (the HUDI-9499 class of over-estimate, which would shrink the
inference sample to a
+ * fraction of the intended 4096 rows). Defaults to 0.
+ */
+ default long sharedSizeEstimate(HoodieSchema schema) {
+ return 0;
+ }
+ }
+
+ /** Creates the real file writer once the inferred typed_value schemas are
known. */
+ @FunctionalInterface
+ public interface InferredWriterFactory<T> {
+ HoodieFileWriter<T> create(Map<String, HoodieSchema> inferredTypedValues)
throws IOException;
+ }
+
+ private final List<String> variantColumns;
+ private final VariantSampleExtractor extractor;
+ private final VariantShreddingSchemaInferrer inferrer;
+ private final InferredWriterFactory<T> writerFactory;
+ private final long maxBufferedBytes;
+ private final SizeEstimator<HoodieRecord> sizeEstimator = new
DefaultSizeEstimator<>();
+
+ private final List<BufferedWrite> buffer = new ArrayList<>();
+ private final List<VariantSample[]> samples = new ArrayList<>();
+ private final Map<String, String> pendingFooterMetadata = new
LinkedHashMap<>();
+ private long estimatedRecordSize = 0;
+ private long bufferedBytes = 0;
+ private HoodieFileWriter<T> delegate;
+ private IOException fatalFailure;
+ private boolean closed = false;
+
+ public VariantShreddingInferenceFileWriter(List<String> variantColumns,
+ VariantSampleExtractor extractor,
+ VariantShreddingSchemaInferrer
inferrer,
+ InferredWriterFactory<T>
writerFactory,
+ long maxFileSize) {
+ this.variantColumns = variantColumns;
+ this.extractor = extractor;
+ this.inferrer = inferrer;
+ this.writerFactory = writerFactory;
+ this.maxBufferedBytes = Math.min(MAX_BUFFERED_BYTES, Math.max(1,
maxFileSize));
+ }
+
+ @Override
+ public boolean canWrite() {
+ // Nothing has been physically written while buffering, so size-based
rollover cannot apply yet.
+ return delegate == null || delegate.canWrite();
+ }
+
+ @Override
+ public void writeWithMetadata(HoodieKey key, HoodieRecord record,
HoodieSchema schema, Properties props) throws IOException {
+ rethrowIfFailed();
+ if (delegate != null) {
+ delegate.writeWithMetadata(key, record, schema, props);
+ } else {
+ buffer(true, key, null, record, schema, props);
+ }
+ }
+
+ @Override
+ public void write(String recordKey, HoodieRecord record, HoodieSchema
schema, Properties props) throws IOException {
+ rethrowIfFailed();
+ if (delegate != null) {
+ delegate.write(recordKey, record, schema, props);
+ } else {
+ buffer(false, null, recordKey, record, schema, props);
+ }
+ }
+
+ @Override
+ public void writeRow(String recordKey, T record) throws IOException {
+ rethrowIfFailed();
+ // No HoodieRecord or schema to sample from: materialize with what has
been sampled so far and
+ // pass the row through. Today only the native log-format delete writer
takes this path, and its
+ // delete schema has no variant column to infer for.
+ materialize();
+ delegate.writeRow(recordKey, record);
+ }
+
+ @Override
+ public void addFooterMetadata(Map<String, String> footerMetadata) {
+ if (delegate != null) {
+ delegate.addFooterMetadata(footerMetadata);
+ } else {
+ // Footer metadata is only consumed at close, so it can wait for the
real writer.
+ pendingFooterMetadata.putAll(footerMetadata);
+ }
+ }
+
+ @Override
+ public void close() throws IOException {
+ if (closed) {
+ return;
+ }
+ closed = true;
+ boolean delegateClosed = false;
+ try {
+ rethrowIfFailed();
+ // Materialize even with an empty buffer: handles expect the file to
exist at close.
+ materialize();
+ // Mark before close() so a throwing delegate.close() surfaces, not
retried in the catch.
+ delegateClosed = true;
+ delegate.close();
+ } catch (IOException | RuntimeException e) {
Review Comment:
`materialize()` latches `Error` and rethrows it, but this catch lists only
`IOException | RuntimeException`, so when the first materialization happens
inside `close()` the delegate created just above is never closed and `closed`
is already set. Widening both decorators' catch to `IOException |
RuntimeException | Error` would match `materialize()`.
##########
hudi-common/src/main/java/org/apache/hudi/common/avro/VariantSchemaUtils.java:
##########
@@ -123,6 +143,235 @@ private static HoodieSchema
stripVariantShreddingAt(HoodieSchema schema) {
return wasNullable ? HoodieSchema.createNullable(replacement) :
replacement;
}
+ /**
+ * Strips {@code typed_value} from top-level fields that have the variant
SHAPE but lost the
+ * variant logical type, i.e. plain records of {@code {metadata: bytes,
value: [nullable]
+ * bytes, typed_value}} (see {@link #isShreddedVariantShape}).
Parquet-footer-derived schemas
+ * come back this way (the converter does not attach the variant logical
type), so
+ * {@link #stripVariantShredding} alone cannot see them. Used by the
table-schema footer
+ * fallback only; returns {@code schema} as-is when nothing matches.
+ *
+ * <p>Unlike {@link #isShreddedVariantTarget}, the match here has NO
requested-side anchor:
+ * the footer fallback runs precisely when no table schema is available to
anchor on, so a
+ * plain user struct that happens to have exactly this shape is stripped too
(a documented,
+ * accepted false positive: {@code metadata} plus {@code typed_value} is the
variant spec's
+ * vocabulary). Top-level fields only, matching the scope of
+ * {@link #getInferableVariantColumns}: inference never shreds a nested
variant, and
+ * {@code HoodieAvroWriteSupport.applyForcedShreddingSchema} walks top-level
fields only. The row
+ * writer can force-shred at depth (see {@link #swapShreddedVariantFields}),
a test-only
Review Comment:
`HoodieRowParquetWriteSupport.processNestedDataType` shreds a nested variant
from the write schema alone, with no forced-DDL config involved, so calling the
at-depth row-path case test-only understates it. With that, the
`TableSchemaResolver` comment's "must never surface in the resolved table
schema" does not hold for a nested shredded column.
##########
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieStorageConfig.java:
##########
@@ -298,6 +298,23 @@ public class HoodieStorageConfig extends HoodieConfig {
+ "The provider parses variant binary data and populates typed_value
columns. "
+ "When not set, the provider is auto-detected from the classpath.");
+ public static final ConfigProperty<Boolean>
PARQUET_VARIANT_SHREDDING_SCHEMA_INFERENCE_ENABLED = ConfigProperty
+ .key("hoodie.parquet.variant.shredding.schema.inference.enabled")
+ .defaultValue(false)
+ .sinceVersion("1.3.0")
+ .withDocumentation("When enabled, the shredding schema for variant
columns without an explicit "
+ + "typed_value in the write schema is inferred automatically per
parquet file from a sample of "
+ + "the records written to that file, mirroring Spark 4.1's "
+ + "spark.sql.variant.inferShreddingSchema. Requires Spark 4.1+ on
the writer classpath; "
+ + "writes stay unshredded otherwise (Spark 4.0, Flink, Java
engines). Applies to every "
+ + "parquet file the writer produces: base files and, on table
version 10+, the native "
+ + "parquet log files of MOR tables (each infers its own schema);
legacy Avro log blocks "
+ + "stay unshredded and shred at compaction. Up to 4096 records or
64MB are buffered per "
+ + "open file writer before the writer is created, on top of
parquet's own row-group "
+ + "buffer, so size executor memory for concurrently open handles
accordingly. Ignored when "
+ + "hoodie.parquet.variant.force.shredding.schema.for.test is set or
when write shredding "
Review Comment:
`isShreddingInferenceEnabled` also requires an empty
`hoodie.internal.schema`, so a table using schema-on-read turns inference off
with no signal, while this text lists only the force-DDL and write-shredding
gates. Adding that third condition would keep the generated config reference
complete.
--
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]
