You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "yashmayya (via GitHub)" <gi...@apache.org> on 2024/07/09 11:52:25 UTC
[PR] Enable null handling by default for leaf stages in the multi-stage query engine [pinot]
yashmayya opened a new pull request, #13570:
URL: https://github.com/apache/pinot/pull/13570
- Without null handling enabled in the leaf stages, we get results that don't adhere to the SQL standard for various aggregations. For instance, the `MAX` aggregate function uses `Double.NEGATIVE_INFINITY` as its default value when null handling is not enabled. The SQL standard states that if no row qualifies, the result of any aggregate function (other than `COUNT`) is the `null` value.
- With null handling enabled, the various aggregation operators do return `null` if no doc is matched. This is due to the use of an object based aggregation result holder rather than a primitive based one, for instance here - https://github.com/apache/pinot/blob/efa43007adc1dd7736580d882f33956e359b0678/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/MaxAggregationFunction.java#L57-L60
- This patch enables null handling by default for leaf stages in the multi-stage query engine. Note that users can still explicitly override this to disable null handling if desired.
--
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] Enable null handling by default for leaf stages in the multi-stage query engine [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #13570:
URL: https://github.com/apache/pinot/pull/13570#issuecomment-2217578713
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/13570?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 61.97%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`534ac81`)](https://app.codecov.io/gh/apache/pinot/commit/534ac81b319b616b5fb94375f192a9719be6ffa4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 731 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #13570 +/- ##
============================================
+ Coverage 61.75% 61.97% +0.22%
- Complexity 207 1296 +1089
============================================
Files 2436 2558 +122
Lines 133233 140921 +7688
Branches 20636 21867 +1231
============================================
+ Hits 82274 87335 +5061
- Misses 44911 46963 +2052
- Partials 6048 6623 +575
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/13570/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/13570/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/13570/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/13570/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/13570/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/13570/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/13570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.97% <100.00%> (+0.34%)` | :arrow_up: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/13570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.96% <100.00%> (+0.21%)` | :arrow_up: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/13570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.95% <100.00%> (+34.22%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/13570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.97% <100.00%> (+0.22%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/13570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.97% <100.00%> (+0.22%)` | :arrow_up: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/13570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.51% <100.00%> (-0.38%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/13570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.65% <0.00%> (-0.08%)` | :arrow_down: |
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/13570?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).
--
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] Enable null handling by default for leaf stages in the multi-stage query engine [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya closed pull request #13570: Enable null handling by default for leaf stages in the multi-stage query engine
URL: https://github.com/apache/pinot/pull/13570
--
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] Enable null handling by default for leaf stages in the multi-stage query engine [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #13570:
URL: https://github.com/apache/pinot/pull/13570#issuecomment-2221207984
> Ideally we only want to enable null handling when there are nullable fields. Do you think that is doable?
I don't think that is ideal. In fact cases like this say otherwise. Even if fields are not nullable, a `max(notNullableField) where 1 = 0` should return null.
--
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] Enable null handling by default for leaf stages in the multi-stage query engine [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #13570:
URL: https://github.com/apache/pinot/pull/13570#issuecomment-2217926010
@Jackie-Jiang what do you think?
--
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] Enable null handling by default for leaf stages in the multi-stage query engine [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on PR #13570:
URL: https://github.com/apache/pinot/pull/13570#issuecomment-2218216578
Though semantically this is the right thing to do, this will change the behavior of existing queries.
Can we clearly define what our long term approach for this is with the Multistage Engine? One strategy could be that since we still have a way to stick with the existing behavior by setting the query option to false, this is a valid change for a minor release and users should refer to the release-notes for caveats before upgrading their Pinot version.
Also, is there a Wiki page which describes what the current null handling behavior is? I think this one doesn't cover the Multistage Engine quirks completely: https://docs.pinot.apache.org/developers/advanced/null-value-support#examples-queries
I am also happy to contribute to the Wiki and/or the policy for query behavior changes.
--
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] Enable null handling by default for leaf stages in the multi-stage query engine [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on PR #13570:
URL: https://github.com/apache/pinot/pull/13570#issuecomment-2218264276
> Can we clearly define what our long term approach for this is with the Multistage Engine
I think the long term goal is to make the v2 query engine SQL compliant and this is one small step towards that. I agree that we should ideally avoid making such default behavior changes and hopefully future releases will have no such surprises once we achieve full compliance.
> One strategy could be that since we still have a way to stick with the existing behavior by setting the query option to false, this is a valid change for a minor release and users should refer to the release-notes for caveats before upgrading their Pinot version.
Yeah, I think that and also the fact that this patch is targeting only the v2 multi-stage query engine (which is still fairly new and not as widely adopted) makes the change justifiable.
> Also, is there a Wiki page which describes what the current null handling behavior is? I think this one doesn't cover the Multistage Engine quirks completely
For aggregations specifically, I was planning to update this page - https://docs.pinot.apache.org/users/user-guide-query/query-syntax/supported-aggregations if this PR is merged. I agree that we should also update https://docs.pinot.apache.org/developers/advanced/null-value-support.
--
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