tarun11Mavani opened a new pull request, #19421: URL: https://github.com/apache/pinot/pull/19421
Stacked on #19093 — based on that branch, so until it merges the diff here also shows its 611 lines. Review that one first. Five independent commits on the segment-commit (seal) path. None changes segment output; the parity test in the last commit seals the same input through both the old and new paths and compares the two output directories byte for byte. A wall-clock profile of a production OPEN_STRUCT realtime server puts `buildSegmentForCommit` at 47.7% of the consuming thread — 22.7% waiting on `_segBuildSemaphore` and 25.0% doing the build. `RoaringBitmap.contains` alone is ~11.5%. These commits target the 25.0%, which also shortens how long each build holds a semaphore permit. ## Commits **1. Size the per-key forward index from segment capacity.** `MutableKeyColumn` hardcoded a 1000-row chunk while every other dictionary-encoded SV mutable forward index sizes its chunk from capacity (`ForwardIndexType#createMutableIndex` passes `context.getCapacity()`). A 5M-row segment allocated ~5,000 chunks per key, each costing a log line, a copy of the copy-on-write reader list, and a division plus list lookup on every read. The chunk count is clamped rather than set to capacity outright: the OPEN_STRUCT key space is user-controlled and mutable mode retains every observed key (`maxDenseKeys` applies at seal, not during consumption), so a capacity-sized chunk would reserve `capacity * 4` bytes for a key seen once. *Write 3.4x, sequential read 2.2x.* **2. Log only the first forward-index chunk at INFO.** `addBuffer` logged at INFO on every chunk; the profile attributed 60 of 104 samples under `addBuffer` to the logging call. The first chunk still logs at INFO so the "column started allocating off-heap" signal survives. This file backs every mutable fixed-byte SV column, not only OPEN_STRUCT keys — kept as a standalone commit so it can be dropped or split out if that product-wide change is unwelcome here. **3. Scatter sparse values in windows instead of ranking per doc per key.** `writeSparseJsonColumn` iterated `numDocs * numSparseKeys`, calling `contains` on each pair and `RoaringBitmap.rank` on each hit. Sparse keys are by definition the numerous ones, so this scaled worst with key-space width. Each key's presence bitmap is now walked once, scattering into per-doc buckets, windowed at 64k documents so live maps stay bounded rather than growing with `numDocs`. Per-document JSON key order is unchanged, which an added test pins against the raw forward-index bytes. *13.6–20.7x at 20 and 100 sparse keys over 2M docs.* **4. Walk each dense key twice at seal instead of three times.** Each dense key was scanned end to end three times — statistics, index writes, null vector — probing the presence bitmap per document. The null-vector pass folds into the index-write pass and both step a cursor instead of probing. The statistics pass stays separate because `resolveUseDictionary` and the index creators consume the sealed collector. *2.8–3.8x per dense key.* **5. Hand the seal path a columnar source instead of per-doc maps.** `SegmentColumnarIndexCreator#indexOpenStructDoc` rebuilt a `Map` per document from the mutable per-key columns, then handed it to `OpenStructColumnSplitter`, which re-inferred the type and re-coerced every value to reconstruct the columnar form it started from. Adds an optional bulk path taken only when `sortedDocIds` and `validDocIds` are both null; sorted-column tables and commit-time compaction keep the per-document path, which the offline and minion paths also use. *4.4x at 10 keys, 7.5x at 40.* ## Reviewing Commit 5 is 898 of the 1,328 lines and the only one touching `pinot-segment-spi` (one new interface, two `default` methods). It is deliberately last, so `git reset --hard <commit 4>` drops it cleanly if you would rather review the SPI addition separately. All three new SPI methods are `default`, so existing implementations of `ColumnarOpenStructIndexCreator` and `OpenStructDataSource` compile unchanged and take the per-document path. No config key, segment format, or wire format changes. Known limitation: `SegmentIndexCreationDriverImpl.buildByColumn` passes a non-null `sortedDocIds` whenever a table configures a sorted column, so those tables see no benefit from commit 5. Commits 1, 3 and 4 still apply. ## Testing | Scope | Result | |---|---| | `pinot-segment-local` full module | 5,516 tests, 0 failures | | spotless / checkstyle / license | clean on both modules | New: `MutableKeyColumnChunkSizingTest` (7), `OpenStructColumnarHandoffTest` (3), `SegmentColumnarIndexCreatorOpenStructDispatchTest` (7). The dispatch test covers the gate directly — that the columnar path runs only with both docId inputs null, that a `numDocs` mismatch falls back rather than throwing, and that no creator is partially fed when one reports no columnar support. `OpenStructSparseDenseParityTest`, `OpenStructColumnSplitterTest`, `MutableOpenStructIndexTest`, `MutableOpenStructDataSourceTest` and `RealtimeSegmentConverterTest` pass unchanged. Benchmark figures come from standalone probes run against the built module, not committed JMH sources. Happy to land them under `pinot-perf` if you want them re-runnable. Labels: `performance`. Commit 5 adds SPI methods, so `release-notes` and `extension-point` may also apply. -- 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]
