JingsongLi commented on code in PR #10226:
URL: https://github.com/apache/paimon/pull/10226#discussion_r4245255194
##########
paimon-common/src/main/java/org/apache/paimon/data/variant/BaseVariantReader.java:
##########
@@ -124,8 +126,9 @@ protected Object readFromTyped(InternalRow row, ByteBuffer
topLevelMetadata) {
/** A util function to rebuild the variant in binary format from a variant
value. */
protected Variant rebuildVariant(InternalRow row, ByteBuffer
topLevelMetadata) {
GenericVariantBuilder builder = new GenericVariantBuilder(false);
+ cachedMetadata.adopt(topLevelMetadata);
Review Comment:
[P2] Share the adopted dictionary across projected array elements
The recursive ShreddingUtils fix does not cover projected reads through
ArrayReader: its element loop repeatedly invokes the same child VariantReader,
and every child rebuild reaches this full-buffer comparison. For a shredded
object with `items: ARRAY<ROW<typed: INT>>` and one distinct `leftover_i` field
per element, projecting `$.items` as ARRAY<VARIANT> performs N scans of an
O(N)-sized metadata dictionary. I benchmarked the actual `buildFieldsToExtract`
+ `assembleVariantStruct` production path against baseline source classes on
JDK 8/G1. In the second independent JVM fork, median baseline/head times were
534/835 us at 1,000 elements, 2,490/13,309 us at 5,000, and 3,669/47,043 us at
10,000; the first fork reproduced approximately 1.45x/5.48x/14.58x slowdowns.
Canonical per-element output, SHA-256, and retained results after a
different-metadata read all matched. Please adopt once per outer row/read and
share that adopted dictionary with the child readers. A buffer-identity-onl
y shortcut would lose the backing-array reuse protection, so include
projected-array coverage as well as full-value reconstruction.
##########
paimon-common/src/main/java/org/apache/paimon/data/variant/VariantMetadata.java:
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.paimon.data.variant;
+
+import java.nio.ByteBuffer;
+import java.util.Arrays;
+
+import static org.apache.paimon.utils.BinaryRowDataUtil.byteBufferEquals;
+
+/**
+ * The string dictionary of a variant metadata buffer, with lazily decoded
entries.
+ *
+ * <p>Rows of a batch usually share the same metadata bytes, so an instance
can be shared across
+ * rows: {@link #adopt()} reparses the dictionary only when the bytes change.
A shared instance is a
+ * mutable cache slot, not a stable dictionary; do not retain it across calls.
+ *
+ * <p>This class is not thread safe.
+ */
+public class VariantMetadata {
+ private static final int MAX_DICTIONARY_SIZE = 1 << 24;
+
+ private ByteBuffer reuse;
+ private ByteBuffer current;
+ private int dictSize;
+ private String[] dict;
+ private int offsetSize;
+ private int stringStart;
+
+ /** Creates an empty metadata that adopts the first buffer passed to
{@link #adopt}. */
+ public static VariantMetadata empty() {
+ return new VariantMetadata();
+ }
+
+ private VariantMetadata() {
+ this.reuse = null;
+ this.current = null;
+ this.dictSize = 0;
+ this.dict = null;
+ this.offsetSize = 0;
+ this.stringStart = 0;
+ }
+
+ public void setCurrent(ByteBuffer metadata) {
+ this.current = metadata;
+ }
+
+ public void adopt() {
+ if (current == null) {
+ throw GenericVariantUtil.malformedVariant();
+ }
+
+ if (reuse != null
+ && reuse.remaining() == current.remaining()
+ && byteBufferEquals(reuse, current)) {
+ return;
+ }
+
+ reuse = snapshot(current);
Review Comment:
[P2] Publish cache state only after metadata validation succeeds
This updates `reuse` and then `dictSize` before validating the offset table.
If validation throws, an identical subsequent buffer hits the equality return
while decoded entries from the previous valid row remain in `dict`. Through the
public BaseVariantReader API, warming on `{"a":1}` and then reading a `{"b":2}`
value with truncated metadata `[1,1]` first throws MALFORMED_VARIANT, but the
identical second read silently returns `{"a":2}`. A fresh reader and the
baseline reject both reads; a control that validates candidate state before
publishing it also rejects both. This requires a caller that catches
malformed-row errors and reuses the reader/cache; the built-in Parquet batch
path aborts on the first error. Please commit the new
snapshot/header/dictionary state only after validation, or invalidate all
cached state on failure, and verify repeated bad-row reads after a valid
dictionary warmup.
--
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]
