You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2026/03/12 11:20:36 UTC
[PR] Enhance JSON extraction functions to support null handling in queries [pinot]
gortiz opened a new pull request, #17867:
URL: https://github.com/apache/pinot/pull/17867
This pull request enhances the `JsonExtractScalarTransformFunction` to provide better support for handling `null` default values, especially when Pinot's null handling feature is enabled. It introduces logic to correctly propagate nulls when a `null` default is specified, and adds comprehensive tests to validate this behavior.
**Enhancements to null handling in JSON extraction:**
* Added a `_defaultIsNull` flag and logic to detect when the default value for `jsonExtractScalar` is explicitly set to `null`, and to handle this case differently depending on whether null handling is enabled (`_nullHandlingEnabled`). [[1]](diffhunk://#diff-d46a454e5b8d36c10e064b4a33c2cf26e5921a3ef71c8fbf75b1dc2a9dd7bf86R77) [[2]](diffhunk://#diff-d46a454e5b8d36c10e064b4a33c2cf26e5921a3ef71c8fbf75b1dc2a9dd7bf86L113-R151)
* Overrode the `getNullBitmap` method to ensure rows with a `null` result are correctly marked as null when a `null` default is used and null handling is enabled.
* Updated the `init` method signature to accept the `nullHandlingEnabled` parameter, aligning with the new null handling logic.
**Testing improvements:**
* Added new test records with `null` values in the JSON field to the test dataset.
* Introduced two new tests to verify the behavior of `jsonExtractScalar` when the default value is `null`, both with null handling enabled and disabled, ensuring correct output in each scenario.
**Miscellaneous:**
* Added the `@Language("sql")` annotation to improve static analysis of SQL queries in tests.
* Imported `@Nullable` and `RoaringBitmap` to support the new null handling logic.
--
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] Enhance JSON extraction functions to support null handling in queries [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #17867:
URL: https://github.com/apache/pinot/pull/17867
--
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] Enhance JSON extraction functions to support null handling in queries [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17867:
URL: https://github.com/apache/pinot/pull/17867#issuecomment-4046327169
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17867?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 `42 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 34.29%. Comparing base ([`54bea86`](https://app.codecov.io/gh/apache/pinot/commit/54bea86f89c082981f839622fc70b211aa073a5f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`3d890f2`](https://app.codecov.io/gh/apache/pinot/commit/3d890f2bb31876f9866948969d8b6a0060f84543?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:white_check_mark: All tests successful. No failed tests found.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17867?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...m/function/JsonExtractScalarTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/17867?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Ftransform%2Ffunction%2FJsonExtractScalarTransformFunction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vSnNvbkV4dHJhY3RTY2FsYXJUcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | 0.00% | [42 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17867?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 (54bea86) and HEAD (3d890f2). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (54bea86) | HEAD (3d890f2) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|0|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|0|
>|unittests2|2|1|
>|integration|6|0|
>|integration1|2|0|
>|integration2|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17867 +/- ##
=============================================
- Coverage 63.26% 34.29% -28.97%
+ Complexity 1466 739 -727
=============================================
Files 3190 3190
Lines 192039 192077 +38
Branches 29421 29428 +7
=============================================
- Hits 121484 65868 -55616
- Misses 61042 120678 +59636
+ Partials 9513 5531 -3982
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17867/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/17867/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/17867/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/17867/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/17867/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/17867/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/17867/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.29% <0.00%> (-28.95%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17867/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.29% <0.00%> (-28.97%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17867/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.29% <0.00%> (-28.97%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17867/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17867/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.29% <0.00%> (+0.04%)` | :arrow_up: |
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/17867?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] Enhance JSON extraction functions to support null handling in queries [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17867:
URL: https://github.com/apache/pinot/pull/17867#discussion_r2923973114
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java:
##########
@@ -120,6 +158,37 @@ public TransformResultMetadata getResultMetadata() {
return _resultMetadata;
}
+ @Override
+ public @Nullable RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
+ if (!_defaultIsNull) {
+ return super.getNullBitmap(valueBlock);
+ }
+ RoaringBitmap bitmap = new RoaringBitmap();
+ for (TransformFunction arg : _arguments.subList(1, _arguments.size() - 1)) {
+ RoaringBitmap argBitmap = arg.getNullBitmap(valueBlock);
+ if (argBitmap != null) {
+ bitmap.or(argBitmap);
+ }
+ }
+ int numDocs = valueBlock.getNumDocs();
+ RoaringBitmap nullBitmap = new RoaringBitmap();
+ IntFunction<Object> resultExtractor = getResultExtractor(valueBlock);
+ for (int i = 0; i < numDocs; i++) {
+ Object result = null;
+ try {
+ result = resultExtractor.apply(i);
+ } catch (Exception ignored) {
+ }
+ if (result == null) {
+ nullBitmap.add(i);
+ }
+ }
+ if (!nullBitmap.isEmpty()) {
+ bitmap.or(nullBitmap);
+ }
Review Comment:
getNullBitmap() always returns a RoaringBitmap instance even when it is empty. Per the TransformFunction contract (and BaseTransformFunction behavior), it should return null when there are no null rows to avoid extra allocations and to preserve the semantic meaning of a null return value.
```suggestion
}
if (bitmap.isEmpty()) {
return null;
}
```
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java:
##########
@@ -110,7 +114,41 @@ public void init(List<TransformFunction> arguments, Map<String, ColumnContext> c
+ "/DOUBLE_ARRAY/STRING_ARRAY", resultsType));
}
if (arguments.size() == 4) {
- _defaultValue = dataType.convert(((LiteralTransformFunction) arguments.get(3)).getStringLiteral());
+ LiteralTransformFunction literalTransformFun = (LiteralTransformFunction) arguments.get(3);
+ _defaultIsNull = literalTransformFun.isNull() && _nullHandlingEnabled;
+ switch (dataType) {
+ case INT:
+ _defaultValue = literalTransformFun.getIntLiteral();
+ break;
+ case LONG:
+ _defaultValue = literalTransformFun.getLongLiteral();
+ break;
+ case FLOAT:
+ _defaultValue = literalTransformFun.getFloatLiteral();
+ break;
+ case DOUBLE:
+ case TIMESTAMP:
+ _defaultValue = literalTransformFun.getDoubleLiteral();
+ break;
Review Comment:
Default value parsing for TIMESTAMP uses getDoubleLiteral(), which can lose precision for large epoch values and also regresses support for string timestamp literals that previously worked via DataType.TIMESTAMP.convert(...). Consider using the long/timestamp literal accessor (or the existing DataType.TIMESTAMP.convert on the string literal) so TIMESTAMP defaults are handled consistently with other Pinot timestamp parsing rules.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java:
##########
@@ -38,6 +38,8 @@
import org.apache.pinot.core.util.NumericException;
import org.apache.pinot.spi.data.FieldSpec.DataType;
import org.apache.pinot.spi.utils.JsonUtils;
+import org.jspecify.annotations.Nullable;
+import org.roaringbitmap.RoaringBitmap;
Review Comment:
This file introduces org.jspecify.annotations.Nullable, but TransformFunction/BaseTransformFunction (and most Pinot core code) use javax.annotation.Nullable. Using a different `@Nullable` annotation here makes nullness annotations inconsistent and can reduce tooling effectiveness; consider switching to javax.annotation.Nullable for the override and removing the jspecify import.
--
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] Enhance JSON extraction functions to support null handling in queries [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #17867:
URL: https://github.com/apache/pinot/pull/17867#discussion_r2954476402
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java:
##########
@@ -120,6 +158,37 @@ public TransformResultMetadata getResultMetadata() {
return _resultMetadata;
}
+ @Override
+ public @Nullable RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
+ if (!_defaultIsNull) {
+ return super.getNullBitmap(valueBlock);
+ }
+ RoaringBitmap bitmap = new RoaringBitmap();
+ for (TransformFunction arg : _arguments.subList(1, _arguments.size() - 1)) {
+ RoaringBitmap argBitmap = arg.getNullBitmap(valueBlock);
+ if (argBitmap != null) {
+ bitmap.or(argBitmap);
+ }
+ }
+ int numDocs = valueBlock.getNumDocs();
+ RoaringBitmap nullBitmap = new RoaringBitmap();
+ IntFunction<Object> resultExtractor = getResultExtractor(valueBlock);
+ for (int i = 0; i < numDocs; i++) {
+ Object result = null;
+ try {
+ result = resultExtractor.apply(i);
+ } catch (Exception ignored) {
+ }
+ if (result == null) {
+ nullBitmap.add(i);
+ }
+ }
+ if (!nullBitmap.isEmpty()) {
+ bitmap.or(nullBitmap);
+ }
Review Comment:
Yes, I didn't find a better way to do that.
--
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] Enhance JSON extraction functions to support null handling in queries [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #17867:
URL: https://github.com/apache/pinot/pull/17867#discussion_r2954473692
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java:
##########
@@ -38,6 +38,8 @@
import org.apache.pinot.core.util.NumericException;
import org.apache.pinot.spi.data.FieldSpec.DataType;
import org.apache.pinot.spi.utils.JsonUtils;
+import org.jspecify.annotations.Nullable;
+import org.roaringbitmap.RoaringBitmap;
Review Comment:
That is true. jspecify is the more modern version of javax.annotation, but we should not change the one we use
--
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] Enhance JSON extraction functions to support null handling in queries [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #17867:
URL: https://github.com/apache/pinot/pull/17867#discussion_r2929054120
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java:
##########
@@ -38,6 +38,8 @@
import org.apache.pinot.core.util.NumericException;
import org.apache.pinot.spi.data.FieldSpec.DataType;
import org.apache.pinot.spi.utils.JsonUtils;
+import org.jspecify.annotations.Nullable;
+import org.roaringbitmap.RoaringBitmap;
Review Comment:
+1, don't we use `javax.annotation.Nullable` everywhere else in the project?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java:
##########
@@ -120,6 +158,37 @@ public TransformResultMetadata getResultMetadata() {
return _resultMetadata;
}
+ @Override
+ public @Nullable RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
+ if (!_defaultIsNull) {
+ return super.getNullBitmap(valueBlock);
+ }
+ RoaringBitmap bitmap = new RoaringBitmap();
+ for (TransformFunction arg : _arguments.subList(1, _arguments.size() - 1)) {
+ RoaringBitmap argBitmap = arg.getNullBitmap(valueBlock);
+ if (argBitmap != null) {
+ bitmap.or(argBitmap);
+ }
+ }
+ int numDocs = valueBlock.getNumDocs();
+ RoaringBitmap nullBitmap = new RoaringBitmap();
+ IntFunction<Object> resultExtractor = getResultExtractor(valueBlock);
+ for (int i = 0; i < numDocs; i++) {
+ Object result = null;
+ try {
+ result = resultExtractor.apply(i);
+ } catch (Exception ignored) {
+ }
+ if (result == null) {
+ nullBitmap.add(i);
+ }
+ }
+ if (!nullBitmap.isEmpty()) {
+ bitmap.or(nullBitmap);
+ }
Review Comment:
This seems pretty expensive, we're computing the whole result set again?
--
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