wgtmac commented on code in PR #3562:
URL: https://github.com/apache/parquet-java/pull/3562#discussion_r4090734355
##########
parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java:
##########
@@ -880,6 +910,161 @@ static HashMap<String, Integer> getMetadataMap(ByteBuffer
metadata) {
return result;
}
+ /**
+ * Bounds-checks a single Variant value node against its buffer slot.
Performs no recursion
+ * into nested children: child nodes are checked on demand when callers
descend into them.
+ *
+ * <p>Cost: O(1) for primitives and short strings, O(numElements) for
objects and arrays.
+ * Validation of nested structures is deferred so that opening a large
well-formed Variant
+ * is not penalized by sub-trees the caller never inspects.
+ *
+ * <p>Note that as validation is against the total bytebuffer capacity, a
variant can expand into
+ * the buffer space nominally allocated its siblings. This is not something
checked
+ * for; it is memory safe.
+ * @param valueBuffer the variant value buffer (position/limit define the
extent of this node's slot)
+ * @param dictSize the metadata dictionary size, used to bound object field
ids
+ * @throws IllegalArgumentException if the value header or container table
does not fit within
+ * the buffer slot, or if any object field id is out of range
+ */
+ static void validateValueShallow(final ByteBuffer valueBuffer, final int
dictSize) {
+ int pos = valueBuffer.position();
+ long slot = (long) valueBuffer.limit() - pos;
+ Preconditions.checkArgument(slot > 0, "variant value is empty");
+ int header = valueBuffer.get(pos) & 0xFF;
+ int basicType = header & BASIC_TYPE_MASK;
+ int typeInfo = (header >> BASIC_TYPE_BITS) & PRIMITIVE_TYPE_MASK;
+ switch (basicType) {
+ case SHORT_STR:
+ Preconditions.checkArgument(1L + typeInfo <= slot, "variant short
string extends past buffer");
+ return;
+ case OBJECT:
+ validateContainerShallow(valueBuffer, typeInfo, pos, slot, dictSize,
true);
+ return;
+ case ARRAY:
+ validateContainerShallow(valueBuffer, typeInfo, pos, slot, dictSize,
false);
+ return;
+ default:
+ validatePrimitiveShallow(valueBuffer, typeInfo, pos, slot);
+ }
+ }
+
+ /**
+ * Shallow validation of a container.
+ *
+ * @param valueBuffer buffer with the variant data
+ * @param typeInfo type information from the metadata
+ * @param pos buffer read position
+ * @param length length of slot for this container.
+ * @param dictSize dictionary size
+ * @param isObject is this an object?
+ */
+ private static void validateContainerShallow(
+ final ByteBuffer valueBuffer,
+ final int typeInfo,
+ final int pos,
+ final long length,
+ final int dictSize,
+ final boolean isObject) {
+ boolean largeSize;
+ int idSize;
+ if (isObject) {
+ largeSize = ((typeInfo >> 4) & 0x1) != 0;
+ idSize = ((typeInfo >> 2) & 0x3) + 1;
+ } else {
+ largeSize = ((typeInfo >> 2) & 0x1) != 0;
+ idSize = 0;
+ }
+ int offsetSize = (typeInfo & 0x3) + 1;
+ int sizeBytes = largeSize ? U32_SIZE : 1;
+ Preconditions.checkArgument(1L + sizeBytes <= length, "variant container
header truncated");
+ long numElements = readUnsignedLittleEndian(valueBuffer, pos + 1,
sizeBytes);
+ long idStart = 1L + sizeBytes;
+ long idBytes = isObject ? numElements * idSize : 0L;
+ long offsetStart = idStart + idBytes;
+ long offsetBytes = (numElements + 1) * offsetSize;
+ long dataStart = offsetStart + offsetBytes;
+ Preconditions.checkArgument(
+ dataStart <= length, "variant container offset table extends past
buffer: numElements=%s", numElements);
+ long dataLen = length - dataStart;
+ if (isObject) {
+ for (int i = 0; i < numElements; i++) {
+ int id = readUnsignedLittleEndian(valueBuffer, pos + (int) idStart + i
* idSize, idSize);
+ Preconditions.checkArgument(
+ id < dictSize, "variant object key id %s out of range
(dictSize=%s)", id, dictSize);
+ }
+ }
+ // Each child offset must lie within the data region. Children may overlap
or leave gaps;
+ // the trailing terminator offset is range-checked for the same reason.
+ for (int i = 0; i <= numElements; i++) {
Review Comment:
Could we bound this cost? Construction now scans every container entry, so
single-element access to a large array becomes O(N), and repeated child access
rescans.
##########
parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java:
##########
@@ -81,43 +104,59 @@ public Variant(byte[] value, int valuePos, int
valueLength, byte[] metadata, int
this(value, metadata.getEncodedBuffer());
}
+ /**
+ * Create a Variant from a tuple of (value, metadata) buffers.
+ * Includes validation of the version and the toplevel structure.
+ * @param value value buffer
+ * @param metadata metadata buffer
+ * @throws UnsupportedOperationException if there is a version mismatch
+ * @throws IllegalArgumentException for any validation failure.
+ */
public Variant(ByteBuffer value, ByteBuffer metadata) {
this.value = value.asReadOnlyBuffer().order(ByteOrder.LITTLE_ENDIAN);
this.metadata = metadata.asReadOnlyBuffer().order(ByteOrder.LITTLE_ENDIAN);
-
- // There is currently only one allowed version.
- if ((metadata.get(metadata.position()) & VariantUtil.VERSION_MASK) !=
VariantUtil.VERSION) {
- throw new UnsupportedOperationException(String.format(
- "Unsupported variant metadata version: %d",
- metadata.get(metadata.position()) & VariantUtil.VERSION_MASK));
- }
+ this.depth = 0;
+ // A metadata buffer must contain at least the version byte
+ Preconditions.checkArgument(this.metadata.remaining() >= 1, "variant
metadata is empty");
+ // There is no checking of version until a policy of how to handle
mismatched versions
Review Comment:
The Javadoc still says unsupported versions throw
`UnsupportedOperationException`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
