You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "tarun11Mavani (via GitHub)" <gi...@apache.org> on 2025/09/08 07:59:43 UTC
[PR] Add support for commit time compaction with columnMajorBuild [pinot]
tarun11Mavani opened a new pull request, #16769:
URL: https://github.com/apache/pinot/pull/16769
Extension of: #16344
### Summary
This PR extends the Commit Time Compaction (#14588) feature to support column-major segment building, removing the previous limitation that restricted commit-time compaction to row-major builds only. This enhancement allows users to leverage commit-time compaction for storage efficiency and column-major builds for improved build time.
### Technical Implementation
RealtimeSegmentConverter: Enhanced to pass validDocIdsSnapshot to the column-major build process when using compacted readers, ensuring only valid documents are processed during column indexing.
SegmentColumnarIndexCreator.indexColumn(): Extended method signature to accept validDocIds parameter, enabling selective indexing of only valid documents during column-major builds. Maintains backward compatibility with deprecated method signature.
SegmentIndexCreationDriverImpl.buildByColumn(): Updated to accept and utilize validDocIds parameter, passing it through to the column indexing process for filtering invalid documents
TableConfigUtils: Removed validateCommitTimeCompactionConfig() method and associated validation logic that previously blocked column-major builds with commit-time compaction
Enhanced Integration Testing:
- Extended CommitTimeCompactionIntegrationTest to test three table configurations simultaneously:
- Baseline (no compaction, row-major)
- Commit-time compaction with row-major build
- Commit-time compaction with column-major build
- Improved test methods to handle three-table comparisons and validation
### Impact
**Feature Compatibility**: Users can now enable both enableCommitTimeCompaction=true and columnMajorSegmentBuilderEnabled=true simultaneously
**Performance Benefits**: Combines storage optimization from commit-time compaction with performance improvements from column-major indexing
**Backward Compatibility**: All existing functionality remains intact; changes are additive
### Testing
Integration Tests: Extended existing test suite to validate three-table scenarios (baseline, row-major compaction, column-major compaction)
Regression Testing in test cluster: Created tables with and without commit time compaction with column major build enabled for both tables.
- Both table has same data and segment counts
- 3x Segment build time improved with commit time compaction enabled for table with 12x compaction ratio, 40K messages per second, 90 to 120 minute commit frequency
--
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] Add support for commit time compaction with columnMajorBuild [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16769:
URL: https://github.com/apache/pinot/pull/16769#issuecomment-3278468890
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16769?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `17 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.49%. Comparing base ([`a60438a`](https://app.codecov.io/gh/apache/pinot/commit/a60438aa9ccc81e9d0abdb9e5c1df72721232ccb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`286d3e0`](https://app.codecov.io/gh/apache/pinot/commit/286d3e07005b18f10cb10e8ca83bedd2c459d4ea?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 15 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/16769?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ment/creator/impl/SegmentColumnarIndexCreator.java](https://app.codecov.io/gh/apache/pinot/pull/16769?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Fcreator%2Fimpl%2FSegmentColumnarIndexCreator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50Q29sdW1uYXJJbmRleENyZWF0b3IuamF2YQ==) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16769?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...t/creator/impl/SegmentIndexCreationDriverImpl.java](https://app.codecov.io/gh/apache/pinot/pull/16769?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Fcreator%2Fimpl%2FSegmentIndexCreationDriverImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50SW5kZXhDcmVhdGlvbkRyaXZlckltcGwuamF2YQ==) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16769?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ache/pinot/segment/spi/creator/SegmentCreator.java](https://app.codecov.io/gh/apache/pinot/pull/16769?src=pr&el=tree&filepath=pinot-segment-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Fspi%2Fcreator%2FSegmentCreator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2NyZWF0b3IvU2VnbWVudENyZWF0b3IuamF2YQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16769?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...l/realtime/converter/RealtimeSegmentConverter.java](https://app.codecov.io/gh/apache/pinot/pull/16769?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fconverter%2FRealtimeSegmentConverter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9jb252ZXJ0ZXIvUmVhbHRpbWVTZWdtZW50Q29udmVydGVyLmphdmE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16769?src=pr&el=tree&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 (a60438a) and HEAD (286d3e0). Click for more details.
>
> <details><summary>HEAD has 32 uploads less than BASE</summary>
>
>| Flag | BASE (a60438a) | HEAD (286d3e0) |
>|------|------|------|
>|unittests2|2|0|
>|unittests|4|1|
>|java-21|4|1|
>|temurin|9|1|
>|unittests1|2|1|
>|java-11|5|0|
>|integration|5|0|
>|integration1|2|0|
>|custom-integration1|2|0|
>|integration2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16769 +/- ##
============================================
- Coverage 63.47% 56.49% -6.98%
+ Complexity 1399 700 -699
============================================
Files 3054 2395 -659
Lines 178684 133090 -45594
Branches 27381 21169 -6212
============================================
- Hits 113412 75185 -38227
+ Misses 56558 51650 -4908
+ Partials 8714 6255 -2459
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16769/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/16769/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/16769/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/16769/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/16769/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16769/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/16769/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.49% <0.00%> (-6.95%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16769/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.49% <0.00%> (-6.98%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16769/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.49% <0.00%> (-6.98%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16769/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.49% <0.00%> (-0.07%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16769/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 Sentry](https://app.codecov.io/gh/apache/pinot/pull/16769?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] Add support for commit time compaction with columnMajorBuild [pinot]
Posted by "rohityadav1993 (via GitHub)" <gi...@apache.org>.
rohityadav1993 commented on code in PR #16769:
URL: https://github.com/apache/pinot/pull/16769#discussion_r2331024800
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/RecordReader.java:
##########
@@ -76,4 +76,15 @@ GenericRow next(GenericRow reuse)
*/
void rewind()
throws IOException;
+
+ /**
+ * Get the sorted document IDs if available.
+ * This method is used to retrieve the sorted order of document IDs for segment creation.
+ *
+ * @return array of sorted document IDs, or null if not available or not applicable
+ */
+ @Nullable
+ default int[] getSortedDocIds() {
Review Comment:
Is this method needed in interface? We use this specifically in segment conversion and might not be applicable in general to recordReader interface.
--
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] Add support for commit time compaction with columnMajorBuild [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on code in PR #16769:
URL: https://github.com/apache/pinot/pull/16769#discussion_r2338687719
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/SegmentCreator.java:
##########
@@ -67,7 +68,22 @@ void indexRow(GenericRow row)
* @param sortedDocIds - If not null, then this provides the sorted order of documents.
* @param segment - Used to get the values of the column.
*/
- void indexColumn(String columnName, @Nullable int[] sortedDocIds, IndexSegment segment)
+ @Deprecated
Review Comment:
sg.
--
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] Add support for commit time compaction with columnMajorBuild [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on code in PR #16769:
URL: https://github.com/apache/pinot/pull/16769#discussion_r2338689477
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/RecordReader.java:
##########
@@ -76,4 +76,15 @@ GenericRow next(GenericRow reuse)
*/
void rewind()
throws IOException;
+
+ /**
+ * Get the sorted document IDs if available.
+ * This method is used to retrieve the sorted order of document IDs for segment creation.
+ *
+ * @return array of sorted document IDs, or null if not available or not applicable
+ */
+ @Nullable
+ default int[] getSortedDocIds() {
Review Comment:
removed.
--
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] Add support for commit time compaction with columnMajorBuild [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #16769:
URL: https://github.com/apache/pinot/pull/16769#discussion_r2337121567
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/RecordReader.java:
##########
@@ -76,4 +76,15 @@ GenericRow next(GenericRow reuse)
*/
void rewind()
throws IOException;
+
+ /**
+ * Get the sorted document IDs if available.
+ * This method is used to retrieve the sorted order of document IDs for segment creation.
+ *
+ * @return array of sorted document IDs, or null if not available or not applicable
+ */
+ @Nullable
+ default int[] getSortedDocIds() {
Review Comment:
Yeah semantically I am not sure what this means here. Better to remove
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -356,7 +357,8 @@ public void indexRow(GenericRow row)
}
@Override
- public void indexColumn(String columnName, @Nullable int[] sortedDocIds, IndexSegment segment)
+ public void indexColumn(String columnName, @Nullable int[] sortedDocIds, IndexSegment segment,
Review Comment:
nit: can you add a javadoc to explain the role of validDocIds. (and the behavior when it is null)
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/SegmentCreator.java:
##########
@@ -67,7 +68,22 @@ void indexRow(GenericRow row)
* @param sortedDocIds - If not null, then this provides the sorted order of documents.
* @param segment - Used to get the values of the column.
*/
- void indexColumn(String columnName, @Nullable int[] sortedDocIds, IndexSegment segment)
+ @Deprecated
Review Comment:
I don't think we need to Deprecate this method.
--
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] Add support for commit time compaction with columnMajorBuild [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on code in PR #16769:
URL: https://github.com/apache/pinot/pull/16769#discussion_r2338686075
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -356,7 +357,8 @@ public void indexRow(GenericRow row)
}
@Override
- public void indexColumn(String columnName, @Nullable int[] sortedDocIds, IndexSegment segment)
+ public void indexColumn(String columnName, @Nullable int[] sortedDocIds, IndexSegment segment,
Review Comment:
done
--
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] Add support for commit time compaction with columnMajorBuild [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana merged PR #16769:
URL: https://github.com/apache/pinot/pull/16769
--
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