Jackie-Jiang commented on code in PR #14662:
URL: https://github.com/apache/pinot/pull/14662#discussion_r1924761131
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -421,20 +421,43 @@ public static class QueryOptionKey {
public static final String USE_SCAN_REORDER_OPTIMIZATION =
"useScanReorderOpt";
public static final String MAX_EXECUTION_THREADS =
"maxExecutionThreads";
- /** Number of groups AggregateOperator should limit result to after
sorting.
- * Trimming happens only when (sub)query contains order by and limit
clause. */
+ /**
Review Comment:
There are quite some unrelated javadoc format changes. I guess you should
turn off the IDE auto-reformat javadoc
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -421,20 +421,43 @@ public static class QueryOptionKey {
public static final String USE_SCAN_REORDER_OPTIMIZATION =
"useScanReorderOpt";
public static final String MAX_EXECUTION_THREADS =
"maxExecutionThreads";
- /** Number of groups AggregateOperator should limit result to after
sorting.
- * Trimming happens only when (sub)query contains order by and limit
clause. */
+ /**
+ * Number of groups AggregateOperator should limit result to after
sorting.
+ * Trimming happens only when (sub)query contains order by and limit
clause.
+ */
public static final String GROUP_TRIM_SIZE = "groupTrimSize";
- /** Number of groups GroupByOperator should limit result to after
sorting.
- * Trimming happens only when (sub)query contains order by clause. */
+ /**
+ * Number of groups GroupByOperator should limit result to after
sorting.
+ * Trimming happens only when (sub)query contains order by clause.
+ */
public static final String MIN_SEGMENT_GROUP_TRIM_SIZE =
"minSegmentGroupTrimSize";
- /** Max number of groups GroupByCombineOperator (running at server)
should return .*/
+ /**
+ * Max number of groups GroupByCombineOperator (running at server)
should return .
+ */
public static final String MIN_SERVER_GROUP_TRIM_SIZE =
"minServerGroupTrimSize";
- /** Max number of groups GroupByDataTableReducer (running at broker)
should return. */
+ /**
+ * Max number of groups GroupByDataTableReducer (running at broker)
should return.
+ */
public static final String MIN_BROKER_GROUP_TRIM_SIZE =
"minBrokerGroupTrimSize";
+ /**
+ * Number of threads used in the server level final reduce, this is
used for expensive aggregation functions,
+ * and we want to push down reduce to server level instead of
streaming all the data back to broker for
+ * global reduce.
+ *
+ * E.g. Funnel queries are considered as expensive aggregation
functions.
+ */
+ public static final String NUM_THREADS_FOR_SERVER_FINAL_REDUCE =
"numThreadsForServerFinalReduce";
Review Comment:
Consider `numThreadsExtractFinalResult`. Based on my code reading, it will
also be applied to broker reduce. Same for chunk size
##########
pinot-core/src/main/java/org/apache/pinot/core/data/table/IndexedTable.java:
##########
@@ -19,24 +19,32 @@
package org.apache.pinot.core.data.table;
import com.google.common.base.Preconditions;
+import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import org.apache.pinot.common.request.context.ExpressionContext;
import org.apache.pinot.common.utils.DataSchema;
import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
import org.apache.pinot.core.query.aggregation.function.AggregationFunction;
import org.apache.pinot.core.query.request.context.QueryContext;
+import org.apache.pinot.core.util.trace.TraceCallable;
/**
* Base implementation of Map-based Table for indexed lookup
*/
@SuppressWarnings({"rawtypes", "unchecked"})
public abstract class IndexedTable extends BaseTable {
+ private static final int THREAD_POOL_SIZE =
Math.max(Runtime.getRuntime().availableProcessors(), 1);
Review Comment:
(minor) Some constants are available in `ResourceManager`
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/ArrayTest.java:
##########
@@ -873,7 +873,7 @@ public Schema createSchema() {
}
@Override
- public File createAvroFile()
+ public List<File> createAvroFiles()
Review Comment:
I don't fully follow. Why do we change these tests?
##########
pinot-core/src/main/java/org/apache/pinot/core/data/table/IndexedTable.java:
##########
@@ -19,24 +19,32 @@
package org.apache.pinot.core.data.table;
import com.google.common.base.Preconditions;
+import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import org.apache.pinot.common.request.context.ExpressionContext;
import org.apache.pinot.common.utils.DataSchema;
import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
import org.apache.pinot.core.query.aggregation.function.AggregationFunction;
import org.apache.pinot.core.query.request.context.QueryContext;
+import org.apache.pinot.core.util.trace.TraceCallable;
/**
* Base implementation of Map-based Table for indexed lookup
*/
@SuppressWarnings({"rawtypes", "unchecked"})
public abstract class IndexedTable extends BaseTable {
+ private static final int THREAD_POOL_SIZE =
Math.max(Runtime.getRuntime().availableProcessors(), 1);
Review Comment:
This name is also confusing. Seems this is the upper bound when
`_numThreadsForServerFinalReduce` is not configured. Why not use the same upper
bound?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
