Single-pass symbol-dict append; test window remap

PersistedSymbolDict.appendSymbols encoded the mapped chunk in two
passes over the id range: a first loop called Utf8s.utf8Bytes on every
symbol just to size the header varint and the mmap reserve, then a
second loop re-walked each symbol's UTF-8 length to write it. It now
stages the entry region in the scratch buffer with a single walk per
symbol and bulk-copies it into the append window after the header,
dropping the redundant walk. The chunk it writes is byte-identical, so
the exact-file-size "one chunk, one CRC" test still holds. A direct
in-window encode cannot avoid the double walk: the back-to-back chunk
format leaves no room to reserve and back-fill the header in place.
The path is cold -- only a retry after a failed publish re-encodes
from the dictionary; the steady state persists a frame's pre-encoded
bytes through appendRawEntries.

Add testMappedAppendSpansMultipleWindowsAndRecoversDenseIds: it appends
a dictionary larger than one 4 MiB append window, asserts the window
remaps (appendMapGrowthCount >= 2) without a positioned write, then
reopens and checks every id recovers to its own symbol across the
boundary. This exercises the ensureAppendMap remap arithmetic and the
single-pass encode as chunks straddle the boundary, both of which the
single-window happy path left untested.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: glasstiger

core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/PersistedSymbolDict.java

