Reuse fixed-row buffers for DataTable result construction - #19533
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19533 +/- ##
=========================================
Coverage 67.72% 67.72%
Complexity 1450 1450
=========================================
Files 3490 3490
Lines 225032 225032
Branches 35527 35527
=========================================
+ Hits 152393 152404 +11
+ Misses 60612 60595 -17
- Partials 12027 12033 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
655e259 to
f55d2af
Compare
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Please check if we always fill all columns of a row. If so, we can just allocate a buffer in the constructor, and there is no need to clear/reset the buffer.
|
@Jackie-Jiang Confirmed: every production row writer populates every column, including null values. Each setter writes its complete fixed-width slot (or offset/length pair) and sets its own buffer position; Addressed in b51ffd1: the buffer is now allocated once in the constructor and is final, and All 83 focused serde, selection, and GROUP BY tests passed on JDK 25, along with Spotless, Checkstyle, and license checks. Changed-file warning-enabled compilation passed with no warnings on added lines. The existing full warning-enabled reactor compilation issue with the missing |
Building multi-row DataTables creates a short-lived fixed-row buffer for every row. Reuse the builder-owned buffer while resetting its contents and state so partially written rows retain their existing wire representation.
Allocate the row scratch buffer once in the constructor after verifying all production callers populate every column. Document the complete-row contract and cover reverse-order writes, empty values, and explicit nulls in serde. Reproduction: Construct a multi-row V4 DataTable. The previous startRow implementation cleared the entire scratch buffer and reset its state even though every setter seeks to and overwrites a complete column slot. Validation: 83 focused serde, selection, and GROUP BY tests passed on JDK 25. Spotless, Checkstyle, and license checks passed. Scoped lint compilation passed with no new-line warnings; full warning-enabled reactor compilation remains blocked by an unchanged missing JetBrains NotNull dependency.
The reused row buffer only needs the computed row size while it is allocated. Remove the unused instance field and keep rowSizeInBytes local to the constructor, addressing the remaining review comment. Validation: 10 DataTableSerDeTest cases passed on JDK 25.
b51ffd1 to
4dd42c5
Compare
3 participants
PR flow
Reuse a single row buffer for all rows, avoiding per-row allocation.
AI-generated · Green: added · Yellow: modified · Red: removed · Gray: existing
Diff evidence
DataTable result construction allocates a byte array and ByteBuffer for every row. Allocate one fixed-row buffer in the
BaseDataTableBuilderconstructor and reuse it without clearing or resetting it between rows. Every production row writer populates every column; each setter writes its complete slot and sets its own position, andfinishRow()copies the bytes into the output stream. V4 wire encoding and variable-length offsets are preserved.Constructor allocation also means a zero-row builder now allocates one schema-sized scratch buffer.
The builder interface now documents that each row must populate every column. The regression test covers fully populated rows, reverse column-write order, empty variable-length values, explicit nulls, NaN payloads and signed zero.
Reproduction and benchmark
Rerun on September 12, 2026 (PDT), comparing measured PR revision
b51ffd1e43467462d34001dcf424f35635d75156with upstream5771d6acea60cd72738116835111cf7c49965373. The exact benchmark source is unchanged from the earlier experiment (SHA-256d7f6e90a71ab371017d85c7fea454435415c214bdc7e832138cd1474a35878b5).buildDataTablemeasures V4 DataTable construction.serializeDataTablemeasures construction plus wire serialization, usingbuild().toBytes(). NUMERIC contains INT/LONG/DOUBLE columns; MIXED adds STRING/BYTES/INT_ARRAY with 128 deterministic input values. Every measured fork validates all logical output fields during setup, outside timing. Rows 0 and 1 are controls; 5,000 rows exercises repeated-row allocation.JMH 1.37, Temurin JDK 25+36-LTS, Apple M2 Max/macOS, one thread, 512 MiB heap. Three alternating AB/BA/AB pairs, one fresh fork per method/shape/row-count cell per arm, three 500 ms warmups and five 500 ms measurements, with GC profiling: 72 forks total. Both arms use the same frozen runtime; only the freshly compiled
BaseDataTableBuilder.classdiffers. Runtime hashes were verified before and after every arm. The machine was on AC power with ordinary desktop/security processes active; no task builds or competing task benchmarks ran during timing.With the benchmark source on each arm's classpath, run the following once per arm in AB/BA/AB order:
Benchmark results for
b51ffd1e43Times and allocations are medians of the three arm means. Paired ratios are medians of the three baseline/candidate time ratios, so values above 1 favor the candidate. Parentheses show the minimum and maximum paired ratios, not confidence intervals.
At 5,000 rows, construction improves 1.420× for NUMERIC and 1.143× for MIXED; construction plus serialization improves 1.364× and 1.184×. The corresponding allocation savings are approximately 480 KB/op for NUMERIC and 560 KB/op for MIXED.
The optimization does not improve every small-result case. One-row MIXED construction is 0.954× (about 5% slower), and construction plus serialization is 0.985×, despite 56 fewer allocated bytes/op. Empty builders allocate 40 additional bytes/op for NUMERIC and 56 for MIXED; empty construction ratios are 0.954× and 0.975×. Empty construction-plus-serialization ratios vary across pairs, as shown above.
Eight full V4 wire outputs (NUMERIC/MIXED at 0, 1, 32 and 5,000 rows) are byte-identical between arms. These are descriptive component measurements on a shared workstation; they exclude query execution, network, broker reduction and ingestion. Three paired forks per case do not establish a production confidence bound.
Validation
Latest review cleanup (
4dd42c503588d0f944700d316c58a0b8036df567): made the constructor-only row-size field a local variable and rebased onto upstream9ee1c681be85a3f018c7b00dcff48aa8d4180800. All 10DataTableSerDeTestcases and the four required Spotless, Checkstyle, and license checks passed on JDK 25. Direct warning-enabled compilation of the changed class passed with no warnings on added lines; the optional reactor warning check still encounters the existing missingNotNulldependency described below. The benchmark figures above remain measurements ofb51ffd1e43; they were not rerun for this cleanup.At measured revision
b51ffd1e43, 83 focused tests passed on JDK 25: 10 DataTable serde, 2 selection utility, 5 selection service, 22 ordered selection/broker reduction, and 44 inter-segment GROUP BY tests. The serde cases cover empty tables and all data types; the updated row-reuse case fills every column.Spotless, Checkstyle, and license format/check passed with no automatic source changes. Independent review across all eight Pinot review domains found no actionable issues. The warning-enabled reactor compilation still fails in unchanged
ZstandardDecompressor.java:51due to missingorg.jetbrains.annotations.NotNull; direct JDK 25-Xlint:allcompilation of all three changed files passed with no warnings on added lines (only existing license-header and raw/unchecked Map-array warnings).