Copilot commented on code in PR #3865:
URL: https://github.com/apache/avro/pull/3865#discussion_r3567492636
##########
lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumReader.java:
##########
@@ -114,6 +115,26 @@ public void setExpected(Schema reader) {
private static final ThreadLocal<Map<Schema, Map<Schema, ResolvingDecoder>>>
RESOLVER_CACHE = ThreadLocalWithInitial
.of(WeakIdentityHashMap::new);
+ /**
+ * Upper bound on the initial capacity eagerly allocated for a collection
from
+ * its declared block count. The backing array/map grows on demand as
elements
+ * are read, so this is only a starting hint: it prevents a large declared
count
+ * from driving a huge up-front allocation before any element is decoded.
This
Review Comment:
initialCollectionCapacity() caps the initial backing allocation to 1024
unconditionally. This achieves the stated goal for stream sources
(remainingBytes() == -1), but it also changes behavior for byte-array-backed
decoders where remainingBytes() is exact and ensureAvailableCollectionBytes()
already validates the declared count against available bytes. For large valid
arrays/maps from byte[] sources, always starting at 1024 can cause repeated
backing-array growth/copying that the previous preallocation avoided. Consider
making the clamp conditional on the decoder not knowing remaining bytes (or
otherwise having a bounded safe preallocation path), so performance for large
in-memory inputs doesn’t regress unnecessarily.
##########
lang/java/avro/src/main/java/org/apache/avro/io/BinaryDecoder.java:
##########
@@ -458,7 +466,7 @@ public long arrayNext() throws IOException {
@Override
public long skipArray() throws IOException {
- return doSkipItems();
+ return SystemLimitException.checkMaxCollectionLength(doSkipItems());
}
Review Comment:
skipArray()/skipMap() now apply checkMaxCollectionLength(doSkipItems()), but
doSkipItems() still reads the raw block count via readLong() and will accept a
Long.MIN_VALUE negative count (treating it as a “negative block with
byte-size”) and continue skipping. That’s inconsistent with doReadItemCount(),
which now explicitly rejects Long.MIN_VALUE as an invalid/malformed block
count, and means malformed data could be accepted when the field is skipped
during schema resolution/projection. Consider adding the same Long.MIN_VALUE
rejection in doSkipItems() so invalid block counts are rejected consistently on
both read and skip paths.
--
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]
