Re: [PR] Add targetDocsPerChunk cap to VarByteChunkForwardIndexWriterV6 and wire it to table config [pinot]

["xiangfu0 (via GitHub)" <gi...@apache.org>]

xiangfu0 commented on PR #18772:
URL: https://github.com/apache/pinot/pull/18772#issuecomment-5274007959

   Closing this — benchmarking showed the premise doesn't hold. `targetMaxChunkSize` already
   provides everything the docs-per-chunk cap was meant to add, so the new knob isn't needed.
   
   ## What the measurements showed
   
   Setup: 1M docs, ZSTD, V6, JDK 25, Apple Silicon, warm buffers. `randomAccess` = 2000 uniform
   point lookups.
   
   **1. Chunk *bytes*, not docs, is what drives lookup latency.** Normalizing the docs-cap sweep two
   ways, cross-dataset spread at a matched setting is 2.08–5.67x per doc but only 1.08–2.54x per
   uncompressed KB. Decompression cost is a function of bytes; docs only proxy for bytes via average
   value length, which varies per column.
   
   **2. `targetMaxChunkSize` alone already controls chunk geometry — including on skewed columns.**
   Through `SingleValueVarByteRawIndexCreator`, with `targetDocsPerChunk` at its default 1000 and no
   enforcement:
   
   | targetMaxChunkSize | LOW_CARD_URL docs/chunk | SKEWED_LOG docs/chunk |
   |---|---|---|
   | 8 KB | 104 | 110 |
   | 16 KB | 210 | 189 |
   | 32 KB | 422 | 293 |
   | 1 MB (default) | 954 | 9345 |
   
   SKEWED_LOG is avg 107 B with 0.1% ~50 KB values (max 50094 B). The byte budget tracks fine
   regardless of skew. What's broken is only `targetDocsPerChunk`'s `maxLength * N` *derivation*,
   which clamps to `targetMaxChunkSize` and stops tracking N — not byte control itself.
   
   **3. The docs cap does not buy a better size/latency frontier.** The two frontiers interleave:
   
   | dataset | byte-bounded | doc-bounded | result |
   |---|---|---|---|
   | SKEWED_LOG | 32 KB → 8.79 MB, **20.3 µs** | cap 1000 → 8.73 MB, 38.3 µs | byte 1.9x faster, same size |
   | SKEWED_LOG | 16 KB → **9.15 MB, 10.3 µs** | cap 250 → 9.24 MB, 12.1 µs | byte dominates |
   | JSON_LOG | 16 KB → **36.12 MB, 14.1 µs** | cap 1000 → 36.28 MB, 101.0 µs | byte dominates, 7x |
   | LOW_CARD_URL | 64 KB → 3.27 MB, 17.2 µs | cap 1000 → 3.24 MB, 17.8 µs | tie |
   | LOW_CARD_URL | 16 KB → 3.87 MB, 9.7 µs | cap 250 → **3.76 MB, 6.9 µs** | docs cap slightly ahead |
   
   **4. On skewed data the docs cap is actively worse.** It forces a chunk that swallows a 50 KB
   outlier to still collect N docs, so on-disk chunk sizes vary 8.2x; the byte cap flushes early and
   stays uniform:
   
   | dataset | mode | median | p95 | p99 | max | max/median |
   |---|---|---|---|---|---|---|
   | SKEWED_LOG | cap 250 | 1573 B | 4646 | 7434 | 12888 | **8.2x** |
   | SKEWED_LOG | byte 32 KB | 2908 B | 3118 | 3133 | 3158 | **1.1x** |
   | LOW_CARD_URL | cap 250 | 933 B | 950 | 957 | 968 | 1.0x |
   | LOW_CARD_URL | byte 32 KB | 1465 B | 1487 | 1495 | 1508 | 1.0x |
   
   Uniform doc counts are the wrong invariant for latency — uniform bytes is what you want, and that's
   what the byte budget already gives.
   
   ## The useful finding, which needs no code
   
   For point-lookup-heavy raw string/bytes columns, set `targetMaxChunkSize` to 16–32 KB. On the
   skewed column that is **336 µs → 10.3 µs per lookup (33x) for 12% more space** (8.15 → 9.15 MB).
   The 1 MB default is reasonable for scans and poor for point lookups. Worth documenting as tuning
   guidance rather than adding a knob.
   
   Caveats on the numbers: warm-buffer only (cold-page mmap should favor small chunks further, so the
   lookup win is likely understated); ZSTD only for latency; random-access error bars are wide at 1 MB
   chunks. Directions and orders of magnitude are stable across reruns.
   
   ## Possible follow-up
   
   The one real defect left is that `targetDocsPerChunk` is named as if it controls document count but
   only picks a byte budget from `maxLength`, then silently clamps. Deprecating that derivation in
   favor of `targetMaxChunkSize` would be a much smaller and clearer change than this PR. Happy to
   open that separately if there's interest.
   
   Benchmark used here (`BenchmarkVarByteV6TargetDocsPerChunk`) can be resurrected from this branch if
   useful for the follow-up.
   


-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org