You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/11/05 11:39:26 UTC

[GitHub] [pinot] richardstartin opened a new pull request #7707: lighter weight LiteralTransformFunction, avoid excessive array fills

richardstartin opened a new pull request #7707:
URL: https://github.com/apache/pinot/pull/7707


   
   ## Description
   After profiling a load test with complex case statements, it was found that 7% of time was spent in `Arrays.fill` under `LiteralTransformFunction.transformToStringValuesSV`. 
   
   <img width="935" alt="Screenshot 2021-11-05 at 10 56 47" src="https://user-images.githubusercontent.com/16439049/140504568-8dd0f605-72be-4ebe-9aa4-ba4159e01dbf.png">
   
   This is because the arrays are overallocated to 10k elements, when the number of documents in the projection block should generally be small and stable. I also noticed that _for each literal_, over 30 `LiteralTransformFunction`s are constructed in a single query with a simple case statement. Since Pinot servers typically run with more than 32GB of heap and therefore do not benefit from compressed references, I replaced 48 bytes for uncompressed references to typed arrays with 8 bytes for a reference to an `Object`, with instanceof checks before filling the array.
   
   ## Upgrade Notes
   Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
   * [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
   
   Does this PR fix a zero-downtime upgrade introduced earlier?
   * [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
   
   Does this PR otherwise need attention when creating release notes? Things to consider:
   - New configuration options
   - Deprecation of configurations
   - Signature changes to public methods/interfaces
   - New plugins added or old plugins removed
   * [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
   ## Release Notes
   <!-- If you have tagged this as either backward-incompat or release-notes,
   you MUST add text here that you would like to see appear in release notes of the
   next release. -->
   
   <!-- If you have a series of commits adding or enabling a feature, then
   add this section only in final commit that marks the feature completed.
   Refer to earlier release notes to see examples of text.
   -->
   ## Documentation
   <!-- If you have introduced a new feature or configuration, please add it to the documentation as well.
   See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
   -->
   


-- 
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 change in pull request #7707: lighter weight LiteralTransformFunction, avoid excessive array fills

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7707:
URL: https://github.com/apache/pinot/pull/7707#discussion_r744007398



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java
##########
@@ -128,64 +122,88 @@ public Dictionary getDictionary() {
 
   @Override
   public int[] transformToIntValuesSV(ProjectionBlock projectionBlock) {
-    if (_intResult == null) {
-      _intResult = new int[DocIdSetPlanNode.MAX_DOC_PER_CALL];
+    int numDocs = projectionBlock.getNumDocs();
+    Object ref = _result;

Review comment:
       Good point. Currently we create these LiteralTransformFunctions per literal per segment, which isn't really necessary. We can create them only once, and cache them into the `QueryContext` so that they can be shared across different literals and segments. That can be done in a separate 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] richardstartin commented on a change in pull request #7707: lighter weight LiteralTransformFunction, avoid excessive array fills

Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7707:
URL: https://github.com/apache/pinot/pull/7707#discussion_r743962262



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java
##########
@@ -128,64 +122,88 @@ public Dictionary getDictionary() {
 
   @Override
   public int[] transformToIntValuesSV(ProjectionBlock projectionBlock) {
-    if (_intResult == null) {
-      _intResult = new int[DocIdSetPlanNode.MAX_DOC_PER_CALL];
+    int numDocs = projectionBlock.getNumDocs();
+    Object ref = _result;

Review comment:
       None whatsoever, but I also discovered that we create a lot more of these than I think is reasonable per query, and I think it may be wise to cache them externally. In case something changes externally, I want this method invocation to be consistent, even if the updates are racy.

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java
##########
@@ -128,64 +122,88 @@ public Dictionary getDictionary() {
 
   @Override
   public int[] transformToIntValuesSV(ProjectionBlock projectionBlock) {
-    if (_intResult == null) {
-      _intResult = new int[DocIdSetPlanNode.MAX_DOC_PER_CALL];
+    int numDocs = projectionBlock.getNumDocs();
+    Object ref = _result;
+    if (!(ref instanceof int[]) || ((int[]) ref).length < numDocs) {

Review comment:
       What if something changes? E.g. JSON often has variant types (e.g. string or number)

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java
##########
@@ -128,64 +122,88 @@ public Dictionary getDictionary() {
 
   @Override
   public int[] transformToIntValuesSV(ProjectionBlock projectionBlock) {
-    if (_intResult == null) {
-      _intResult = new int[DocIdSetPlanNode.MAX_DOC_PER_CALL];
+    int numDocs = projectionBlock.getNumDocs();
+    Object ref = _result;
+    if (!(ref instanceof int[]) || ((int[]) ref).length < numDocs) {

Review comment:
       instanceof checks aren’t worth trying to optimise away anyway, they’re intrinsified and speculatively eliminated with an uncommon trap, just like null checks




-- 
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] richardstartin commented on a change in pull request #7707: lighter weight LiteralTransformFunction, avoid excessive array fills

Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7707:
URL: https://github.com/apache/pinot/pull/7707#discussion_r744595934



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java
##########
@@ -128,64 +122,88 @@ public Dictionary getDictionary() {
 
   @Override
   public int[] transformToIntValuesSV(ProjectionBlock projectionBlock) {
-    if (_intResult == null) {
-      _intResult = new int[DocIdSetPlanNode.MAX_DOC_PER_CALL];
+    int numDocs = projectionBlock.getNumDocs();
+    Object ref = _result;
+    if (!(ref instanceof int[]) || ((int[]) ref).length < numDocs) {

Review comment:
       Maybe this needs a different approach - one of these is created per segment, so we should construct all literals in the scope of the query context, then we don't care about the footprint of a single instance.




-- 
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] richardstartin commented on a change in pull request #7707: lighter weight LiteralTransformFunction, avoid excessive array fills

Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7707:
URL: https://github.com/apache/pinot/pull/7707#discussion_r744129157



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java
##########
@@ -128,64 +122,88 @@ public Dictionary getDictionary() {
 
   @Override
   public int[] transformToIntValuesSV(ProjectionBlock projectionBlock) {
-    if (_intResult == null) {
-      _intResult = new int[DocIdSetPlanNode.MAX_DOC_PER_CALL];
+    int numDocs = projectionBlock.getNumDocs();
+    Object ref = _result;
+    if (!(ref instanceof int[]) || ((int[]) ref).length < numDocs) {

Review comment:
       I'm ok with throwing an exception if the literal is used as the wrong type.




-- 
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 #7707: lighter weight LiteralTransformFunction, avoid excessive array fills

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #7707:
URL: https://github.com/apache/pinot/pull/7707


   


-- 
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 change in pull request #7707: lighter weight LiteralTransformFunction, avoid excessive array fills

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7707:
URL: https://github.com/apache/pinot/pull/7707#discussion_r744008672



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java
##########
@@ -128,64 +122,88 @@ public Dictionary getDictionary() {
 
   @Override
   public int[] transformToIntValuesSV(ProjectionBlock projectionBlock) {
-    if (_intResult == null) {
-      _intResult = new int[DocIdSetPlanNode.MAX_DOC_PER_CALL];
+    int numDocs = projectionBlock.getNumDocs();
+    Object ref = _result;
+    if (!(ref instanceof int[]) || ((int[]) ref).length < numDocs) {

Review comment:
       The check itself is not causing the problem. The problem is that if the behavior changes, we might end up filling the array again and again, and causing performance degradation. E.g. if a function always read the literal as both INT and LONG for each block, we will keep creating new arrays and fill them.
   So I'd suggest guarding this potential misuse of the function by throwing an exception.




-- 
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 #7707: lighter weight LiteralTransformFunction, avoid excessive array fills

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #7707:
URL: https://github.com/apache/pinot/pull/7707#issuecomment-961884628


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/7707?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 [#7707](https://codecov.io/gh/apache/pinot/pull/7707?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ab885da) into [master](https://codecov.io/gh/apache/pinot/commit/e80198db7f71f6c043e4ffc728d6cfce6eb9b729?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e80198d) will **decrease** coverage by `56.95%`.
   > The diff coverage is `0.00%`.
   
   > :exclamation: Current head ab885da differs from pull request most recent head 7a3868c. Consider uploading reports for the commit 7a3868c to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/pinot/pull/7707/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/pinot/pull/7707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #7707       +/-   ##
   =============================================
   - Coverage     71.50%   14.54%   -56.96%     
   + Complexity     4030       80     -3950     
   =============================================
     Files          1581     1535       -46     
     Lines         80400    78569     -1831     
     Branches      11943    11753      -190     
   =============================================
   - Hits          57486    11426    -46060     
   - Misses        19026    66306    +47280     
   + Partials       3888      837     -3051     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `?` | |
   | integration2 | `?` | |
   | unittests1 | `?` | |
   | unittests2 | `14.54% <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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../pinot/common/utils/ClientSSLContextGenerator.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvQ2xpZW50U1NMQ29udGV4dEdlbmVyYXRvci5qYXZh) | `0.00% <0.00%> (-31.04%)` | :arrow_down: |
   | [...e/pinot/common/utils/FileUploadDownloadClient.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvRmlsZVVwbG9hZERvd25sb2FkQ2xpZW50LmphdmE=) | `0.00% <0.00%> (-66.95%)` | :arrow_down: |
   | [...rg/apache/pinot/common/utils/helix/TableCache.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvVGFibGVDYWNoZS5qYXZh) | `0.00% <0.00%> (-82.72%)` | :arrow_down: |
   | [.../core/operator/combine/GroupByCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0dyb3VwQnlDb21iaW5lT3BlcmF0b3IuamF2YQ==) | `0.00% <0.00%> (-79.37%)` | :arrow_down: |
   | [...reaming/StreamingSelectionOnlyCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nU2VsZWN0aW9uT25seUNvbWJpbmVPcGVyYXRvci5qYXZh) | `0.00% <0.00%> (-71.12%)` | :arrow_down: |
   | [...r/transform/function/LiteralTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vTGl0ZXJhbFRyYW5zZm9ybUZ1bmN0aW9uLmphdmE=) | `0.00% <0.00%> (-75.41%)` | :arrow_down: |
   | [...c/main/java/org/apache/pinot/common/tier/Tier.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [1256 more](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7707?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7707?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e80198d...7a3868c](https://codecov.io/gh/apache/pinot/pull/7707?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] Jackie-Jiang commented on a change in pull request #7707: lighter weight LiteralTransformFunction, avoid excessive array fills

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7707:
URL: https://github.com/apache/pinot/pull/7707#discussion_r743934438



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java
##########
@@ -128,64 +122,88 @@ public Dictionary getDictionary() {
 
   @Override
   public int[] transformToIntValuesSV(ProjectionBlock projectionBlock) {
-    if (_intResult == null) {
-      _intResult = new int[DocIdSetPlanNode.MAX_DOC_PER_CALL];
+    int numDocs = projectionBlock.getNumDocs();
+    Object ref = _result;

Review comment:
       Any performance benefit of caching member variable into a local variable?

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java
##########
@@ -128,64 +122,88 @@ public Dictionary getDictionary() {
 
   @Override
   public int[] transformToIntValuesSV(ProjectionBlock projectionBlock) {
-    if (_intResult == null) {
-      _intResult = new int[DocIdSetPlanNode.MAX_DOC_PER_CALL];
+    int numDocs = projectionBlock.getNumDocs();
+    Object ref = _result;
+    if (!(ref instanceof int[]) || ((int[]) ref).length < numDocs) {

Review comment:
       Per the current way of query execution, we can make the following assumptions:
   1. Only one type of the result will be read
   2. The first projection block will contain the most `numDocs` (up to 10K)
   
   With these assumptions we can change the check to
   ```suggestion
       if (ref == null) {
         ...
       } else {
         assert (ref instanceof int[]) && ((int[]) ref).length >= numDocs;
       }
   ```
   
   If the assumption break, we'd better let it throw exception, or the performance will degrade because we will end up filling the array multiple times.




-- 
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 #7707: lighter weight LiteralTransformFunction, avoid excessive array fills

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #7707:
URL: https://github.com/apache/pinot/pull/7707#issuecomment-961884628






-- 
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 edited a comment on pull request #7707: lighter weight LiteralTransformFunction, avoid excessive array fills

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7707:
URL: https://github.com/apache/pinot/pull/7707#issuecomment-961884628


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/7707?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 [#7707](https://codecov.io/gh/apache/pinot/pull/7707?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2b4b0bf) into [master](https://codecov.io/gh/apache/pinot/commit/e80198db7f71f6c043e4ffc728d6cfce6eb9b729?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e80198d) will **decrease** coverage by `40.69%`.
   > The diff coverage is `41.95%`.
   
   > :exclamation: Current head 2b4b0bf differs from pull request most recent head 96266d6. Consider uploading reports for the commit 96266d6 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/pinot/pull/7707/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/pinot/pull/7707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #7707       +/-   ##
   =============================================
   - Coverage     71.50%   30.80%   -40.70%     
   =============================================
     Files          1581     1572        -9     
     Lines         80400    80142      -258     
     Branches      11943    11922       -21     
   =============================================
   - Hits          57486    24690    -32796     
   - Misses        19026    53320    +34294     
   + Partials       3888     2132     -1756     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `29.29% <38.50%> (+0.22%)` | :arrow_up: |
   | integration2 | `27.68% <41.95%> (+0.08%)` | :arrow_up: |
   | 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/7707?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../pinot/common/utils/ClientSSLContextGenerator.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvQ2xpZW50U1NMQ29udGV4dEdlbmVyYXRvci5qYXZh) | `0.00% <0.00%> (-31.04%)` | :arrow_down: |
   | [...e/pinot/controller/helix/SegmentStatusChecker.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9TZWdtZW50U3RhdHVzQ2hlY2tlci5qYXZh) | `71.12% <0.00%> (-18.19%)` | :arrow_down: |
   | [...ller/validation/OfflineSegmentIntervalChecker.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci92YWxpZGF0aW9uL09mZmxpbmVTZWdtZW50SW50ZXJ2YWxDaGVja2VyLmphdmE=) | `76.40% <0.00%> (-9.68%)` | :arrow_down: |
   | [...r/validation/RealtimeSegmentValidationManager.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci92YWxpZGF0aW9uL1JlYWx0aW1lU2VnbWVudFZhbGlkYXRpb25NYW5hZ2VyLmphdmE=) | `72.85% <0.00%> (-8.40%)` | :arrow_down: |
   | [...ment/creator/impl/SegmentColumnarIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50Q29sdW1uYXJJbmRleENyZWF0b3IuamF2YQ==) | `0.00% <0.00%> (-84.55%)` | :arrow_down: |
   | [...ot/segment/spi/creator/SegmentGeneratorConfig.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2NyZWF0b3IvU2VnbWVudEdlbmVyYXRvckNvbmZpZy5qYXZh) | `0.00% <0.00%> (-82.68%)` | :arrow_down: |
   | [...elix/core/periodictask/ControllerPeriodicTask.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3BlcmlvZGljdGFzay9Db250cm9sbGVyUGVyaW9kaWNUYXNrLmphdmE=) | `70.90% <20.00%> (-5.10%)` | :arrow_down: |
   | [...r/transform/function/LiteralTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vTGl0ZXJhbFRyYW5zZm9ybUZ1bmN0aW9uLmphdmE=) | `36.70% <39.53%> (-38.71%)` | :arrow_down: |
   | [...reaming/StreamingSelectionOnlyCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nU2VsZWN0aW9uT25seUNvbWJpbmVPcGVyYXRvci5qYXZh) | `65.30% <41.66%> (-5.81%)` | :arrow_down: |
   | [...rg/apache/pinot/common/utils/helix/TableCache.java](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvVGFibGVDYWNoZS5qYXZh) | `81.76% <62.50%> (-0.96%)` | :arrow_down: |
   | ... and [1083 more](https://codecov.io/gh/apache/pinot/pull/7707/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7707?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7707?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e80198d...96266d6](https://codecov.io/gh/apache/pinot/pull/7707?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] richardstartin commented on a change in pull request #7707: lighter weight LiteralTransformFunction, avoid excessive array fills

Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7707:
URL: https://github.com/apache/pinot/pull/7707#discussion_r744820143



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java
##########
@@ -128,64 +122,88 @@ public Dictionary getDictionary() {
 
   @Override
   public int[] transformToIntValuesSV(ProjectionBlock projectionBlock) {
-    if (_intResult == null) {
-      _intResult = new int[DocIdSetPlanNode.MAX_DOC_PER_CALL];
+    int numDocs = projectionBlock.getNumDocs();
+    Object ref = _result;
+    if (!(ref instanceof int[]) || ((int[]) ref).length < numDocs) {

Review comment:
       Please take a look now, this change now only consists of the size change (plus allowing for racy writes while guaranteeing consistent reads) - the caching is done in #7720




-- 
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