You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2026/09/12 09:02:47 UTC
[PR] Reuse fixed-row buffers for DataTable result construction [pinot]
xiangfu0 opened a new pull request, #19533:
URL: https://github.com/apache/pinot/pull/19533
DataTable result construction currently allocates a new byte array and ByteBuffer for every row. Reuse one lazily allocated fixed-row buffer per builder, clearing it between rows so omitted fields keep their original zero representation. This reduces Java allocation during selection and GROUP BY result serialization while preserving V4 wire bytes.
Added a regression for populated, partially written and explicit-null rows (including variable-length offsets, NaN payloads and signed zero), plus a deterministic JMH benchmark and raw paired results under `pinot-perf/benchmark-results/datatable-row-buffer-20260912`.
| 5,000-row operation | Median paired speedup | Allocation reduction |
| --- | ---: | ---: |
| Numeric construction | 1.33× | 56.9% |
| Mixed construction | 1.13× | 37.7% |
| Numeric construction + serialization | 1.29× | 38.6% |
| Mixed construction + serialization | 1.10× | 23.7% |
Three alternating baseline/candidate pairs on JDK 25, ARM64 macOS, one thread and a 512 MiB heap. The runtimes differ only in the freshly compiled builder class; the report documents the class-overlay control and includes all raw JMH results. Single-row allocation is unchanged; mixed single-row construction has a 0.976× median paired ratio (roughly 2.4% slower). These are shared-workstation component measurements, not end-to-end query throughput claims. Reproduction commands are in `pinot-perf/README.md`.
Validation: 59 focused tests passed (`DataTableSerDeTest`, `SelectionOperatorServiceTest`, `InterSegmentGroupBySingleValueQueriesTest`); eight deterministic V4 outputs matched baseline wire hashes. Spotless, Checkstyle and both license goals passed. Independent correctness and performance/testing reviews found no actionable issues.
Additional full-reactor `-Xlint:all` compilation stopped in unchanged `ZstandardDecompressor` on a missing `org.jetbrains.annotations.NotNull` class. Normal compilation/tests pass. Scoped lint of the changed sources passes with no warnings on added implementation/test lines; it excludes the repository-standard ASF header's dangling-doc warning and retains two existing raw Map warnings. No dependency workaround is included in this performance PR.
--
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
Re: [PR] Reuse fixed-row buffers for DataTable result construction [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #19533:
URL: https://github.com/apache/pinot/pull/19533
--
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
Re: [PR] Reuse fixed-row buffers for DataTable result construction [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19533:
URL: https://github.com/apache/pinot/pull/19533#issuecomment-5645044923
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19533?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 57.83%. Comparing base ([`5771d6a`](https://app.codecov.io/gh/apache/pinot/commit/5771d6acea60cd72738116835111cf7c49965373?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`86a23c6`](https://app.codecov.io/gh/apache/pinot/commit/86a23c696dcea081f70a7d6da195a269276db20e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (5771d6a) and HEAD (86a23c6). Click for more details.
>
> <details><summary>HEAD has 24 uploads less than BASE</summary>
>
>| Flag | BASE (5771d6a) | HEAD (86a23c6) |
>|------|------|------|
>|unittests|2|1|
>|java-25|6|1|
>|temurin|6|1|
>|lane-a|2|0|
>|integration|4|0|
>|integration2|2|0|
>|lane-b|2|0|
>|integration1|2|0|
>|unittests2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19533 +/- ##
============================================
- Coverage 67.72% 57.83% -9.89%
+ Complexity 1450 1 -1449
============================================
Files 3490 2685 -805
Lines 225032 164359 -60673
Branches 35527 26687 -8840
============================================
- Hits 152393 95051 -57342
- Misses 60612 61279 +667
+ Partials 12027 8029 -3998
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19533/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19533/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19533/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19533/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19533/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.83% <100.00%> (-9.89%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19533/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19533/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19533/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.83% <100.00%> (-9.89%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19533/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.83% <100.00%> (-9.89%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19533/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.83% <100.00%> (+0.01%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19533/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19533?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] Reuse fixed-row buffers for DataTable result construction [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19533:
URL: https://github.com/apache/pinot/pull/19533#issuecomment-5649841158
@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; `finishRow()` copies the bytes into the output stream. There is no dependency on clearing or resetting the buffer.
Addressed in b51ffd1e43467462d34001dcf424f35635d75156: the buffer is now allocated once in the constructor and is final, and `startRow()` only increments the row count. I also documented the complete-row contract and updated the regression test to populate every column, including reverse-order writes, empty variable-length values, explicit nulls, NaN payloads, and signed zero.
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 `NotNull` dependency is documented in the PR description, and the earlier benchmark results are explicitly labeled as belonging to the previous revision.
--
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
Re: [PR] Reuse fixed-row buffers for DataTable result construction [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #19533:
URL: https://github.com/apache/pinot/pull/19533#discussion_r4000925779
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datatable/BaseDataTableBuilder.java:
##########
@@ -65,6 +65,7 @@ public abstract class BaseDataTableBuilder implements DataTableBuilder {
protected final int _version;
protected final int[] _columnOffsets;
protected final int _rowSizeInBytes;
Review Comment:
(nit) This can be changed to a local field
--
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
Re: [PR] Reuse fixed-row buffers for DataTable result construction [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19533:
URL: https://github.com/apache/pinot/pull/19533#discussion_r4002691757
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datatable/BaseDataTableBuilder.java:
##########
@@ -65,6 +65,7 @@ public abstract class BaseDataTableBuilder implements DataTableBuilder {
protected final int _version;
protected final int[] _columnOffsets;
protected final int _rowSizeInBytes;
Review Comment:
Addressed in 4dd42c503588d0f944700d316c58a0b8036df567: removed `_rowSizeInBytes` from the instance fields and made `rowSizeInBytes` a constructor-local variable. The value is only needed to allocate the row buffer, and the sole builder subclass does not use it. All 10 `DataTableSerDeTest` cases and the required Spotless, Checkstyle, and license checks passed on JDK 25.
--
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