Merge branch 'master' into bump-1.11.0

.github/workflows/ion-java-performance-regression-detector.yml

@@ -6,7 +6,7 @@ name: Ion Java performance regression detector
 on:
   pull_request:
     paths:
-      - 'src/*'
+      - 'src/**'
 
 permissions:
   contents: read
@@ -74,12 +74,15 @@ jobs:
         uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
         with:
           ref: ${{ github.base_ref }}
+          submodules: recursive
           path: baseline
 
       - name: Checkout ion-java from the new commit.
         uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
         with:
+          repository: ${{ github.event.pull_request.head.repo.full_name }}
           ref: ${{ github.head_ref }}
+          submodules: recursive
           path: new
 
       - name: Download test Ion Data from artifacts
@@ -92,7 +95,7 @@ jobs:
       - name: Build ion-java from the base commit
         working-directory: baseline
         run: |
-          git submodule init && git submodule update && ./gradlew clean publishToMavenLocal
+          ./gradlew clean publishToMavenLocal
 
       - name: Benchmark ion-java from the base commit
         working-directory: ion-java-benchmark-cli
@@ -104,7 +107,7 @@ jobs:
       - name: Build ion-java from the new commit
         working-directory: new
         run: |
-          git submodule init && git submodule update && ./gradlew clean publishToMavenLocal
+          ./gradlew clean publishToMavenLocal
 
       - name: Benchmark ion-java from the new commit
         working-directory: ion-java-benchmark-cli

.github/workflows/ion-test-driver.yml

@@ -3,7 +3,7 @@ name: ion-test-driver
 on:
   pull_request:
     paths:
-      - 'src/*'
+      - 'src/**'
 
 permissions:
   contents: read

src/com/amazon/ion/impl/IonCursorBinary.java

@@ -9,6 +9,7 @@
 import com.amazon.ion.IonCursor;
 import com.amazon.ion.IonType;
 import com.amazon.ion.IvmNotificationConsumer;
+import com.amazon.ion.SystemSymbols;
 
 import java.io.ByteArrayInputStream;
 import java.io.EOFException;