@@ -415,11 +415,18 @@ public synchronized void appendSymbol(CharSequence symbol) {
 
     /**
      * Appends the dense id range {@code [from .. to]} as one chunk with one
-     * checksum. Production encodes directly into a segmented append mapping, avoiding
-     * both the staging copy and a positioned-write syscall on every flush. The
-     * scratch/positioned-write fallback exists only behind an injected filesystem
-     * facade so short-write recovery tests retain their fault seam. Callers pass the
-     * dictionary and the range so the ids resolve to their symbol strings.
+     * checksum. This is the RE-ENCODE path: the steady-state persist ships a frame's
+     * pre-encoded delta bytes through {@link #appendRawEntries}, and only a retry
+     * after a failed publish rebuilds a range straight from the dictionary. The
+     * mapped path stages the entry region in the scratch buffer with a single UTF-8
+     * walk per symbol, then bulk-copies it into the append window after the header,
+     * and still commits WITHOUT a positioned-write syscall. A direct encode into the
+     * window would walk each symbol's UTF-8 length twice -- the exact entriesLen sizes
+     * both the header varint and the mmap reserve -- and the back-to-back chunk format
+     * leaves no room to reserve-and-back-fill the header in place. The
+     * positioned-write fallback runs only behind an injected filesystem facade so
+     * short-write recovery tests retain their fault seam. Callers pass the dictionary
+     * and the range so the ids resolve to their symbol strings.
      * <p>
      * Same durability and idempotency contract as {@link #appendSymbol}: no fsync,
      * and a short write throws WITHOUT advancing {@code size}/{@code appendOffset},
@@ -432,32 +439,32 @@ public synchronized void appendSymbols(GlobalSymbolDictionary dict, int from, in
         }
         int count = to - from + 1;
         if (mappedAppend) {
-            long entriesLenLong = 0L;
+            // Stage the entry region in scratch with ONE UTF-8 walk per symbol, then
+            // bulk-copy it into the append window after the header (see the method
+            // javadoc for why a direct in-window encode would have to walk each symbol
+            // twice). ensureScratch enforces the same MAX_SCRATCH_BYTES ceiling the old
+            // sizing pass did, throwing before size/appendOffset advance.
+            int entriesLen = 0;
             for (int id = from; id <= to; id++) {
-                int utf8Len = Utf8s.utf8Bytes(dict.getSymbol(id));
-                entriesLenLong += NativeBufferWriter.varintSize(utf8Len) + (long) utf8Len;
-                if (entriesLenLong > MAX_SCRATCH_BYTES) {
-                    throw new IllegalStateException("symbol dict chunk exceeds the maximum size to "
-                            + FILE_NAME + " [required=" + entriesLenLong + ", max="
-                            + MAX_SCRATCH_BYTES + ']');
+                CharSequence symbol = dict.getSymbol(id);
+                int utf8Len = Utf8s.utf8Bytes(symbol);
+                int wireLen = NativeBufferWriter.varintSize(utf8Len) + utf8Len; // [len][utf8]
+                ensureScratch((long) entriesLen + wireLen);
+                long q = NativeBufferWriter.writeVarint(scratchAddr + entriesLen, utf8Len);
+                if (utf8Len > 0) {
+                    Utf8s.strCpyUtf8(symbol, q, utf8Len);
                 }
+                entriesLen += wireLen;
             }
-            int entriesLen = (int) entriesLenLong;
             int hdrLen = NativeBufferWriter.varintSize(count)
                     + NativeBufferWriter.varintSize(entriesLen);
             long recLen = (long) hdrLen + entriesLen + CRC_SIZE;
             ensureAppendMap(checkedRequiredOffset(recLen));
             long recStart = appendMapAddr + appendOffset - appendMapOffset;
             long p = NativeBufferWriter.writeVarint(recStart, count);
-            p = NativeBufferWriter.writeVarint(p, entriesLen);
-            for (int id = from; id <= to; id++) {
-                CharSequence symbol = dict.getSymbol(id);
-                int utf8Len = Utf8s.utf8Bytes(symbol);
-                p = NativeBufferWriter.writeVarint(p, utf8Len);
-                if (utf8Len > 0) {
-                    Utf8s.strCpyUtf8(symbol, p, utf8Len);
-                    p += utf8Len;
-                }
+            NativeBufferWriter.writeVarint(p, entriesLen);
+            if (entriesLen > 0) {
+                Unsafe.getUnsafe().copyMemory(scratchAddr, recStart + hdrLen, entriesLen);
             }
             commitMappedChunk(recStart, hdrLen, entriesLen, count);
             return;

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/PersistedSymbolDictTest.java

@@ -452,6 +452,54 @@ public void testMappedAppendUsesMultiMegabyteWindowsWithoutPositionedWrites() th
         });
     }
 
+    @Test
+    public void testMappedAppendSpansMultipleWindowsAndRecoversDenseIds() throws Exception {
+        // A dictionary larger than one 4 MiB append window must remap the window
+        // mid-append (appendMapGrowthCount >= 2) and still recover every symbol in
+        // dense id order across the boundary. Guards two paths the single-window
+        // happy path leaves uncovered: the remap arithmetic in ensureAppendMap, and
+        // the single-pass appendSymbols encode as its chunks straddle the boundary.
+        assertMemoryLeak(() -> {
+            final int n = 640;
+            final String pad = TestUtils.repeat("x", 8192); // ~8 KiB per symbol -> >4 MiB total
+            GlobalSymbolDictionary dict = new GlobalSymbolDictionary();
+            for (int i = 0; i < n; i++) {
+                dict.getOrAddSymbol("sym-" + i + "-" + pad); // distinct, id == i
+            }
+            Assert.assertEquals(n, dict.size());
+
+            Path dir = newFolder("qwp-symdict");
+            try (PersistedSymbolDict d = PersistedSymbolDict.open(dir.toString())) {
+                Assert.assertNotNull(d);
+                // Multi-symbol batches so the appendSymbols encode loop is exercised,
+                // not just the single-symbol path.
+                for (int from = 0; from < n; from += 8) {
+                    int to = Math.min(from + 8, n) - 1;
+                    d.appendSymbols(dict, from, to);
+                }
+                Assert.assertEquals(n, d.size());
+                Assert.assertEquals("production mmap appends must not use positioned writes",
+                        0L, d.appendWriteCount());
+                Assert.assertTrue(
+                        "a dictionary larger than the 4 MiB window must remap at least once (saw "
+                                + d.appendMapGrowthCount() + ")",
+                        d.appendMapGrowthCount() >= 2);
+            }
+
+            // Fresh open: every id must resolve to its own symbol, including the ids
+            // whose chunks straddled the 4 MiB window boundary.
+            try (PersistedSymbolDict re = PersistedSymbolDict.open(dir.toString())) {
+                Assert.assertNotNull(re);
+                Assert.assertEquals(n, re.size());
+                ObjList<String> got = re.readLoadedSymbols();
+                for (int i = 0; i < n; i++) {
+                    Assert.assertEquals("symbol at id " + i + " must survive the window boundary",
+                            dict.getSymbol(i), got.getQuick(i));
+                }
+            }
+        });
+    }
+
     private static int varintSize(int v) {
         int n = 1;
         while ((v >>>= 7) != 0) {