You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "9aman (via GitHub)" <gi...@apache.org> on 2025/12/17 07:06:42 UTC
[PR] Fixing ColumnReader interface to return validity bitset in case of multi-value primitive type [pinot]
9aman opened a new pull request, #17387:
URL: https://github.com/apache/pinot/pull/17387
(no comment)
--
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] Fixing ColumnReader interface to return validity bitset in case of multi-value primitive type [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #17387:
URL: https://github.com/apache/pinot/pull/17387#discussion_r2625973591
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentColumnReaderImpl.java:
##########
@@ -200,43 +201,43 @@ public byte[] nextBytes() {
}
@Override
- public int[] nextIntMV() {
+ public MultiValueResult<int[]> nextIntMV() {
if (!hasNext()) {
throw new IllegalStateException("No more values available");
}
int[] value = _segmentColumnReader.getIntMV(_currentIndex);
_currentIndex++;
- return value;
+ return MultiValueResult.of(value, null);
Review Comment:
lets document that pinot doesn't support nulls in MV and thus we're passing null to validity vector
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentColumnReaderImplTest.java:
##########
@@ -359,22 +360,23 @@ public void testSingleValueAccessors(String columnName, SingleValueGetter random
@DataProvider(name = "multiValueAccessorProvider")
public Object[][] multiValueAccessorProvider() {
// Base column configurations
+ // For primitive MV types, we need to extract .getValues() from MultiValueResult
Review Comment:
lets add assertions that hasNulls() returns false
--
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] Fixing ColumnReader interface to return validity bitset in case of multi-value primitive type [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #17387:
URL: https://github.com/apache/pinot/pull/17387#discussion_r2634371045
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/MultiValueResult.java:
##########
@@ -0,0 +1,123 @@
+/**
+ * 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.spi.data.readers;
+
+import java.util.BitSet;
+import javax.annotation.Nullable;
+
+
+/**
+ * Result wrapper for multi-value column reads that tracks element-level null validity.
+ *
+ * <p>This class addresses a limitation where bulk reads from columnar formats (like Arrow)
+ * don't check the validity bitmap for null elements. By returning both the values array
+ * and a validity BitSet, callers can properly handle null elements within multi-value columns.
+ *
+ * <p><b>BitSet Semantics:</b>
+ * <ul>
+ * <li>Set bit (1) = valid/non-null element</li>
+ * <li>Unset bit (0) = null element</li>
+ * <li>Null validity BitSet = no nulls in the range (fast path)</li>
+ * </ul>
+ *
+ * <p><b>Usage Example:</b>
+ * <pre>{@code
+ * MultiValueResult<int[]> result = columnReader.nextIntMV();
+ * int[] values = result.getValues();
+ *
+ * if (result.hasNulls()) {
+ * for (int i = 0; i < values.length; i++) {
+ * if (result.isNull(i)) {
+ * // Handle null element - values[i] contains default value (0 for int)
+ * } else {
+ * // Use values[i]
+ * }
+ * }
+ * } else {
+ * // Fast path: no nulls, use values array directly
+ * }
+ * }</pre>
+ *
+ * @param <T> The array type (int[], long[], float[], double[])
+ */
+public class MultiValueResult<T> {
+ private final T _values;
+ @Nullable
+ private final BitSet _validity;
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] Fixing ColumnReader interface to return validity bitset in case of multi-value primitive type [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #17387:
URL: https://github.com/apache/pinot/pull/17387#discussion_r2632467648
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/MultiValueResult.java:
##########
@@ -0,0 +1,123 @@
+/**
+ * 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.spi.data.readers;
+
+import java.util.BitSet;
+import javax.annotation.Nullable;
+
+
+/**
+ * Result wrapper for multi-value column reads that tracks element-level null validity.
+ *
+ * <p>This class addresses a limitation where bulk reads from columnar formats (like Arrow)
+ * don't check the validity bitmap for null elements. By returning both the values array
+ * and a validity BitSet, callers can properly handle null elements within multi-value columns.
+ *
+ * <p><b>BitSet Semantics:</b>
+ * <ul>
+ * <li>Set bit (1) = valid/non-null element</li>
+ * <li>Unset bit (0) = null element</li>
+ * <li>Null validity BitSet = no nulls in the range (fast path)</li>
+ * </ul>
+ *
+ * <p><b>Usage Example:</b>
+ * <pre>{@code
+ * MultiValueResult<int[]> result = columnReader.nextIntMV();
+ * int[] values = result.getValues();
+ *
+ * if (result.hasNulls()) {
+ * for (int i = 0; i < values.length; i++) {
+ * if (result.isNull(i)) {
+ * // Handle null element - values[i] contains default value (0 for int)
+ * } else {
+ * // Use values[i]
+ * }
+ * }
+ * } else {
+ * // Fast path: no nulls, use values array directly
+ * }
+ * }</pre>
+ *
+ * @param <T> The array type (int[], long[], float[], double[])
+ */
+public class MultiValueResult<T> {
+ private final T _values;
+ @Nullable
+ private final BitSet _validity;
Review Comment:
Consider naming it `nulls` for clarity
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/readers/ColumnReader.java:
##########
@@ -57,7 +57,7 @@
* } catch (Exception e) {
* // Handle exception / log
* continue;
-* }
+ * }
Review Comment:
(nit) Fix indentation
--
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] Fixing ColumnReader interface to return validity bitset in case of multi-value primitive type [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #17387:
URL: https://github.com/apache/pinot/pull/17387#discussion_r2630606600
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentColumnReaderImplTest.java:
##########
@@ -359,22 +360,23 @@ public void testSingleValueAccessors(String columnName, SingleValueGetter random
@DataProvider(name = "multiValueAccessorProvider")
public Object[][] multiValueAccessorProvider() {
// Base column configurations
+ // For primitive MV types, we need to extract .getValues() from MultiValueResult
Review Comment:
Added
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentColumnReaderImpl.java:
##########
@@ -200,43 +201,43 @@ public byte[] nextBytes() {
}
@Override
- public int[] nextIntMV() {
+ public MultiValueResult<int[]> nextIntMV() {
if (!hasNext()) {
throw new IllegalStateException("No more values available");
}
int[] value = _segmentColumnReader.getIntMV(_currentIndex);
_currentIndex++;
- return value;
+ return MultiValueResult.of(value, null);
Review Comment:
Documented.
--
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] Fixing ColumnReader interface to return validity bitset in case of multi-value primitive type [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17387:
URL: https://github.com/apache/pinot/pull/17387#issuecomment-3664122796
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17387?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 `25 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.60%. Comparing base ([`0ae7f87`](https://app.codecov.io/gh/apache/pinot/commit/0ae7f87552b022f7d8612bf459d4e1d157e78e27?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7198319`](https://app.codecov.io/gh/apache/pinot/commit/71983191911db4e58efc3e0a445f1c1f1a460e8a?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/17387?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...pache/pinot/spi/data/readers/MultiValueResult.java](https://app.codecov.io/gh/apache/pinot/pull/17387?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fdata%2Freaders%2FMultiValueResult.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9yZWFkZXJzL011bHRpVmFsdWVSZXN1bHQuamF2YQ==) | 0.00% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17387?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ocal/segment/readers/DefaultValueColumnReader.java](https://app.codecov.io/gh/apache/pinot/pull/17387?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Freaders%2FDefaultValueColumnReader.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3JlYWRlcnMvRGVmYXVsdFZhbHVlQ29sdW1uUmVhZGVyLmphdmE=) | 0.00% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17387?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../segment/readers/PinotSegmentColumnReaderImpl.java](https://app.codecov.io/gh/apache/pinot/pull/17387?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Freaders%2FPinotSegmentColumnReaderImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3JlYWRlcnMvUGlub3RTZWdtZW50Q29sdW1uUmVhZGVySW1wbC5qYXZh) | 0.00% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17387?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 (0ae7f87) and HEAD (7198319). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (0ae7f87) | HEAD (7198319) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|0|
>|unittests2|2|0|
>|integration|6|0|
>|integration2|2|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17387 +/- ##
============================================
- Coverage 63.22% 55.60% -7.63%
+ Complexity 1474 702 -772
============================================
Files 3147 2460 -687
Lines 187575 138865 -48710
Branches 28712 22139 -6573
============================================
- Hits 118601 77210 -41391
+ Misses 59771 55139 -4632
+ Partials 9203 6516 -2687
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17387/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/17387/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/17387/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/17387/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/17387/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/17387/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/17387/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.60% <0.00%> (-7.58%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17387/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.60% <0.00%> (-7.63%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17387/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.60% <0.00%> (-7.63%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17387/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.60% <0.00%> (-0.03%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17387/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/17387?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] Fixing ColumnReader interface to return validity bitset in case of multi-value primitive type [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #17387:
URL: https://github.com/apache/pinot/pull/17387#issuecomment-3670736929
I thought we didn't support nulls inside not-null multivalues. Adding @Jackie-Jiang here for him to verify.
I think this feature is correct, but I'm afraid this may end up producing bugs later at query time
--
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] Fixing ColumnReader interface to return validity bitset in case of multi-value primitive type [pinot]
Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on PR #17387:
URL: https://github.com/apache/pinot/pull/17387#issuecomment-3670885882
> I thought we didn't support nulls inside not-null multivalues. Adding @Jackie-Jiang here for him to verify.
>
> I think this feature is correct, but I'm afraid this may end up producing bugs later at query time
Yes. This PR just enables the interface so that the clients of these reader can identify if there is a null or not. This isn't applicable for pinot column readers because pinot MV won't have nulls. But other readers like parquet readers can return nulls. So returning a bitset vector can enable the clients to then filter the nulls before indexing data
--
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] Fixing ColumnReader interface to return validity bitset in case of multi-value primitive type [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #17387:
URL: https://github.com/apache/pinot/pull/17387
--
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