Jackie-Jiang commented on code in PR #17481:
URL: https://github.com/apache/pinot/pull/17481#discussion_r2677780117
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -414,6 +413,12 @@ private SqlNode validate(SqlNode sqlNode, PlannerContext
plannerContext) {
}
validated.accept(new BytesCastVisitor(plannerContext.getValidator()));
validated.accept(new RowExpressionValidationVisitor());
+
+ // Validate IN clause size to prevent Calcite planner bottlenecks with
large IN lists
+ int maxInClauseElements = QueryOptionsUtils.getMseMaxInClauseElements(
+ plannerContext.getOptions(),
plannerContext.getEnvConfig().defaultMseMaxInClauseElements());
+ validated.accept(new InClauseSizeValidationVisitor(maxInClauseElements));
Review Comment:
Do not run visitor when it is not limited
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -236,9 +237,7 @@ private PlannerContext getPlannerContext(SqlNodeAndOptions
sqlNodeAndOptions) {
sqlNodeAndOptions.getOptions(), _envConfig, format,
physicalPlannerContext);
}
- /// @deprecated Use [#compile] and then
[plan][CompiledQuery#planQuery(long)] the returned query instead
@VisibleForTesting
- @Deprecated
Review Comment:
Should we keep it as deprecated? Seems like it is used only in test, and not
for production usage
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -623,6 +623,16 @@ public static class Broker {
// TODO: Change this default to something very high, as this
_optimnization_ is usually not beneficial.
public static final int DEFAULT_SORT_EXCHANGE_COPY_THRESHOLD = 10_000;
+ /**
+ * Maximum number of elements allowed in an IN clause for multi-stage
engine queries.
+ * Queries with IN clauses exceeding this limit will be rejected to
prevent Calcite planner bottlenecks.
+ * A value of 0 or negative disables this validation.
+ * This value can always be overridden by {@link
Request.QueryOptionKey#MSE_MAX_IN_CLAUSE_ELEMENTS} query option
+ */
+ public static final String CONFIG_OF_MSE_MAX_IN_CLAUSE_ELEMENTS =
+ "pinot.broker.multistage.max.in.clause.elements";
+ public static final int DEFAULT_MSE_MAX_IN_CLAUSE_ELEMENTS = 1000;
Review Comment:
This will introduce backward incompatibility for existing queries right?
--
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]
