[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6225: Perf optimization for SQL GROUP BY ORDER BY

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on a change in pull request #6225:
URL: https://github.com/apache/incubator-pinot/pull/6225#discussion_r516889198



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/table/Record.java
##########
@@ -45,6 +45,7 @@
 public class Record {
   private final Object[] _values;
 
+  private Key _key;

Review comment:
       I don't think we should embed `Key` inside `Record`, especially when the key is extracted from the `Record`, and then set back into `Record` using a setter.
   It's okay to merge `Key` class into the `Record` class so that we have a central place for all the record information. The constructor should initialize both keys and values, and they should be final.

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java
##########
@@ -265,7 +265,7 @@ private IndexedTable getIndexedTable(DataSchema dataSchema, Collection<DataTable
     int numReduceThreadsToUse = getNumReduceThreadsToUse(numDataTables, reducerContext.getMaxReduceThreadsPerQuery());
 
     // In case of single reduce thread, fall back to SimpleIndexedTable to avoid redundant locking/unlocking calls.
-    int capacity = GroupByUtils.getTableCapacity(_queryContext);
+    int capacity = _queryContext.getLimit();

Review comment:
       (Critical) This will break Having clause




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