Jackie-Jiang commented on code in PR #19674:
URL: https://github.com/apache/pinot/pull/19674#discussion_r4113661491
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -798,6 +798,27 @@ 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;
+ /// Config for the smallest IN list that the multi-stage planner hides
from Calcite's optimizer.
+ ///
+ /// Calcite keeps an IN list as one `SEARCH` call over a sorted range set.
Many planner rules and metadata
+ /// handlers rebuild that range set every time they touch the predicate,
so planning time grows with the list size
+ /// times the number of touches. IN lists (and `OR` chains of equalities
in a filter) with at least this many values
+ /// are sealed into an opaque predicate during optimization and restored
afterwards.
+ ///
+ /// Predicates in the same filter or join condition still fold into the
list first, as without sealing. Sealed lists
+ /// lose Calcite's value-level reasoning across plan nodes: a predicate
that a rule moves next to a sealed list on
+ /// the same column is not merged into it. For example, an `x IS NOT NULL`
in another clause than a sealed list on
+ /// `x` is dropped as redundant when the two meet, which is correct in
SQL. With null handling disabled, the servers
+ /// then do not filter the rows where `x` is null, as today for `x < 5 AND
x IS NOT NULL`.
+ ///
+ /// A value of 0 or less disables sealing. Planning outside the broker's
multi-stage request handler (for example
+ /// for the controller `/sql` endpoint) does not read this broker config
and uses the default. The query option
+ /// works everywhere.
+ public static final String CONFIG_OF_SEALED_IN_LIST_THRESHOLD =
"pinot.broker.multistage.sealed.in.list.threshold";
+ /// Same as the default of Calcite's
`SqlToRelConverter.Config#getInSubQueryThreshold()`: the size from which
+ /// Calcite itself stops treating an IN list as a scalar predicate.
+ public static final int DEFAULT_SEALED_IN_LIST_THRESHOLD = 20;
Review Comment:
Could we preserve the cross-clause null guard before enabling sealing by
default? For a nullable `b.x`, a query with `b.x NOT IN (<20+ values>)` in
`JOIN ON` and `b.x IS NOT NULL` in `WHERE` can lose the `IS NOT NULL` check
after the sealed predicate meets it. With null handling disabled, the server
may then admit rows where `b.x` is null. The new `SealedInListPlanningTest`
comment at lines 201-208 acknowledges and excludes this case. Please add a
cross-clause regression with null handling disabled and retain the guard (or
leave sealing off by default until it is retained).
--
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]
