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/06/14 07:08:52 UTC

[GitHub] [incubator-pinot] GSharayu opened a new pull request #7047: Add numRowsResultSet to RequestStatistics

GSharayu opened a new pull request #7047:
URL: https://github.com/apache/incubator-pinot/pull/7047


   This PR adds numRowsResultSet to RequestStatistics. As we already have ResultTable object computed, there is no extra overhead 


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

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] [incubator-pinot] siddharthteotia commented on a change in pull request #7047: Add numRowsResultSet to RequestStatistics

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #7047:
URL: https://github.com/apache/incubator-pinot/pull/7047#discussion_r649629237



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BrokerReduceService.java
##########
@@ -232,6 +232,7 @@ public BrokerResponseNative reduceOnDataTable(BrokerRequest brokerRequest,
     }
 
     // Set execution statistics.
+    ResultTable resultTable = brokerResponseNative.getResultTable();

Review comment:
       Since ResultTable is only available in SQL mode, I suggest  doing the following
   
   @JsonProperty("resultTable")
   public void setResultTable(ResultTable resultTable) {
       _resultTable = resultTable;
      _numRowsResultSet = resultTable.getRows().size()
     }
   
   This will ensure we don't run into NPE. Also, this will populate non zero numRowsResultSet only for SQL mode but that is fine as PQL will be removed anyway

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BrokerReduceService.java
##########
@@ -232,6 +232,7 @@ public BrokerResponseNative reduceOnDataTable(BrokerRequest brokerRequest,
     }
 
     // Set execution statistics.
+    ResultTable resultTable = brokerResponseNative.getResultTable();

Review comment:
       Since ResultTable is only available in SQL mode, I suggest  doing the following
   
   ```
   @JsonProperty("resultTable")
   public void setResultTable(ResultTable resultTable) {
       _resultTable = resultTable;
      _numRowsResultSet = resultTable.getRows().size()
     }
   ```
   
   This will ensure we don't run into NPE. Also, this will populate non zero numRowsResultSet only for SQL mode but that is fine as PQL will be removed anyway

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BrokerReduceService.java
##########
@@ -232,6 +232,7 @@ public BrokerResponseNative reduceOnDataTable(BrokerRequest brokerRequest,
     }
 
     // Set execution statistics.
+    ResultTable resultTable = brokerResponseNative.getResultTable();

Review comment:
       Since ResultTable is only available in SQL mode, I suggest  doing the following in BrokerResponseNative
   
   ```
   @JsonProperty("resultTable")
   public void setResultTable(ResultTable resultTable) {
       _resultTable = resultTable;
      _numRowsResultSet = resultTable.getRows().size()
     }
   ```
   
   This will ensure we don't run into NPE. Also, this will populate non zero numRowsResultSet only for SQL mode but that is fine as PQL will be removed anyway

##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/api/RequestStatistics.java
##########
@@ -220,6 +222,10 @@ public long getOfflineThreadCpuTimeNs() {
     return _offlineThreadCpuTimeNs;
   }
 
+  public int getNumRowsResultSet() { return _numRowsResultSet; }
+
+  public void setNumRowsResultSet(int numRowsResultSet) { _numRowsResultSet = numRowsResultSet; }

Review comment:
       Can we remove this method as no one seems to be calling this ?

##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/api/RequestStatistics.java
##########
@@ -220,6 +222,10 @@ public long getOfflineThreadCpuTimeNs() {
     return _offlineThreadCpuTimeNs;
   }
 
+  public int getNumRowsResultSet() { return _numRowsResultSet; }
+
+  public void setNumRowsResultSet(int numRowsResultSet) { _numRowsResultSet = numRowsResultSet; }

Review comment:
       We still need the getter as we will use it internally




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

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] [incubator-pinot] GSharayu commented on a change in pull request #7047: Add numRowsResultSet to RequestStatistics

Posted by GitBox <gi...@apache.org>.
GSharayu commented on a change in pull request #7047:
URL: https://github.com/apache/incubator-pinot/pull/7047#discussion_r649632684



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/api/RequestStatistics.java
##########
@@ -220,6 +222,10 @@ public long getOfflineThreadCpuTimeNs() {
     return _offlineThreadCpuTimeNs;
   }
 
