You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2026/04/11 09:44:05 UTC
[PR] Refactor ColumnStatistics: clean up interfaces, implementations, and improve test coverage [pinot]
Jackie-Jiang opened a new pull request, #18170:
URL: https://github.com/apache/pinot/pull/18170
## Summary
- Refactor `ColumnStatistics` interface: add Javadoc, default methods (`getValueType`, `isSingleValue`, `isFixedLength`, `isAscii`, `getMaxRowLengthInBytes`), move `MapColumnStatistics` to `Utf8Utils`
- Clean up stats collectors: rename `BytesColumnPredIndexStatsCollector` → `BytesColumnPreIndexStatsCollector`, extract `EmptyColumnStatistics` for empty columns, simplify `DefaultColumnStatistics` to derive all stats from `FieldSpec`
- Enhance `MutableForwardIndex` to track element lengths and ASCII flag, propagated through `MutableColumnStatistics` and `MutableNoDictColumnStatistics`
- Improve `CompactedColumnStatistics` and `CompactedNoDictColumnStatistics` to compute all stats (min/max, cardinality, element lengths, ASCII, sorted, row lengths) in a single pass over valid documents
- Significantly expand test coverage: add logical type tests (BOOLEAN, TIMESTAMP, JSON), MV tests for all types, assertion ordering aligned with `ColumnStatistics` API, `getLengthOfShortestElement` coverage
## Test plan
- [ ] All existing unit tests pass (`pinot-segment-local`, `pinot-segment-spi`)
- [ ] New tests: `EmptyColumnStatisticsTest`, `DefaultColumnStatisticsTest`, `MutableNoDictColumnStatisticsTest`
- [ ] Enhanced tests: `MutableColumnStatisticsTest`, `CompactedColumnStatisticsTest`, `CompactedNoDictColumnStatisticsTest`, `AbstractColumnStatisticsCollectorTest`, `NoDictColumnStatisticsCollectorTest`
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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] Refactor ColumnStatistics: clean up interface, add isAscii [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18170:
URL: https://github.com/apache/pinot/pull/18170#discussion_r3068799676
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/impl/dictionary/SameValueMutableDictionary.java:
##########
@@ -22,21 +22,24 @@
import java.io.IOException;
import java.math.BigDecimal;
import org.apache.pinot.segment.spi.index.mutable.MutableDictionary;
-import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.apache.pinot.spi.utils.Utf8Utils;
+
/**
* SameValueMutableDictionary is used to wrap any MutableDictionary, but store the same value. This is done to
* allow noRawDataForTextIndex config to work with mutable indexes.
*/
public class SameValueMutableDictionary implements MutableDictionary {
-
- private final Object _actualValue;
- private final Object[] _actualValues;
+ private final String _actualValue;
+ private final String[] _actualValues;
+ private final byte[] _valueBytes;
private final MutableDictionary _delegate;
- public SameValueMutableDictionary(Object value, MutableDictionary delegate) {
- _actualValue = value;
- _actualValues = new Object[]{value};
+ public SameValueMutableDictionary(Object actualValue, MutableDictionary delegate) {
+ _actualValue = actualValue.toString();
+ _actualValues = new String[]{_actualValue};
+ _valueBytes = Utf8Utils.encode(_actualValue);
Review Comment:
is this the right way to host bytes, e.g. digest bytes or uuid etc
--
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] Refactor ColumnStatistics: clean up interface, add isAscii [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18170:
URL: https://github.com/apache/pinot/pull/18170#issuecomment-4229271985
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18170?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 `23.78641%` with `314 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.29%. Comparing base ([`34d3fd6`](https://app.codecov.io/gh/apache/pinot/commit/34d3fd6f5bb23d161f65de638391c4875f684c23?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`bdcdf9a`](https://app.codecov.io/gh/apache/pinot/commit/bdcdf9a3b7e0f150bdfb81ecbd37f5447b90b4b2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18170?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...l/stats/ColumnarSegmentPreIndexStatsContainer.java](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Fcreator%2Fimpl%2Fstats%2FColumnarSegmentPreIndexStatsContainer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9zdGF0cy9Db2x1bW5hclNlZ21lbnRQcmVJbmRleFN0YXRzQ29udGFpbmVyLmphdmE=) | 0.00% | [35 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ltime/converter/stats/MutableColumnStatistics.java](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fconverter%2Fstats%2FMutableColumnStatistics.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9jb252ZXJ0ZXIvc3RhdHMvTXV0YWJsZUNvbHVtblN0YXRpc3RpY3MuamF2YQ==) | 0.00% | [34 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...or/impl/stats/NoDictColumnStatisticsCollector.java](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Fcreator%2Fimpl%2Fstats%2FNoDictColumnStatisticsCollector.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9zdGF0cy9Ob0RpY3RDb2x1bW5TdGF0aXN0aWNzQ29sbGVjdG9yLmphdmE=) | 34.04% | [20 Missing and 11 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ime/converter/stats/CompactedColumnStatistics.java](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fconverter%2Fstats%2FCompactedColumnStatistics.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9jb252ZXJ0ZXIvc3RhdHMvQ29tcGFjdGVkQ29sdW1uU3RhdGlzdGljcy5qYXZh) | 0.00% | [26 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...nverter/stats/CompactedNoDictColumnStatistics.java](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fconverter%2Fstats%2FCompactedNoDictColumnStatistics.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9jb252ZXJ0ZXIvc3RhdHMvQ29tcGFjdGVkTm9EaWN0Q29sdW1uU3RhdGlzdGljcy5qYXZh) | 0.00% | [26 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../loader/defaultcolumn/DefaultColumnStatistics.java](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Findex%2Floader%2Fdefaultcolumn%2FDefaultColumnStatistics.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2xvYWRlci9kZWZhdWx0Y29sdW1uL0RlZmF1bHRDb2x1bW5TdGF0aXN0aWNzLmphdmE=) | 44.68% | [22 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ealtime/impl/forward/CLPMutableForwardIndexV2.java](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fimpl%2Fforward%2FCLPMutableForwardIndexV2.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2ZvcndhcmQvQ0xQTXV0YWJsZUZvcndhcmRJbmRleFYyLmphdmE=) | 0.00% | [17 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...me/impl/dictionary/SameValueMutableDictionary.java](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fimpl%2Fdictionary%2FSameValueMutableDictionary.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvU2FtZVZhbHVlTXV0YWJsZURpY3Rpb25hcnkuamF2YQ==) | 0.00% | [15 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../realtime/impl/forward/CLPMutableForwardIndex.java](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fimpl%2Fforward%2FCLPMutableForwardIndex.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2ZvcndhcmQvQ0xQTXV0YWJsZUZvcndhcmRJbmRleC5qYXZh) | 0.00% | [12 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...converter/stats/MutableNoDictColumnStatistics.java](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frealtime%2Fconverter%2Fstats%2FMutableNoDictColumnStatistics.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9jb252ZXJ0ZXIvc3RhdHMvTXV0YWJsZU5vRGljdENvbHVtblN0YXRpc3RpY3MuamF2YQ==) | 0.00% | [11 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| ... and [17 more](https://app.codecov.io/gh/apache/pinot/pull/18170?src=pr&el=tree-more&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 (34d3fd6) and HEAD (bdcdf9a). Click for more details.
>
> <details><summary>HEAD has 20 uploads less than BASE</summary>
>
>| Flag | BASE (34d3fd6) | HEAD (bdcdf9a) |
>|------|------|------|
>|java-21|3|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|6|1|
>|java-11|3|0|
>|unittests2|2|0|
>|integration|2|0|
>|integration2|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18170 +/- ##
============================================
- Coverage 63.13% 55.29% -7.85%
+ Complexity 1610 826 -784
============================================
Files 3213 2520 -693
Lines 195730 144781 -50949
Branches 30240 23281 -6959
============================================
- Hits 123583 80054 -43529
+ Misses 62281 57820 -4461
+ Partials 9866 6907 -2959
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18170/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/18170/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/18170/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/18170/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/18170/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.29% <23.78%> (-7.81%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18170/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.29% <23.78%> (-7.85%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18170/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.29% <23.78%> (-7.85%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18170/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.29% <23.78%> (-0.09%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18170/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/18170?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] Refactor ColumnStatistics: clean up interface, add isAscii [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #18170:
URL: https://github.com/apache/pinot/pull/18170
--
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] Refactor ColumnStatistics: clean up interface, add isAscii [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18170:
URL: https://github.com/apache/pinot/pull/18170#discussion_r3068908436
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableColumnStatistics.java:
##########
@@ -100,14 +122,24 @@ private void collectElementLengthIfNeeded() {
_maxElementLength = length;
} else {
// If the stored type is not fixed width, iterate over the dictionary to find the min/max element length
+ // TODO: Collect these stats within Dictionary to avoid an extra scan
_minElementLength = Integer.MAX_VALUE;
_maxElementLength = 0;
+ boolean isAscii = valueType == DataType.STRING;
int length = _dictionary.length();
for (int i = 0; i < length; i++) {
- int elementLength = _dictionary.getValueSize(i);
- _minElementLength = Math.min(_minElementLength, elementLength);
- _maxElementLength = Math.max(_maxElementLength, elementLength);
+ if (isAscii) {
+ byte[] bytes = _dictionary.getBytesValue(i);
+ _minElementLength = Math.min(_minElementLength, bytes.length);
+ _maxElementLength = Math.max(_maxElementLength, bytes.length);
+ isAscii = Utf8Utils.isAscii(bytes);
+ } else {
+ int elementLength = _dictionary.getValueSize(i);
+ _minElementLength = Math.min(_minElementLength, elementLength);
+ _maxElementLength = Math.max(_maxElementLength, elementLength);
+ }
}
+ _isAscii = isAscii;
Review Comment:
I don't follow. Why is it only reflecting the last entry?
--
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] Refactor ColumnStatistics: clean up interface, add isAscii [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18170:
URL: https://github.com/apache/pinot/pull/18170#discussion_r3067864111
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/NoDictColumnStatisticsCollector.java:
##########
@@ -238,14 +263,7 @@ public void seal() {
}
private void updateHllPlus(Object value) {
- if (value instanceof BigDecimal) {
- // Canonicalize BigDecimal as string to avoid scale-related equality issues:
- // BigDecimals with different scales (e.g., 1.0 vs 1.00) are not equal by default,
- // but their string representations normalize the value for cardinality tracking.
- _hllPlus.offer(((BigDecimal) value).toString());
- } else {
- _hllPlus.offer(value);
- }
+ _hllPlus.offer(value);
}
Review Comment:
NoDictColumnStatisticsCollector.updateHllPlus() currently offers the raw value object to HLL++. For byte[] this hashes by identity (not content) and for BigDecimal it hashes with scale sensitivity, so once exact tracking is disabled (threshold exceeded) the approximate cardinality can become incorrect. Consider canonicalizing the offered value the same way as trackExactUnique() (e.g., wrap byte[] in ByteArray and offer BigDecimal.toString()/toPlainString()).
--
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] Refactor ColumnStatistics: clean up interface, add isAscii [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #18170:
URL: https://github.com/apache/pinot/pull/18170#issuecomment-4230503555
Can you add the motivation for adding `isAscii` to the interface and potential usage for this.
--
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] Refactor ColumnStatistics: clean up interface, add isAscii [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18170:
URL: https://github.com/apache/pinot/pull/18170#discussion_r3068977875
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableColumnStatistics.java:
##########
@@ -100,14 +122,24 @@ private void collectElementLengthIfNeeded() {
_maxElementLength = length;
} else {
// If the stored type is not fixed width, iterate over the dictionary to find the min/max element length
+ // TODO: Collect these stats within Dictionary to avoid an extra scan
_minElementLength = Integer.MAX_VALUE;
_maxElementLength = 0;
+ boolean isAscii = valueType == DataType.STRING;
int length = _dictionary.length();
for (int i = 0; i < length; i++) {
- int elementLength = _dictionary.getValueSize(i);
- _minElementLength = Math.min(_minElementLength, elementLength);
- _maxElementLength = Math.max(_maxElementLength, elementLength);
+ if (isAscii) {
+ byte[] bytes = _dictionary.getBytesValue(i);
+ _minElementLength = Math.min(_minElementLength, bytes.length);
+ _maxElementLength = Math.max(_maxElementLength, bytes.length);
+ isAscii = Utf8Utils.isAscii(bytes);
+ } else {
+ int elementLength = _dictionary.getValueSize(i);
+ _minElementLength = Math.min(_minElementLength, elementLength);
+ _maxElementLength = Math.max(_maxElementLength, elementLength);
+ }
}
+ _isAscii = isAscii;
Review Comment:
Ah, my bad.
isAscii is not set on the else branch :)
--
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] Refactor ColumnStatistics: clean up interface, add isAscii [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18170:
URL: https://github.com/apache/pinot/pull/18170#discussion_r3068907121
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/impl/dictionary/SameValueMutableDictionary.java:
##########
@@ -22,21 +22,24 @@
import java.io.IOException;
import java.math.BigDecimal;
import org.apache.pinot.segment.spi.index.mutable.MutableDictionary;
-import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.apache.pinot.spi.utils.Utf8Utils;
+
/**
* SameValueMutableDictionary is used to wrap any MutableDictionary, but store the same value. This is done to
* allow noRawDataForTextIndex config to work with mutable indexes.
*/
public class SameValueMutableDictionary implements MutableDictionary {
-
- private final Object _actualValue;
- private final Object[] _actualValues;
+ private final String _actualValue;
+ private final String[] _actualValues;
+ private final byte[] _valueBytes;
private final MutableDictionary _delegate;
- public SameValueMutableDictionary(Object value, MutableDictionary delegate) {
- _actualValue = value;
- _actualValues = new Object[]{value};
+ public SameValueMutableDictionary(Object actualValue, MutableDictionary delegate) {
+ _actualValue = actualValue.toString();
+ _actualValues = new String[]{_actualValue};
+ _valueBytes = Utf8Utils.encode(_actualValue);
Review Comment:
This is a hack for text index when user doesn't want to keep the raw values, where the value here can only be `STRING` type.
We should consider cleaning it 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
Re: [PR] Refactor ColumnStatistics: clean up interface, add isAscii [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18170:
URL: https://github.com/apache/pinot/pull/18170#discussion_r3068811966
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/converter/stats/MutableColumnStatistics.java:
##########
@@ -100,14 +122,24 @@ private void collectElementLengthIfNeeded() {
_maxElementLength = length;
} else {
// If the stored type is not fixed width, iterate over the dictionary to find the min/max element length
+ // TODO: Collect these stats within Dictionary to avoid an extra scan
_minElementLength = Integer.MAX_VALUE;
_maxElementLength = 0;
+ boolean isAscii = valueType == DataType.STRING;
int length = _dictionary.length();
for (int i = 0; i < length; i++) {
- int elementLength = _dictionary.getValueSize(i);
- _minElementLength = Math.min(_minElementLength, elementLength);
- _maxElementLength = Math.max(_maxElementLength, elementLength);
+ if (isAscii) {
+ byte[] bytes = _dictionary.getBytesValue(i);
+ _minElementLength = Math.min(_minElementLength, bytes.length);
+ _maxElementLength = Math.max(_maxElementLength, bytes.length);
+ isAscii = Utf8Utils.isAscii(bytes);
+ } else {
+ int elementLength = _dictionary.getValueSize(i);
+ _minElementLength = Math.min(_minElementLength, elementLength);
+ _maxElementLength = Math.max(_maxElementLength, elementLength);
+ }
}
+ _isAscii = isAscii;
Review Comment:
This _isAscii always reflect the last entry of dictionary, is it expected behavior?
--
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] Refactor ColumnStatistics: clean up interface, add isAscii [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18170:
URL: https://github.com/apache/pinot/pull/18170#discussion_r3070480939
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/EmptyColumnStatistics.java:
##########
@@ -0,0 +1,121 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.creator.impl.stats;
+
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.spi.creator.ColumnStatistics;
+import org.apache.pinot.segment.spi.partition.PartitionFunction;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+
+
+/// Column statistics for an empty column (zero rows).
+@SuppressWarnings("rawtypes")
+public class EmptyColumnStatistics implements ColumnStatistics {
+ private final FieldSpec _fieldSpec;
+
+ public EmptyColumnStatistics(FieldSpec fieldSpec) {
+ _fieldSpec = fieldSpec;
+ }
+
+ @Override
+ public FieldSpec getFieldSpec() {
+ return _fieldSpec;
+ }
+
+ @Nullable
+ @Override
+ public Comparable getMinValue() {
+ return null;
+ }
+
+ @Nullable
+ @Override
+ public Comparable getMaxValue() {
+ return null;
+ }
+
+ @Nullable
+ @Override
+ public Object getUniqueValuesSet() {
+ return null;
+ }
+
+ @Override
+ public int getCardinality() {
+ return 0;
+ }
+
+ @Override
+ public int getLengthOfShortestElement() {
+ DataType valueType = getValueType();
+ return valueType.isFixedWidth() ? valueType.size() : 0;
+ }
+
+ @Override
+ public int getLengthOfLongestElement() {
+ DataType valueType = getValueType();
+ return valueType.isFixedWidth() ? valueType.size() : 0;
+ }
+
+ @Override
+ public boolean isSorted() {
+ return isSingleValue();
+ }
+
+ @Override
+ public int getTotalNumberOfEntries() {
+ return 0;
+ }
+
+ @Override
+ public int getMaxNumberOfMultiValues() {
+ return 0;
+ }
+
+ @Override
+ public int getMaxRowLengthInBytes() {
+ return 0;
+ }
+
+ @Nullable
+ @Override
+ public PartitionFunction getPartitionFunction() {
Review Comment:
Good point. Ideally empty segment should be handled separately, but keeping the partition function for defensive purpose
--
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] Refactor ColumnStatistics: clean up interface, add isAscii [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #18170:
URL: https://github.com/apache/pinot/pull/18170#issuecomment-4230657560
> Can you add the motivation for adding `isAscii` to the interface and potential usage for this.
Will wire it in in the following PR. When a string column is ASCII only, the read/comparison can be simplified
--
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] Refactor ColumnStatistics: clean up interface, add isAscii [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18170:
URL: https://github.com/apache/pinot/pull/18170#discussion_r3068392085
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/NoDictColumnStatisticsCollector.java:
##########
@@ -238,14 +263,7 @@ public void seal() {
}
private void updateHllPlus(Object value) {
- if (value instanceof BigDecimal) {
- // Canonicalize BigDecimal as string to avoid scale-related equality issues:
- // BigDecimals with different scales (e.g., 1.0 vs 1.00) are not equal by default,
- // but their string representations normalize the value for cardinality tracking.
- _hllPlus.offer(((BigDecimal) value).toString());
- } else {
- _hllPlus.offer(value);
- }
+ _hllPlus.offer(value);
}
Review Comment:
The internal implementation is doing the same
--
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] Refactor ColumnStatistics: clean up interface, add isAscii [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18170:
URL: https://github.com/apache/pinot/pull/18170#discussion_r3068907121
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/impl/dictionary/SameValueMutableDictionary.java:
##########
@@ -22,21 +22,24 @@
import java.io.IOException;
import java.math.BigDecimal;
import org.apache.pinot.segment.spi.index.mutable.MutableDictionary;
-import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.apache.pinot.spi.utils.Utf8Utils;
+
/**
* SameValueMutableDictionary is used to wrap any MutableDictionary, but store the same value. This is done to
* allow noRawDataForTextIndex config to work with mutable indexes.
*/
public class SameValueMutableDictionary implements MutableDictionary {
-
- private final Object _actualValue;
- private final Object[] _actualValues;
+ private final String _actualValue;
+ private final String[] _actualValues;
+ private final byte[] _valueBytes;
private final MutableDictionary _delegate;
- public SameValueMutableDictionary(Object value, MutableDictionary delegate) {
- _actualValue = value;
- _actualValues = new Object[]{value};
+ public SameValueMutableDictionary(Object actualValue, MutableDictionary delegate) {
+ _actualValue = actualValue.toString();
+ _actualValues = new String[]{_actualValue};
+ _valueBytes = Utf8Utils.encode(_actualValue);
Review Comment:
This class is a hack for text index when user doesn't want to keep the raw values, where the value here can only be `STRING` type.
We should consider cleaning it 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
Re: [PR] Refactor ColumnStatistics: clean up interface, add isAscii [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18170:
URL: https://github.com/apache/pinot/pull/18170#discussion_r3068907121
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/impl/dictionary/SameValueMutableDictionary.java:
##########
@@ -22,21 +22,24 @@
import java.io.IOException;
import java.math.BigDecimal;
import org.apache.pinot.segment.spi.index.mutable.MutableDictionary;
-import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.apache.pinot.spi.utils.Utf8Utils;
+
/**
* SameValueMutableDictionary is used to wrap any MutableDictionary, but store the same value. This is done to
* allow noRawDataForTextIndex config to work with mutable indexes.
*/
public class SameValueMutableDictionary implements MutableDictionary {
-
- private final Object _actualValue;
- private final Object[] _actualValues;
+ private final String _actualValue;
+ private final String[] _actualValues;
+ private final byte[] _valueBytes;
private final MutableDictionary _delegate;
- public SameValueMutableDictionary(Object value, MutableDictionary delegate) {
- _actualValue = value;
- _actualValues = new Object[]{value};
+ public SameValueMutableDictionary(Object actualValue, MutableDictionary delegate) {
+ _actualValue = actualValue.toString();
+ _actualValues = new String[]{_actualValue};
+ _valueBytes = Utf8Utils.encode(_actualValue);
Review Comment:
This is a hack for text index, where value can only be `STRING` type.
We should consider cleaning it 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
Re: [PR] Refactor ColumnStatistics: clean up interface, add isAscii [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18170:
URL: https://github.com/apache/pinot/pull/18170#discussion_r3069004847
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/EmptyColumnStatistics.java:
##########
@@ -0,0 +1,121 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.creator.impl.stats;
+
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.spi.creator.ColumnStatistics;
+import org.apache.pinot.segment.spi.partition.PartitionFunction;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+
+
+/// Column statistics for an empty column (zero rows).
+@SuppressWarnings("rawtypes")
+public class EmptyColumnStatistics implements ColumnStatistics {
+ private final FieldSpec _fieldSpec;
+
+ public EmptyColumnStatistics(FieldSpec fieldSpec) {
+ _fieldSpec = fieldSpec;
+ }
+
+ @Override
+ public FieldSpec getFieldSpec() {
+ return _fieldSpec;
+ }
+
+ @Nullable
+ @Override
+ public Comparable getMinValue() {
+ return null;
+ }
+
+ @Nullable
+ @Override
+ public Comparable getMaxValue() {
+ return null;
+ }
+
+ @Nullable
+ @Override
+ public Object getUniqueValuesSet() {
+ return null;
+ }
+
+ @Override
+ public int getCardinality() {
+ return 0;
+ }
+
+ @Override
+ public int getLengthOfShortestElement() {
+ DataType valueType = getValueType();
+ return valueType.isFixedWidth() ? valueType.size() : 0;
+ }
+
+ @Override
+ public int getLengthOfLongestElement() {
+ DataType valueType = getValueType();
+ return valueType.isFixedWidth() ? valueType.size() : 0;
+ }
+
+ @Override
+ public boolean isSorted() {
+ return isSingleValue();
+ }
+
+ @Override
+ public int getTotalNumberOfEntries() {
+ return 0;
+ }
+
+ @Override
+ public int getMaxNumberOfMultiValues() {
+ return 0;
+ }
+
+ @Override
+ public int getMaxRowLengthInBytes() {
+ return 0;
+ }
+
+ @Nullable
+ @Override
+ public PartitionFunction getPartitionFunction() {
Review Comment:
do we need to keep the partition info?
Will this cause regression for segment pruning or table rebalance?
--
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