You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "61yao (via GitHub)" <gi...@apache.org> on 2023/04/14 04:11:43 UTC
[GitHub] [pinot] 61yao opened a new pull request, #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
61yao opened a new pull request, #10613:
URL: https://github.com/apache/pinot/pull/10613
Pass the null bitmap up from transform function to transform blockValueSet.
We only do the null related calculation when enableNullHandling is set to true due to performance reason.
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 #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #10613:
URL: https://github.com/apache/pinot/pull/10613#discussion_r1201357156
##########
pinot-core/src/main/java/org/apache/pinot/core/common/RowBasedBlockValueFetcher.java:
##########
Review Comment:
optional: file an issue to create a unit test file for this file as well as TransformBlock and TransformBlockValSet.
--
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] KKcorps commented on a diff in pull request #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #10613:
URL: https://github.com/apache/pinot/pull/10613#discussion_r1181438028
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CoalesceTransformFunction.java:
##########
@@ -399,4 +390,22 @@ public String[] transformToStringValuesSV(ValueBlock valueBlock) {
}
return getStringTransformResults(valueBlock);
}
+
+ @Override
+ public RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
+ RoaringBitmap[] nullBitMaps = getNullBitMaps(valueBlock, _transformFunctions);
+ RoaringBitmap bitmap = nullBitMaps[0];
+ for (int i = 1; i < nullBitMaps.length; i++) {
+ RoaringBitmap curBitmap = nullBitMaps[i];
+ if (bitmap != null && curBitmap != null) {
+ bitmap.and(curBitmap);
+ } else {
+ bitmap = null;
Review Comment:
we can break the loop when setting bitmap to 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
[GitHub] [pinot] KKcorps commented on a diff in pull request #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #10613:
URL: https://github.com/apache/pinot/pull/10613#discussion_r1181444884
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -56,6 +56,8 @@ public String getName() {
@Override
public void init(List<TransformFunction> arguments, Map<String, ColumnContext> columnContextMap) {
+ // TODO: Proper support for null in groovy functions.
Review Comment:
Can you elaborate this TODO a bit more. What's missing to provide proper 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
[GitHub] [pinot] shenyu0127 commented on pull request #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on PR #10613:
URL: https://github.com/apache/pinot/pull/10613#issuecomment-1595475333
@61yao will not continue working on this PR; I will patch this PR and finish it.
--
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 #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #10613:
URL: https://github.com/apache/pinot/pull/10613#discussion_r1222324918
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/docvalsets/TransformBlockValSet.java:
##########
@@ -46,47 +45,23 @@ public class TransformBlockValSet implements BlockValSet {
private final TransformFunction _transformFunction;
private final ExpressionContext _expression;
- private boolean _nullBitmapSet;
- private RoaringBitmap _nullBitmap;
+ private RoaringBitmap _nullBitmap = null;
private int[] _numMVEntries;
+ private final boolean _isNullHandlingEnabled;
+
public TransformBlockValSet(ValueBlock valueBlock, TransformFunction transformFunction,
- ExpressionContext expression) {
+ ExpressionContext expression, boolean isNullHandlingEnabled) {
_valueBlock = valueBlock;
_transformFunction = transformFunction;
_expression = expression;
+ _isNullHandlingEnabled = isNullHandlingEnabled;
}
@Nullable
@Override
public RoaringBitmap getNullBitmap() {
Review Comment:
We may call `getNullBitmap` before `getXXValuesSV`. [Here](https://github.com/apache/pinot/blob/01ff18d47caeaee6e31b09b1b6486f756eff0db1/pinot-core/src/main/java/org/apache/pinot/core/operator/query/SelectionOnlyOperator.java#L108 ) is an example.
Do we want to implement `getNullBitmap` by having a `_nullBitmapSet` and calling the corresponding `getXXValuesSV` to initialize the `_nullBitmap` on demand?
--
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 #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10613:
URL: https://github.com/apache/pinot/pull/10613#discussion_r1220938885
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/docvalsets/TransformBlockValSet.java:
##########
@@ -46,47 +45,23 @@ public class TransformBlockValSet implements BlockValSet {
private final TransformFunction _transformFunction;
private final ExpressionContext _expression;
- private boolean _nullBitmapSet;
- private RoaringBitmap _nullBitmap;
+ private RoaringBitmap _nullBitmap = null;
private int[] _numMVEntries;
+ private final boolean _isNullHandlingEnabled;
+
public TransformBlockValSet(ValueBlock valueBlock, TransformFunction transformFunction,
- ExpressionContext expression) {
+ ExpressionContext expression, boolean isNullHandlingEnabled) {
_valueBlock = valueBlock;
_transformFunction = transformFunction;
_expression = expression;
+ _isNullHandlingEnabled = isNullHandlingEnabled;
}
@Nullable
@Override
public RoaringBitmap getNullBitmap() {
Review Comment:
We control the call order. so getNullBitmap won't be called before getXXValuesSV.
This is not an ideal API design. but in Pinot, there are a lot of call dependencies like this. So I was thinking this is not a big deal.
--
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 #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #10613:
URL: https://github.com/apache/pinot/pull/10613#discussion_r1201343304
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java:
##########
@@ -299,4 +296,189 @@ public void testNullLiteralSelectionOnlyBroker()
assertEquals(rows.size(), 1);
assertEquals(rows.get(0).get(0).asText(), "null");
}
+
+ @Test
+ public void testNullSelectionOnlyTransform()
Review Comment:
nit: break down the test into multiple tests so that we test behaviors separately.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java:
##########
@@ -904,7 +904,7 @@ public Pair<byte[][], RoaringBitmap> transformToBytesValuesSVWithNull(ValueBlock
final RoaringBitmap bitmap = new RoaringBitmap();
int[] selected = getSelectedArray(valueBlock, true);
int numDocs = valueBlock.getNumDocs();
- initStringValuesSV(numDocs);
+ initBytesValuesSV(numDocs);
Review Comment:
optional: add a unit test so that if we revert this line the unit test fails.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/NullHandlingIntegrationTest.java:
##########
@@ -299,4 +296,189 @@ public void testNullLiteralSelectionOnlyBroker()
assertEquals(rows.size(), 1);
assertEquals(rows.get(0).get(0).asText(), "null");
}
+
+ @Test
+ public void testNullSelectionOnlyTransform()
+ throws Exception {
+ BitSet nullSalary = new BitSet();
+ String sqlQuery = "SELECT salary FROM " + getTableName() + " OPTION(enableNullHandling=true);";
+ JsonNode response = postQuery(sqlQuery, _brokerBaseApiUrl);
+ JsonNode rows = response.get("resultTable").get("rows");
+ double[] salaries = new double[10];
+ for (int i = 0; i < 10; i++) {
+ if (rows.get(i).get(0).asText().equals("null")) {
+ nullSalary.set(i);
+ } else {
+ salaries[i] = rows.get(i).get(0).asDouble();
+ }
+ }
+ BitSet nullDescription = new BitSet();
+ sqlQuery = "SELECT description FROM " + getTableName() + " OPTION(enableNullHandling=true);";
+ response = postQuery(sqlQuery, _brokerBaseApiUrl);
+ rows = response.get("resultTable").get("rows");
+ for (int i = 0; i < 10; i++) {
+ if (rows.get(i).get(0).asText().equals("null")) {
+ nullDescription.set(i);
+ }
+ }
+ // AdditionTransformFunction
+ sqlQuery = "SELECT add(salary, add(null, 1)) FROM " + getTableName() + " OPTION(enableNullHandling=true);";
+ response = postQuery(sqlQuery, _brokerBaseApiUrl);
+ rows = response.get("resultTable").get("rows");
+ for (int i = 0; i < 10; i++) {
+ if (nullSalary.get(i)) {
+ assertEquals(rows.get(i).get(0).asText(), "null");
+ }
Review Comment:
Assert that every row is null?
##########
pinot-core/src/main/java/org/apache/pinot/core/common/RowBasedBlockValueFetcher.java:
##########
Review Comment:
Please file an issue to create a unit test file for this file as well as TransformBlock and TransformBlockValSet.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/ScalarTransformFunctionWrapper.java:
##########
@@ -174,7 +174,7 @@ public Pair<int[], RoaringBitmap> transformToIntValuesSVWithNull(ValueBlock valu
}
Object result = _functionInvoker.invoke(_scalarArguments);
if (result != null) {
- _intValuesSV[i] = (int) result;
+ _intValuesSV[i] = (int) _resultType.toInternal(result);
Review Comment:
optional: add a unit test so that if we revert this line the unit test fails.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/docvalsets/TransformBlockValSet.java:
##########
@@ -46,47 +45,23 @@ public class TransformBlockValSet implements BlockValSet {
private final TransformFunction _transformFunction;
private final ExpressionContext _expression;
- private boolean _nullBitmapSet;
- private RoaringBitmap _nullBitmap;
+ private RoaringBitmap _nullBitmap = null;
private int[] _numMVEntries;
+ private final boolean _isNullHandlingEnabled;
+
public TransformBlockValSet(ValueBlock valueBlock, TransformFunction transformFunction,
- ExpressionContext expression) {
+ ExpressionContext expression, boolean isNullHandlingEnabled) {
_valueBlock = valueBlock;
_transformFunction = transformFunction;
_expression = expression;
+ _isNullHandlingEnabled = isNullHandlingEnabled;
}
@Nullable
@Override
public RoaringBitmap getNullBitmap() {
Review Comment:
What if `getNullBitmap` is called before any of the `getXXValuesSV` 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] KKcorps commented on pull request #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on PR #10613:
URL: https://github.com/apache/pinot/pull/10613#issuecomment-1529418354
Seems like another rebase needs to be done to resolve conflicts from previous 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
[GitHub] [pinot] shenyu0127 commented on a diff in pull request #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #10613:
URL: https://github.com/apache/pinot/pull/10613#discussion_r1222324918
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/docvalsets/TransformBlockValSet.java:
##########
@@ -46,47 +45,23 @@ public class TransformBlockValSet implements BlockValSet {
private final TransformFunction _transformFunction;
private final ExpressionContext _expression;
- private boolean _nullBitmapSet;
- private RoaringBitmap _nullBitmap;
+ private RoaringBitmap _nullBitmap = null;
private int[] _numMVEntries;
+ private final boolean _isNullHandlingEnabled;
+
public TransformBlockValSet(ValueBlock valueBlock, TransformFunction transformFunction,
- ExpressionContext expression) {
+ ExpressionContext expression, boolean isNullHandlingEnabled) {
_valueBlock = valueBlock;
_transformFunction = transformFunction;
_expression = expression;
+ _isNullHandlingEnabled = isNullHandlingEnabled;
}
@Nullable
@Override
public RoaringBitmap getNullBitmap() {
Review Comment:
We may call `getNullBitmap` before `getXXValuesSV`. [Here](https://github.com/apache/pinot/blob/6ab65f8cf734a40bc56c26534c2fb82a2c3c8849/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/SumAggregationFunction.java#L75) is an example.
Do we want to implement `getNullBitmap` by having a `_nullBitmapSet` and calling the corresponding `getXXValuesSV` to initialize the `_nullBitmap` on demand?
--
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 commented on a diff in pull request #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10613:
URL: https://github.com/apache/pinot/pull/10613#discussion_r1227401708
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/docvalsets/TransformBlockValSet.java:
##########
@@ -118,111 +93,181 @@ public int[] getDictionaryIdsSV() {
public int[] getIntValuesSV() {
try (InvocationScope scope = Tracing.getTracer().createScope(TransformBlockValSet.class)) {
recordTransformValues(scope, DataType.INT, true);
- return _transformFunction.transformToIntValuesSV(_valueBlock);
+ if (!_isNullHandlingEnabled) {
+ return _transformFunction.transformToIntValuesSV(_valueBlock);
+ }
+ Pair<int[], RoaringBitmap> result = _transformFunction.transformToIntValuesSVWithNull(_valueBlock);
Review Comment:
Should we also add `getIntValuesSVWithNull()` etc so that it is consistent with transform function behavior (separate the APIs with or without null handling)
--
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 #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10613:
URL: https://github.com/apache/pinot/pull/10613#issuecomment-1507933264
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10613?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 [#10613](https://codecov.io/gh/apache/pinot/pull/10613?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f84571b) 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 `45.99%`.
> The diff coverage is `9.79%`.
```diff
@@ Coverage Diff @@
## master #10613 +/- ##
=============================================
- Coverage 70.37% 24.38% -45.99%
+ Complexity 6495 49 -6446
=============================================
Files 2106 2090 -16
Lines 113004 113222 +218
Branches 17026 17181 +155
=============================================
- Hits 79521 27609 -51912
- Misses 27917 82695 +54778
+ Partials 5566 2918 -2648
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.38% <9.79%> (-0.16%)` | :arrow_down: |
| integration2 | `?` | |
| unittests1 | `?` | |
| 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/10613?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../transform/function/CoalesceTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10613?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQ29hbGVzY2VUcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-87.31%)` | :arrow_down: |
| [.../transform/function/DateTimeTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10613?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vRGF0ZVRpbWVUcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-90.22%)` | :arrow_down: |
| [...transform/function/DateTruncTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10613?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vRGF0ZVRydW5jVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-86.67%)` | :arrow_down: |
| [...r/transform/function/ExtractTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10613?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vRXh0cmFjdFRyYW5zZm9ybUZ1bmN0aW9uLmphdmE=) | `0.00% <0.00%> (-84.22%)` | :arrow_down: |
| [.../transform/function/GreatestTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10613?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vR3JlYXRlc3RUcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-92.99%)` | :arrow_down: |
| [...erator/transform/function/InTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10613?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) | `0.00% <0.00%> (-56.55%)` | :arrow_down: |
| [...transform/function/IsNotNullTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10613?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vSXNOb3ROdWxsVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-66.67%)` | :arrow_down: |
| [...or/transform/function/IsNullTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10613?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vSXNOdWxsVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-82.76%)` | :arrow_down: |
| [...form/function/JsonExtractKeyTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10613?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vSnNvbkV4dHJhY3RLZXlUcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-81.82%)` | :arrow_down: |
| [...tor/transform/function/LeastTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10613?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vTGVhc3RUcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [15 more](https://codecov.io/gh/apache/pinot/pull/10613?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
... and [1560 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10613/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] KKcorps commented on a diff in pull request #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #10613:
URL: https://github.com/apache/pinot/pull/10613#discussion_r1181438175
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/DateTimeConversionTransformFunction.java:
##########
@@ -106,8 +107,8 @@ public void init(List<TransformFunction> arguments, Map<String, ColumnContext> c
_dateTimeTransformer = DateTimeTransformerFactory.getDateTimeTransformer(
((LiteralTransformFunction) arguments.get(1)).getStringLiteral(),
- ((LiteralTransformFunction) arguments.get(2)).getStringLiteral(),
- ((LiteralTransformFunction) arguments.get(3)).getStringLiteral());
+ ((LiteralTransformFunction) arguments.get(2)).getStringLiteral(),
Review Comment:
nit: unnecessary formatting
--
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 #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #10613:
URL: https://github.com/apache/pinot/pull/10613#discussion_r1228422910
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/docvalsets/TransformBlockValSet.java:
##########
@@ -46,47 +45,23 @@ public class TransformBlockValSet implements BlockValSet {
private final TransformFunction _transformFunction;
private final ExpressionContext _expression;
- private boolean _nullBitmapSet;
- private RoaringBitmap _nullBitmap;
+ private RoaringBitmap _nullBitmap = null;
private int[] _numMVEntries;
+ private final boolean _isNullHandlingEnabled;
+
public TransformBlockValSet(ValueBlock valueBlock, TransformFunction transformFunction,
- ExpressionContext expression) {
+ ExpressionContext expression, boolean isNullHandlingEnabled) {
_valueBlock = valueBlock;
_transformFunction = transformFunction;
_expression = expression;
+ _isNullHandlingEnabled = isNullHandlingEnabled;
}
@Nullable
@Override
public RoaringBitmap getNullBitmap() {
Review Comment:
Per @Jackie-Jiang we can call `_transformFunction.getNullBitmap()` here.
--
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 closed pull request #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang closed pull request #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
URL: https://github.com/apache/pinot/pull/10613
--
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] KKcorps commented on a diff in pull request #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #10613:
URL: https://github.com/apache/pinot/pull/10613#discussion_r1181444730
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/GreatestTransformFunction.java:
##########
@@ -74,6 +135,35 @@ public float[] transformToFloatValuesSV(ValueBlock valueBlock) {
return _floatValuesSV;
}
+ @Override
+ public Pair<float[], RoaringBitmap> transformToFloatValuesSVWithNull(ValueBlock valueBlock) {
+ int numDocs = valueBlock.getNumDocs();
+ initFloatValuesSV(numDocs);
+ Pair<float[], RoaringBitmap> values = _arguments.get(0).transformToFloatValuesSVWithNull(valueBlock);
+ System.arraycopy(values.getLeft(), 0, _floatValuesSV, 0, numDocs);
+ RoaringBitmap nullBitmap = values.getRight();
+ for (int i = 1; i < _arguments.size(); i++) {
Review Comment:
Can we extract out this logic in a method. Seems like the only difference is the datatype of the array.
--
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 #10613: [feature] [null support # 10] Add null support in all transform function and pass the bitmap to upstream
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #10613:
URL: https://github.com/apache/pinot/pull/10613#discussion_r1212367303
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/docvalsets/TransformBlockValSet.java:
##########
@@ -46,47 +45,23 @@ public class TransformBlockValSet implements BlockValSet {
private final TransformFunction _transformFunction;
private final ExpressionContext _expression;
- private boolean _nullBitmapSet;
- private RoaringBitmap _nullBitmap;
+ private RoaringBitmap _nullBitmap = null;
private int[] _numMVEntries;
+ private final boolean _isNullHandlingEnabled;
+
public TransformBlockValSet(ValueBlock valueBlock, TransformFunction transformFunction,
- ExpressionContext expression) {
+ ExpressionContext expression, boolean isNullHandlingEnabled) {
_valueBlock = valueBlock;
_transformFunction = transformFunction;
_expression = expression;
+ _isNullHandlingEnabled = isNullHandlingEnabled;
}
@Nullable
@Override
public RoaringBitmap getNullBitmap() {
Review Comment:
We only need to address this comment, the other comments are nit-picks and optional.
--
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