You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2023/09/08 06:47:29 UTC
[GitHub] [pinot] gortiz commented on a diff in pull request #11546: Fix AND and OR transform functions.
gortiz commented on code in PR #11546:
URL: https://github.com/apache/pinot/pull/11546#discussion_r1319438573
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/AndOperatorTransformFunction.java:
##########
@@ -45,4 +48,37 @@ int getLogicalFuncResult(int left, int right) {
}
return 0;
}
+
+ @Nullable
+ @Override
+ public RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
+ int numDocs = valueBlock.getNumDocs();
+ int numArguments = _arguments.size();
+ int[][] intValuesSVs = new int[numArguments][numDocs];
Review Comment:
I think this is incorrect from performance perspective. This sentence instantiates the whole matrix (it allocates an array of `numArguments` elements pointing to a new array of `numDocs` ints). Few lines later elements on `intValuesSVs` are changed to `_arguments.get(i).transformToIntValuesSV(valueBlock);`. Therefore we are creating the internal arrays for no reason.
What you should do here is `new int[numArguments][];`
You can verify that in jshell:
```
╰─➤ jshell microk8s
| Welcome to JShell -- Version 11.0.19
| For an introduction type: /help intro
jshell> var a = new int[2][3];
a ==> int[2][] { int[3] { 0, 0, 0 }, int[3] { 0, 0, 0 } }
jshell> a[0]
$2 ==> int[3] { 0, 0, 0 }
jshell> var b = new int[2][];
b ==> int[2][] { null, null }
jshell> b[0]
$4 ==> null
```
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/AndOperatorTransformFunction.java:
##########
@@ -45,4 +48,37 @@ int getLogicalFuncResult(int left, int right) {
}
return 0;
}
+
+ @Nullable
+ @Override
+ public RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
+ int numDocs = valueBlock.getNumDocs();
+ int numArguments = _arguments.size();
+ int[][] intValuesSVs = new int[numArguments][numDocs];
Review Comment:
I think this is incorrect from performance perspective. This sentence instantiates the whole matrix (it allocates an array of `numArguments` elements pointing to a new array of `numDocs` ints). Few lines later elements on `intValuesSVs` are changed to `_arguments.get(i).transformToIntValuesSV(valueBlock);`. Therefore we are creating the internal arrays for no reason.
What you should do here is `new int[numArguments][];`
You can verify that in jshell:
```
╰─➤ jshell microk8s
| Welcome to JShell -- Version 11.0.19
| For an introduction type: /help intro
jshell> var a = new int[2][3];
a ==> int[2][] { int[3] { 0, 0, 0 }, int[3] { 0, 0, 0 } }
jshell> a[0]
$2 ==> int[3] { 0, 0, 0 }
jshell> var b = new int[2][];
b ==> int[2][] { null, null }
jshell> b[0]
$4 ==> 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