xiangfu0 commented on code in PR #19944:
URL: https://github.com/apache/pinot/pull/19944#discussion_r4343871015
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -648,10 +648,19 @@ public static class Broker {
public static final String CONFIG_OF_INFER_PARTITION_HINT =
"pinot.broker.multistage.infer.partition.hint";
public static final boolean DEFAULT_INFER_PARTITION_HINT = false;
- /// Whether to use spools in multistage query engine by default.
+ /// The default spool mode of the multistage query engine: `off`, `auto`
or `on`. `false` and `no` are aliases of
+ /// `off`, and `true` and `yes` are aliases of `on`.
+ ///
+ /// - `off` never shares a stage between consumers.
+ /// - `auto` shares a stage only when the planner can prove that the
sharing cannot block the query.
+ /// - `on` shares every equivalent stage that the planner supports,
without that safety check.
+ ///
+ /// Only the default planner applies spools. With
[Request.QueryOptionKey#USE_PHYSICAL_OPTIMIZER] the mode is
+ /// ignored.
+ ///
/// This value can always be overridden by
[Request.QueryOptionKey#USE_SPOOLS] query option
public static final String CONFIG_OF_SPOOLS =
"pinot.broker.multistage.spools";
- public static final boolean DEFAULT_OF_SPOOLS = false;
+ public static final String DEFAULT_SPOOLS_MODE = "auto";
Review Comment:
MAJOR (discussion, not a code defect): flipping the default from off to
`auto` is a silent result-semantics change for existing deployments. As the PR
body notes, a non-deterministic CTE (e.g. `LIMIT` without `ORDER BY`) that was
previously evaluated once per consumer is now evaluated once and shared, so
queries that happened to rely on independent evaluation can start returning
different (though not more wrong) results. The deadlock guard removes the hang
risk, which is great, but this correctness-visible default change deserves a
prominent call-out in release notes/upgrade docs. I see the plan to merge after
1.16 for bake time — that addresses the risk management side; just flagging the
user-visible semantics so it's documented.
--
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]
