You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "shenyu0127 (via GitHub)" <gi...@apache.org> on 2023/04/13 17:43:50 UTC
[GitHub] [pinot] shenyu0127 opened a new pull request, #10604: Supports null for IN and NOT IN clauses.
shenyu0127 opened a new pull request, #10604:
URL: https://github.com/apache/pinot/pull/10604
Supports null for IN and NOT IN clauses.
Tested: unit tests.
--
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] 61yao commented on a diff in pull request #10604: Support null in InTransformFunction and NotInTransformFunction.
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10604:
URL: https://github.com/apache/pinot/pull/10604#discussion_r1171945687
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -199,6 +210,8 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
}
}
break;
+ case UNKNOWN:
+ break;
Review Comment:
Maybe put default null value in _intValuesSV to be consistent across calls
--
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] 61yao commented on a diff in pull request #10604: Support null in InTransformFunction and NotInTransformFunction.
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10604:
URL: https://github.com/apache/pinot/pull/10604#discussion_r1166287581
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -365,11 +380,56 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
}
}
break;
+ case UNKNOWN:
+ break;
default:
throw new IllegalStateException();
}
}
return _intValuesSV;
}
+
Review Comment:
Can we also implement transformToInValuesSVWithNull for perf reason?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -365,11 +380,56 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
}
}
break;
+ case UNKNOWN:
+ break;
default:
throw new IllegalStateException();
}
}
return _intValuesSV;
}
+
+ @Nullable
+ @Override
+ public RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
+ int length = valueBlock.getNumDocs();
+ RoaringBitmap result = new RoaringBitmap();
+ RoaringBitmap mainFunctionNullBitmap = _mainFunction.getNullBitmap(valueBlock);
+ if (mainFunctionNullBitmap != null) {
+ result.or(mainFunctionNullBitmap);
+ }
Review Comment:
Return early if nullbitmap cardinality is equal to num of docs?
--
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 #10604: Support null in InTransformFunction and NotInTransformFunction.
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on PR #10604:
URL: https://github.com/apache/pinot/pull/10604#issuecomment-1509113652
> Can we add to comments and PR's description that when we expect null from these functions?
>
> Let's also mention the design doc and related github issues Null support issue: #10252
>
> Design doc: https://docs.google.com/document/d/1w2eBuZrS73xE2JIw8MYlySy5pNaX9iE8kZr_YAL20CM/edit
Done. (Added the issue in the PR description, which links to the design doc.)
--
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 #10604: Support null in InTransformFunction and NotInTransformFunction.
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #10604:
URL: https://github.com/apache/pinot/pull/10604#discussion_r1167204131
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -365,11 +380,56 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
}
}
break;
+ case UNKNOWN:
+ break;
default:
throw new IllegalStateException();
}
}
return _intValuesSV;
}
+
Review Comment:
I suggest not to implement `transformToInValuesSVWithNull`.
- Implementing something like [`BaseTransformFunction.transformToInValuesSVWithNull`](https://github.com/apache/pinot/blob/13a792f03f267bf53e36fc06340d43d8fc1d7af4/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BaseTransformFunction.java#L184) violates [DRY](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself).
- There wouldn't be much performance gain because `BaseTransformFunction.transformToInValuesSVWithNull` still gets int[] and bitmap separately.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -365,11 +380,56 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
}
}
break;
+ case UNKNOWN:
+ break;
default:
throw new IllegalStateException();
}
}
return _intValuesSV;
}
+
+ @Nullable
+ @Override
+ public RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
+ int length = valueBlock.getNumDocs();
+ RoaringBitmap result = new RoaringBitmap();
+ RoaringBitmap mainFunctionNullBitmap = _mainFunction.getNullBitmap(valueBlock);
+ if (mainFunctionNullBitmap != null) {
+ result.or(mainFunctionNullBitmap);
+ }
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
[GitHub] [pinot] 61yao commented on a diff in pull request #10604: Support null in InTransformFunction and NotInTransformFunction.
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10604:
URL: https://github.com/apache/pinot/pull/10604#discussion_r1172794619
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -199,6 +210,8 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
}
}
break;
+ case UNKNOWN:
+ break;
Review Comment:
If we enable null in data ingestion/storage, it is possible client will also call the query without null enabled query option. and the query should give consistent results. We decided not to crush the query per discussion with @Jackie-Jiang
--
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] Jackie-Jiang merged pull request #10604: Support null in InTransformFunction and NotInTransformFunction.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10604:
URL: https://github.com/apache/pinot/pull/10604
--
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 #10604: Support null in InTransformFunction and NotInTransformFunction.
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #10604:
URL: https://github.com/apache/pinot/pull/10604#discussion_r1167308201
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -365,11 +380,56 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
}
}
break;
+ case UNKNOWN:
+ break;
default:
throw new IllegalStateException();
}
}
return _intValuesSV;
}
+
Review Comment:
Had an offline discussion with @61yao
- There is performance improvement if we override `transformToInValuesSVWithNull` because
- Some subclass's `transformToInValuesSVWithNull` is faster because they can compute the values and null map together (e.g. CASE WHEN).
- `BaseTransformFunction.transformToInValuesSVWithNull` calls `transformToInValuesSVWithNull`, which can be a subclass (polymorphism) and is faster (explained in the above sub-point).
- The `InTransformFunction.transformToInValuesSV` is long, if we override `transformToInValuesSVWithNull` with a similar implementation, the repeated code decreases maintainability. Added a TODO and we will override after [refactoring](https://github.com/apache/pinot/issues/10618).
--
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] 61yao commented on pull request #10604: Support null in InTransformFunction and NotInTransformFunction.
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on PR #10604:
URL: https://github.com/apache/pinot/pull/10604#issuecomment-1507956471
Can we add to comments and PR's description that when we expect null from these functions?
Let's also mention the design doc and related github issues
Null support issue:
https://github.com/apache/pinot/issues/10252
Design doc:
https://docs.google.com/document/d/1w2eBuZrS73xE2JIw8MYlySy5pNaX9iE8kZr_YAL20CM/edit
--
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 #10604: Support null in InTransformFunction and NotInTransformFunction.
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #10604:
URL: https://github.com/apache/pinot/pull/10604#discussion_r1172093471
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -199,6 +210,8 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
}
}
break;
+ case UNKNOWN:
+ break;
Review Comment:
We don't have the problem of inconsistent results.
We can enable/disable null in two places:
- Data ingestion/storage
- Query option
If we disable null in data ingestion/storage, there is no inconsistency problem in the results between null enabled and disabled in the query option. If we enable null in data ingestion/storage, we should only allow the null enabled query option.
--
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] 61yao commented on a diff in pull request #10604: Support null in InTransformFunction and NotInTransformFunction.
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10604:
URL: https://github.com/apache/pinot/pull/10604#discussion_r1174175796
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -365,11 +380,56 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
}
}
break;
+ case UNKNOWN:
+ break;
default:
throw new IllegalStateException();
}
}
return _intValuesSV;
}
+
Review Comment:
The problem without implementing transformToXXWithNull is that we cannot fill in the default values for all nulls.
For example, if the value is later evaluated null in getNullBitmap call, the intValue field is not updated.
I think it is fine now because filling default value rule is not strictly followed in default implementation. We can change this later when default implementation is fixed.
Do you mind putting a TODO or a comment noting that the value for null is undefined/indeterministic if the transformToInt API is called?
--
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] 61yao commented on a diff in pull request #10604: Support null in InTransformFunction and NotInTransformFunction.
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10604:
URL: https://github.com/apache/pinot/pull/10604#discussion_r1171978071
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -199,6 +210,8 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
}
}
break;
+ case UNKNOWN:
+ break;
Review Comment:
My point is to make the result consistent when enableNullHandling is set to false. In that call, we don't get the nullbitmap.
--
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 #10604: Supports null for IN and NOT IN clauses.
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10604:
URL: https://github.com/apache/pinot/pull/10604#issuecomment-1507466424
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10604?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10604](https://codecov.io/gh/apache/pinot/pull/10604?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0f10e5f) into [master](https://codecov.io/gh/apache/pinot/commit/0cb456a3b03805dbba5b7b34db7c0d877c08c58d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0cb456a) will **decrease** coverage by `7.03%`.
> The diff coverage is `94.28%`.
```diff
@@ Coverage Diff @@
## master #10604 +/- ##
============================================
- Coverage 70.37% 63.34% -7.03%
+ Complexity 6495 6092 -403
============================================
Files 2106 2090 -16
Lines 113004 112607 -397
Branches 17026 16982 -44
============================================
- Hits 79521 71335 -8186
- Misses 27917 36052 +8135
+ Partials 5566 5220 -346
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.53% <0.00%> (+<0.01%)` | :arrow_up: |
| integration2 | `23.95% <0.00%> (-0.06%)` | :arrow_down: |
| unittests1 | `68.00% <94.28%> (+0.02%)` | :arrow_up: |
| unittests2 | `?` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10604?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...tor/transform/function/NotInTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10604?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vTm90SW5UcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `88.88% <0.00%> (-11.12%)` | :arrow_down: |
| [...erator/transform/function/InTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10604?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vSW5UcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `64.91% <97.05%> (+8.37%)` | :arrow_up: |
... and [303 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10604/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
: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=The+Apache+Software+Foundation)
--
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 #10604: Support null in InTransformFunction and NotInTransformFunction.
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #10604:
URL: https://github.com/apache/pinot/pull/10604#discussion_r1167308201
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -365,11 +380,56 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
}
}
break;
+ case UNKNOWN:
+ break;
default:
throw new IllegalStateException();
}
}
return _intValuesSV;
}
+
Review Comment:
Had an offline discussion with @61yao
- There is performance improvement if we override `transformToInValuesSVWithNull` because
- Some subclass's `transformToInValuesSVWithNull` is faster because they can compute the values and null map together (e.g. CASE WHEN).
- `BaseTransformFunction.transformToInValuesSVWithNull` may call a non-integer "WithNull" function, which can be a subclass implementation and is faster.
- The `InTransformFunction.transformToInValuesSV` is long, if we override `transformToInValuesSVWithNull` with a similar implementation, the repeated code decreases maintainability. Added a TODO and we will override after [refactoring](https://github.com/apache/pinot/issues/10618).
--
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 #10604: Support null in InTransformFunction and NotInTransformFunction.
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #10604:
URL: https://github.com/apache/pinot/pull/10604#discussion_r1172093471
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -199,6 +210,8 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
}
}
break;
+ case UNKNOWN:
+ break;
Review Comment:
We don't have the problem of inconsistent results.
We can enable/disable null in two places:
- Data ingestion/storage
- Query option
If we disable null in data ingestion/storage, the null enabled and null disabled query options generate consistent results. If we enable null in data ingestion/storage, we should only allow the null enabled query option.
--
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] 61yao commented on a diff in pull request #10604: Support null in InTransformFunction and NotInTransformFunction.
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10604:
URL: https://github.com/apache/pinot/pull/10604#discussion_r1167254855
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -365,11 +380,56 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
}
}
break;
+ case UNKNOWN:
+ break;
default:
throw new IllegalStateException();
}
}
return _intValuesSV;
}
+
Review Comment:
Performance matters more here than DRY. :)
Overriding transformToInValuesSVWithNull will avoid calling the base function's transformToInValuesSVWithNull and you can call _mainFunction.transformToInValuesSVWithNull to get the values and null bit map at the same 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
[GitHub] [pinot] shenyu0127 commented on a diff in pull request #10604: Support null in InTransformFunction and NotInTransformFunction.
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #10604:
URL: https://github.com/apache/pinot/pull/10604#discussion_r1173170691
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -199,6 +210,8 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
}
}
break;
+ case UNKNOWN:
+ break;
Review Comment:
Modified the code to put default null value in `_intValuesSV`.
Had an offline discussion with @61yao:
In theory if we enable null in data ingestion/storage and the client makes a query without the null enabled query option, we should fail the query. In practice we don't fail the query.
--
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 #10604: Support null in InTransformFunction and NotInTransformFunction.
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #10604:
URL: https://github.com/apache/pinot/pull/10604#discussion_r1171958095
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -199,6 +210,8 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock) {
}
}
break;
+ case UNKNOWN:
+ break;
Review Comment:
If the value is null, we don't care about the values in _intValuesSV so we don't need to set the values.
So we want to keep here as is and remove the code elsewhere that sets the default null value in _intValuesSV ([e.g.](https://github.com/apache/pinot/blob/60ee5eb0e8a2cf8276565ae8b20fb81292315843/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BaseTransformFunction.java#L173)) and the related unit tests ([e.g.](https://github.com/apache/pinot/blob/d50f9ee8b04f615abc57eab4c08a53ad4902f72a/pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/ScalarTransformFunctionWrapperTest.java#L883))? I'd be happy to do the cleanup in a different PR.
--
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