+  public int getNumRowsResultSet() { return _numRowsResultSet; }
+
+  public void setNumRowsResultSet(int numRowsResultSet) { _numRowsResultSet = numRowsResultSet; }

Review comment:
       Most of the getters and setters are used here, should we add a follow up to clean them as well
   

##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/api/RequestStatistics.java
##########
@@ -220,6 +222,10 @@ public long getOfflineThreadCpuTimeNs() {
     return _offlineThreadCpuTimeNs;
   }
 
+  public int getNumRowsResultSet() { return _numRowsResultSet; }
+
+  public void setNumRowsResultSet(int numRowsResultSet) { _numRowsResultSet = numRowsResultSet; }

Review comment:
       Most of the getters and setters are unused here, should we add a follow up to clean them as well
   

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BrokerReduceService.java
##########
@@ -232,6 +232,7 @@ public BrokerResponseNative reduceOnDataTable(BrokerRequest brokerRequest,
     }
 
     // Set execution statistics.
+    ResultTable resultTable = brokerResponseNative.getResultTable();

Review comment:
       updated!
   
   

##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/api/RequestStatistics.java
##########
@@ -220,6 +222,10 @@ public long getOfflineThreadCpuTimeNs() {
     return _offlineThreadCpuTimeNs;
   }
 
+  public int getNumRowsResultSet() { return _numRowsResultSet; }
+
+  public void setNumRowsResultSet(int numRowsResultSet) { _numRowsResultSet = numRowsResultSet; }

Review comment:
       updated!




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

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] [incubator-pinot] siddharthteotia merged pull request #7047: Add numRowsResultSet to RequestStatistics

Posted by GitBox <gi...@apache.org>.
siddharthteotia merged pull request #7047:
URL: https://github.com/apache/incubator-pinot/pull/7047


   


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

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] [incubator-pinot] codecov-commenter commented on pull request #7047: Add numRowsResultSet to RequestStatistics

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


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7047?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 [#7047](https://codecov.io/gh/apache/incubator-pinot/pull/7047?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (80a9ad3) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/18c0d9d7002faded994c4ee66ea40655b417c93d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (18c0d9d) will **decrease** coverage by `7.96%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/7047/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/incubator-pinot/pull/7047?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    #7047      +/-   ##
   ============================================
   - Coverage     73.38%   65.42%   -7.97%     
     Complexity       12       12              
   ============================================
     Files          1454     1454              
     Lines         72091    72096       +5     
     Branches      10441    10441              
   ============================================
   - Hits          52903    47166    -5737     
   - Misses        15654    21522    +5868     
   + Partials       3534     3408     -126     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration | `?` | |
   | unittests | `65.42% <100.00%> (-0.01%)` | :arrow_down: |
   
   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/incubator-pinot/pull/7047?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...org/apache/pinot/broker/api/RequestStatistics.java](https://codecov.io/gh/apache/incubator-pinot/pull/7047/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL1JlcXVlc3RTdGF0aXN0aWNzLmphdmE=) | `42.46% <100.00%> (-22.82%)` | :arrow_down: |
   | [...t/common/response/broker/BrokerResponseNative.java](https://codecov.io/gh/apache/incubator-pinot/pull/7047/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzcG9uc2UvYnJva2VyL0Jyb2tlclJlc3BvbnNlTmF0aXZlLmphdmE=) | `91.66% <100.00%> (+0.32%)` | :arrow_up: |
   | [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/7047/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7047/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/7047/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/7047/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/7047/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-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...t/core/startree/plan/StarTreeDocIdSetPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7047/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlRG9jSWRTZXRQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../core/startree/plan/StarTreeTransformPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7047/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlVHJhbnNmb3JtUGxhbk5vZGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...core/startree/plan/StarTreeProjectionPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7047/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlUHJvamVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [339 more](https://codecov.io/gh/apache/incubator-pinot/pull/7047/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/incubator-pinot/pull/7047?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/incubator-pinot/pull/7047?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 [18c0d9d...80a9ad3](https://codecov.io/gh/apache/incubator-pinot/pull/7047?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.

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