You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "abhioncbr (via GitHub)" <gi...@apache.org> on 2023/09/22 02:20:42 UTC
[GitHub] [pinot] abhioncbr opened a new pull request, #11653: ScalarTransformFunctionWrapper NULL support
abhioncbr opened a new pull request, #11653:
URL: https://github.com/apache/pinot/pull/11653
As per the [issue](https://github.com/apache/pinot/issues/11548)
--
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] Added calls for BaseTransformation init function for NULL support [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #11653:
URL: https://github.com/apache/pinot/pull/11653
--
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
[GitHub] [pinot] abhioncbr commented on a diff in pull request #11653: ScalarTransformFunctionWrapper NULL support
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #11653:
URL: https://github.com/apache/pinot/pull/11653#discussion_r1341862027
##########
pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/ScalarTransformFunctionWrapperTest.java:
##########
@@ -262,6 +262,27 @@ public void testIsNullOperator() {
testTransformFunction(transformFunction, expectedValues);
}
+ @Test
+ public void testCaseWhenTransformFunctionNullColumn() {
Review Comment:
Added the test for `casewhen` null column, and it's working fine. However, I am not sure
why do we have `casewhen` in ObjectFunctions when we already have a separate implementation? In what scenario will it get used?
--
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
[GitHub] [pinot] shenyu0127 commented on a diff in pull request #11653: ScalarTransformFunctionWrapper NULL support
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #11653:
URL: https://github.com/apache/pinot/pull/11653#discussion_r1339518401
##########
pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunctionTest.java:
##########
@@ -237,6 +239,26 @@ private void testCaseQueryWithStringResults(String predicate, String[] expectedV
testTransformFunction(transformFunction, expectedValues);
}
+ @Test
+ public void testCaseTransformationWithNullColumn() {
Review Comment:
This test exercises the `CaseTransformFunction`. We want to fix the `ScalarTransformFunctionWrapper` and add a test to exercise the `ScalarTransformFunctionWrapper` that wraps `ObjectFunctions::caseWhen`.
##########
pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/BaseTransformFunctionTest.java:
##########
@@ -580,9 +580,14 @@ protected void testTransformFunction(TransformFunction transformFunction, String
protected void testTransformFunctionWithNull(TransformFunction transformFunction, String[] expectedValues,
RoaringBitmap expectedNulls) {
+ testTransformFunctionWithNull(transformFunction, expectedValues, expectedNulls, false);
+ }
+
+ protected void testTransformFunctionWithNull(TransformFunction transformFunction, String[] expectedValues,
Review Comment:
We don't need to define this function because we can use the `testTransformFunction`.
--
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] Added calls for BaseTransformation init function for NULL support [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11653:
URL: https://github.com/apache/pinot/pull/11653#issuecomment-1743730121
I'd suggest 2 follow ups after this PR:
1. Add null support for ScalarTransformFunctionWrapper
2. Add test for all transform functions on nullable columns
--
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
[GitHub] [pinot] abhioncbr commented on pull request #11653: Added calls for BaseTransformation init function for NULL support
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #11653:
URL: https://github.com/apache/pinot/pull/11653#issuecomment-1741888203
> This is not the correct way to support null in `ScalarTransformFunctionWrapper` because for scalar function, the result might not be `null` when one of the input values is `null`. It needs to extends `ComputeDifferentlyWhenNullHandlingEnabledTransformFunction` and handle `null` differently.
>
> We may keep the change in this PR as it adds default null support to more functions, and make a separate PR for `ScalarTransformFunctionWrapper`
Thanks for mentioning. I updated the PR title per the changes and will raise another PR for `ScalarTransformFunctionWrapper`. Please review the PR once more.
--
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
[GitHub] [pinot] abhioncbr commented on a diff in pull request #11653: ScalarTransformFunctionWrapper NULL support
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #11653:
URL: https://github.com/apache/pinot/pull/11653#discussion_r1341865991
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java:
##########
@@ -297,6 +297,7 @@ public DataType toDataType() {
case TIMESTAMP:
case TIMESTAMP_ARRAY:
return DataType.TIMESTAMP;
+ case OBJECT:
Review Comment:
This is added to confirm the test. This shouldn't be here.
Question: to determine the `TransformResultMetadata _resultMetadata` in `ScalarTransformFunctionWrapper` for `Object` resultType; what should we do? Like in `CaseTransformFunction` we have [calculateResultMetadata](https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java#L102), do we have to do similar?
--
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
[GitHub] [pinot] shenyu0127 commented on a diff in pull request #11653: Added calls for BaseTransformation init function for NULL support
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #11653:
URL: https://github.com/apache/pinot/pull/11653#discussion_r1342052930
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/ScalarTransformFunctionWrapper.java:
##########
@@ -444,4 +446,10 @@ private void getNonLiteralValues(ValueBlock valueBlock) {
}
}
}
+
+ @Nullable
Review Comment:
We don't need to define this function because we can inherit the function from the subclass.
##########
pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunctionTest.java:
##########
@@ -237,6 +239,26 @@ private void testCaseQueryWithStringResults(String predicate, String[] expectedV
testTransformFunction(transformFunction, expectedValues);
}
+ @Test
+ public void testCaseTransformationWithNullColumn() {
Review Comment:
If your PR is "Added calls for BaseTransformation init function for NULL support", suggest to revert this irrelevant test.
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #11653: ScalarTransformFunctionWrapper NULL support
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11653:
URL: https://github.com/apache/pinot/pull/11653#issuecomment-1730737331
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11653?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11653](https://app.codecov.io/gh/apache/pinot/pull/11653?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (48c5938) into [master](https://app.codecov.io/gh/apache/pinot/commit/3501b869c7ece2ddd088daf34c68a15ad6740a3a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (3501b86) will **decrease** coverage by `48.66%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11653 +/- ##
=============================================
- Coverage 63.06% 14.40% -48.66%
+ Complexity 1118 201 -917
=============================================
Files 2342 2342
Lines 125690 125701 +11
Branches 19306 19318 +12
=============================================
- Hits 79262 18107 -61155
- Misses 40778 106068 +65290
+ Partials 5650 1526 -4124
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11653/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/11653/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/11653/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/11653/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/11653/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-17](https://app.codecov.io/gh/apache/pinot/pull/11653/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.40% <0.00%> (-48.52%)` | :arrow_down: |
| [java-20](https://app.codecov.io/gh/apache/pinot/pull/11653/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11653/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.40% <0.00%> (-48.66%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/11653/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.40% <0.00%> (-48.66%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11653/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/11653/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.40% <0.00%> (+<0.01%)` | :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.
| [Files Changed](https://app.codecov.io/gh/apache/pinot/pull/11653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...unction/BaseBooleanAssertionTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/11653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQmFzZUJvb2xlYW5Bc3NlcnRpb25UcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-79.17%)` | :arrow_down: |
| [...ator/transform/function/BaseTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/11653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQmFzZVRyYW5zZm9ybUZ1bmN0aW9uLmphdmE=) | `0.00% <ø> (-71.40%)` | :arrow_down: |
| [...transform/function/CLPDecodeTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/11653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQ0xQRGVjb2RlVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-94.12%)` | :arrow_down: |
| [.../function/DateTimeConversionTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/11653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vRGF0ZVRpbWVDb252ZXJzaW9uVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-77.50%)` | :arrow_down: |
| [...transform/function/DateTruncTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/11653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vRGF0ZVRydW5jVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-90.33%)` | :arrow_down: |
| [...r/transform/function/ExtractTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/11653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vRXh0cmFjdFRyYW5zZm9ybUZ1bmN0aW9uLmphdmE=) | `0.00% <0.00%> (-68.75%)` | :arrow_down: |
| [...or/transform/function/GroovyTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/11653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vR3Jvb3Z5VHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-71.84%)` | :arrow_down: |
| [...r/transform/function/InIdSetTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/11653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vSW5JZFNldFRyYW5zZm9ybUZ1bmN0aW9uLmphdmE=) | `0.00% <0.00%> (-38.10%)` | :arrow_down: |
| [...erator/transform/function/InTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/11653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vSW5UcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-61.67%)` | :arrow_down: |
| [...or/transform/function/IsNullTransformFunction.java](https://app.codecov.io/gh/apache/pinot/pull/11653?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vSXNOdWxsVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-93.34%)` | :arrow_down: |
| ... and [8 more](https://app.codecov.io/gh/apache/pinot/pull/11653?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
... and [1497 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11653/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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
[GitHub] [pinot] shenyu0127 commented on pull request #11653: ScalarTransformFunctionWrapper NULL support
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on PR #11653:
URL: https://github.com/apache/pinot/pull/11653#issuecomment-1732155459
The https://github.com/apache/pinot/issues/11548 is not asking for changes in this PR but asking for making `ScalarTransformFunctionWrapper::getNullBitmap` return correct results. This means for the query `SELECT CASE WHEN col IS NULL THEN 1 ELSE 2 END`, if the col is NULL, it should return 1. Currently it would incorrectly 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