@@ -388,7 +389,10 @@ private static IonBufferConfiguration getFixedSizeConfigurationFor(
         ByteArrayInputStream inputStream,
         int alreadyReadLen
     ) {
-        int fixedBufferSize = inputStream.available();
+        // Note: ByteArrayInputStream.available() can return a negative number because its constructor does
+        // not validate that the offset and length provided are actually within range of the provided byte array.
+        // Setting the result to 0 in this case avoids an error when looking up the fixed sized configuration.
+        int fixedBufferSize = Math.max(0, inputStream.available());
         if (alreadyReadLen > 0) {
             fixedBufferSize += alreadyReadLen;
         }
@@ -523,8 +527,7 @@ private boolean fillAt(long index, long numberOfBytes) {
             refillableState.bytesRequested = numberOfBytes + (index - offset);
             if (ensureCapacity(refillableState.bytesRequested)) {
                 // Fill all the free space, not just the shortfall; this reduces I/O.
-                refill(freeSpaceAt(limit));
-                shortfall = refillableState.bytesRequested - availableAt(offset);
+                shortfall = refill(refillableState.bytesRequested);
             } else {
                 // The request cannot be satisfied, but not because data was unavailable. Return normally; it is the
                 // caller's responsibility to recover.
@@ -645,24 +648,34 @@ private void shiftIndicesLeft(int shiftAmount) {
     }
 
     /**
-     * Fills the buffer with up to the requested number of additional bytes. It is the caller's responsibility to
-     * ensure that there is space in the buffer.
-     * @param numberOfBytesToFill the number of additional bytes to attempt to add to the buffer.
+     * Attempts to fill the buffer with up to the requested number of additional bytes. It is the caller's
+     * responsibility to ensure that there is space in the buffer.
+     * @param minimumNumberOfBytesRequired the minimum number of bytes requested to fill the current value.
+     * @return the shortfall between the number of bytes that were filled and the minimum number requested. If less than
+     *  1, then at least `minimumNumberOfBytesRequired` were filled.
      */
-    private void refill(long numberOfBytesToFill) {
+    private long refill(long minimumNumberOfBytesRequired) {
         int numberOfBytesFilled = -1;
-        try {
-            numberOfBytesFilled = refillableState.inputStream.read(buffer, (int) limit, (int) numberOfBytesToFill);
-        } catch (EOFException e) {
-            // Certain InputStream implementations (e.g. GZIPInputStream) throw EOFException if more bytes are requested
-            // to read than are currently available (e.g. if a header or trailer is incomplete).
-        } catch (IOException e) {
-            throwAsIonException(e);
-        }
-        if (numberOfBytesFilled < 0) {
-            return;
-        }
-        limit += numberOfBytesFilled;
+        long shortfall;
+        // Sometimes an InputStream implementation will return fewer than the number of bytes requested even
+        // if the stream is not at EOF. If this happens and there is still a shortfall, keep requesting bytes
+        // until either the shortfall is filled or EOF is reached.
+        do {
+            try {
+                numberOfBytesFilled = refillableState.inputStream.read(buffer, (int) limit, (int) freeSpaceAt(limit));
+            } catch (EOFException e) {
+                // Certain InputStream implementations (e.g. GZIPInputStream) throw EOFException if more bytes are requested
+                // to read than are currently available (e.g. if a header or trailer is incomplete).
+                numberOfBytesFilled = -1;
+            } catch (IOException e) {
+                throwAsIonException(e);
+            }
+            if (numberOfBytesFilled > 0) {
+                limit += numberOfBytesFilled;
+            }
+            shortfall = minimumNumberOfBytesRequired - availableAt(offset);
+        } while (shortfall > 0 && numberOfBytesFilled >= 0);
+        return shortfall;
     }
 
     /**
@@ -1457,7 +1470,7 @@ private Event slowStepOutOfContainer() {
             if (slowSeek(parent.endIndex - offset)) {
                 return event;
             }
-            peekIndex = parent.endIndex;
+            peekIndex = offset;
         }
         setCheckpointBeforeUnannotatedTypeId();
         if (--containerIndex >= 0) {
@@ -1786,15 +1799,26 @@ Marker getValueMarker() {
      * can be used to seek the reader to a "span" of bytes that represent a value in the stream.
      * @param offset the offset at which the slice will begin.
      * @param limit the slice's limit.
+     * @param ionVersionId the Ion version ID for the slice, e.g. $ion_1_0 for Ion 1.0.
      */
-    void slice(long offset, long limit) {
+    void slice(long offset, long limit, String ionVersionId) {
         peekIndex = offset;
         this.limit = limit;
         setCheckpointBeforeUnannotatedTypeId();
         valueMarker.endIndex = -1;
         event = Event.NEEDS_DATA;
         valueTid = null;
-        containerIndex = -1; // Slices are treated as if they were at the top level.
+        // Slices are treated as if they were at the top level.
+        parent = null;
+        containerIndex = -1;
+        if (SystemSymbols.ION_1_0.equals(ionVersionId)) {
+            typeIds = IonTypeID.TYPE_IDS_1_0;
+            majorVersion = 1;
+            minorVersion = 0;
+        } else {
+            // TODO changes are needed here to support Ion 1.1.
+            throw new IonException(String.format("Attempted to seek using an unsupported Ion version %s.", ionVersionId));
+        }
     }
 
     /**

src/com/amazon/ion/impl/IonReaderContinuableApplicationBinary.java

@@ -214,7 +214,7 @@ private SymbolTable getSystemSymbolTable() {
     /**
      * Read-only snapshot of the local symbol table at the reader's current position.
      */
-    private class LocalSymbolTableSnapshot implements SymbolTable, SymbolTableAsStruct {
+    private class LocalSymbolTableSnapshot implements _Private_LocalSymbolTable, SymbolTableAsStruct {
 
         // The system symbol table.
         private final SymbolTable system = IonReaderContinuableApplicationBinary.this.getSystemSymbolTable();
@@ -425,6 +425,17 @@ public IonStruct getIonRepresentation(ValueFactory valueFactory) {
             }
             return structCache.getIonRepresentation(valueFactory);
         }
+
+        @Override
+        public _Private_LocalSymbolTable makeCopy() {
+            // This is a mutable copy. LocalSymbolTable handles the mutability concerns.
+            return new LocalSymbolTable(importedTables, Arrays.asList(idToText));
+        }
+
+        @Override
+        public SymbolTable[] getImportedTablesNoCopy() {
+            return importedTables.getImportedTablesNoCopy();
+        }
     }
 
     /**
@@ -544,7 +555,7 @@ String getSymbol(int sid) {
         }
         int localSymbolOffset = sid - firstLocalSymbolId;
         if (localSymbolOffset > localSymbolMaxOffset) {
-            throw new IonException("Symbol ID exceeds the max ID of the symbol table.");
+            throw new UnknownSymbolException(sid);
         }
         return symbols[localSymbolOffset];
     }
@@ -565,7 +576,7 @@ private SymbolToken getSymbolToken(int sid) {
             }
         }
         if (sid >= symbolTableSize) {
-            throw new IonException("Symbol ID exceeds the max ID of the symbol table.");
+            throw new UnknownSymbolException(sid);
         }
         SymbolToken token = symbolTokensById.get(sid);
         if (token == null) {

src/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java

@@ -51,11 +51,13 @@ class IonReaderContinuableCoreBinary extends IonCursorBinary implements IonReade
     // The number of bytes occupied by a Java long.
     private static final int LONG_SIZE_IN_BYTES = 8;
 
-    // The smallest negative 8-byte integer that can fit in a long is -0x80_00_00_00_00_00_00_00.
-    private static final int MOST_SIGNIFICANT_BYTE_OF_MIN_LONG = 0x80;
+    // The smallest negative 8-byte integer that can fit in a long is 0x80_00_00_00_00_00_00_00 and the smallest
+    // negative 4-byte integer that can fit in an int is 0x80_00_00_00.
+    private static final int MOST_SIGNIFICANT_BYTE_OF_MIN_INTEGER = 0x80;
 
-    // The largest positive 8-byte integer that can fit in a long is 0x7F_FF_FF_FF_FF_FF_FF_FF.
-    private static final int MOST_SIGNIFICANT_BYTE_OF_MAX_LONG = 0x7F;
+    // The largest positive 8-byte integer that can fit in a long is 0x7F_FF_FF_FF_FF_FF_FF_FF and the largest positive
+    // 4-byte integer that can fit in an int is 0x7F_FF_FF_FF.
+    private static final int MOST_SIGNIFICANT_BYTE_OF_MAX_INTEGER = 0x7F;
 
     // The second-most significant bit in the most significant byte of a VarInt is the sign.
     private static final int VAR_INT_SIGN_BITMASK = 0x40;
@@ -418,34 +420,30 @@ private boolean readBoolean_1_0() {
     }
 
     /**
-     * Determines whether the 8-byte integer starting at `valueMarker.startIndex` and ending at `valueMarker.endIndex`
-     * fits in a long, or requires a BigInteger.
-     * @return either `IntegerSize.LONG` or `IntegerSize.BIG_INTEGER`.
+     * Determines whether the integer starting at `valueMarker.startIndex` and ending at `valueMarker.endIndex`
+     * crosses a type boundary. Callers must only invoke this method when the integer's size is known to be either
+     * 4 or 8 bytes.
+     * @return true if the value fits in the Java integer type that matches its Ion serialized size; false if it
+     *  requires the next larger size.
      */
-    private IntegerSize classifyEightByteInt_1_0() {
+    private boolean classifyInteger_1_0() {
         if (valueTid.isNegativeInt) {
-            // The smallest negative 8-byte integer that can fit in a long is -0x80_00_00_00_00_00_00_00.
             int firstByte = buffer[(int)(valueMarker.startIndex)] & SINGLE_BYTE_MASK;
-            if (firstByte < MOST_SIGNIFICANT_BYTE_OF_MIN_LONG) {
-                return IntegerSize.LONG;
-            } else if (firstByte > MOST_SIGNIFICANT_BYTE_OF_MIN_LONG) {
-                return IntegerSize.BIG_INTEGER;
+            if (firstByte < MOST_SIGNIFICANT_BYTE_OF_MIN_INTEGER) {
+                return true;
+            } else if (firstByte > MOST_SIGNIFICANT_BYTE_OF_MIN_INTEGER) {
+                return false;
             }
             for (long i = valueMarker.startIndex + 1; i < valueMarker.endIndex; i++) {
                 if (0x00 != buffer[(int)(i)]) {
-                    return IntegerSize.BIG_INTEGER;
+                    return false;
                 }
             }
-        } else {
-            // The largest positive 8-byte integer that can fit in a long is 0x7F_FF_FF_FF_FF_FF_FF_FF.
-            if ((buffer[(int)(valueMarker.startIndex)] & SINGLE_BYTE_MASK) > MOST_SIGNIFICANT_BYTE_OF_MAX_LONG) {
-                return IntegerSize.BIG_INTEGER;
-            }
+            return true;
         }
-        return IntegerSize.LONG;
+        return (buffer[(int) (valueMarker.startIndex)] & SINGLE_BYTE_MASK) <= MOST_SIGNIFICANT_BYTE_OF_MAX_INTEGER;
     }
 
-
     int readVarUInt_1_1() {
         throw new UnsupportedOperationException();
     }
@@ -536,16 +534,13 @@ public IntegerSize getIntegerSize() {
         if (valueTid.length < 0) {
             return IntegerSize.BIG_INTEGER;
         } else if (valueTid.length < INT_SIZE_IN_BYTES) {
-            // Note: this is conservative. Most integers of size 4 also fit in an int, but since exactly the
-            // same parsing code is used for ints and longs, there is no point wasting the time to determine the
-            // smallest possible type.
             return IntegerSize.INT;
+        } else if (valueTid.length == INT_SIZE_IN_BYTES) {
+            return (minorVersion != 0 || classifyInteger_1_0()) ? IntegerSize.INT : IntegerSize.LONG;
         } else if (valueTid.length < LONG_SIZE_IN_BYTES) {
             return IntegerSize.LONG;
         } else if (valueTid.length == LONG_SIZE_IN_BYTES) {
-            // Because creating BigIntegers is so expensive, it is worth it to look ahead and determine exactly
-            // which 8-byte integers can fit in a long.
-            return minorVersion == 0 ? classifyEightByteInt_1_0() : IntegerSize.LONG;
+            return (minorVersion != 0 || classifyInteger_1_0()) ? IntegerSize.LONG : IntegerSize.BIG_INTEGER;
         }
         return IntegerSize.BIG_INTEGER;
     }

src/com/amazon/ion/impl/IonReaderContinuableTopLevelBinary.java

@@ -97,6 +97,11 @@ public SymbolTable pop_passed_symbol_table() {
             return null;
         }
         symbolTableLastTransferred = currentSymbolTable;
+        if (symbolTableLastTransferred.isLocalTable()) {
+            // This method is called when transferring the reader's symbol table to either a writer or an IonDatagram.
+            // Those cases require a mutable copy of the reader's symbol table.
+            return ((_Private_LocalSymbolTable) symbolTableLastTransferred).makeCopy();
+        }
         return symbolTableLastTransferred;
     }
 
@@ -303,8 +308,8 @@ public void hoist(Span span) {
             // of the value to be the end of the stream, in order to comply with the SeekableReader contract. From
             // an implementation perspective, this is not necessary; if we leave the buffer's limit unchanged, the
             // reader can continue after processing the hoisted value.
-            slice(binarySpan.bufferOffset, binarySpan.bufferLimit);
             restoreSymbolTable(binarySpan.symbolTable);
+            slice(binarySpan.bufferOffset, binarySpan.bufferLimit, binarySpan.symbolTable.getIonVersionId());
             type = null;
         }
     }

src/com/amazon/ion/impl/LocalSymbolTable.java

@@ -51,7 +51,7 @@
  * Instances of this class are safe for use by multiple threads.
  */
 class LocalSymbolTable
-    implements SymbolTable
+    implements _Private_LocalSymbolTable
 {
 
     static class Factory implements _Private_LocalSymbolTableFactory
@@ -329,7 +329,8 @@ else if (fieldType == IonType.SYMBOL && ION_SYMBOL_TABLE.equals(reader.stringVal
         return new LocalSymbolTableImports(importsList);
     }
 
-    synchronized LocalSymbolTable makeCopy()
+    @Override
+    public synchronized _Private_LocalSymbolTable makeCopy()
     {
         return new LocalSymbolTable(this, getMaxId());
     }
@@ -604,19 +605,8 @@ public SymbolTable[] getImportedTables()
         return myImportsList.getImportedTables();
     }
 
-    /**
-     * Returns the imported symbol tables without making a copy.
-     * <p>
-     * <b>Note:</b> Callers must not modify the resulting SymbolTable array!
-     * This will violate the immutability property of this class.
-     *
-     * @return
-     *          the imported symtabs, as-is; the first element is a system
-     *          symtab, the rest are non-system shared symtabs
-     *
-     * @see #getImportedTables()
-     */
-    SymbolTable[] getImportedTablesNoCopy()
+    @Override
+    public SymbolTable[] getImportedTablesNoCopy()
     {
         return myImportsList.getImportedTablesNoCopy();
     }

src/com/amazon/ion/impl/_Private_IonBinaryWriterBuilder.java

@@ -157,7 +157,7 @@ public void setInitialSymbolTable(SymbolTable symtab)
             if (symtab.isLocalTable())
             {
                 SymbolTable[] imports =
-                    ((LocalSymbolTable) symtab).getImportedTablesNoCopy();
+                    ((_Private_LocalSymbolTable) symtab).getImportedTablesNoCopy();
                 for (SymbolTable imported : imports)
                 {
                     if (imported.isSubstitute())
@@ -353,7 +353,7 @@ SymbolTable buildContextSymbolTable()
             return myInitialSymbolTable;
         }
 
-        return ((LocalSymbolTable) myInitialSymbolTable).makeCopy();
+        return ((_Private_LocalSymbolTable) myInitialSymbolTable).makeCopy();
     }