xiangfu0 commented on code in PR #19944:
URL: https://github.com/apache/pinot/pull/19944#discussion_r4502421207
##########
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, compatibility / CI gate.** Removing the public `DEFAULT_OF_SPOOLS`
constant from `pinot-spi` makes the required [Binary Compatibility
Check](https://github.com/apache/pinot/actions/runs/41811983241/job/126513219606)
fail: `japicmp: REMOVED FIELD: PUBLIC STATIC FINAL boolean DEFAULT_OF_SPOOLS
... CommonConstants$Broker.DEFAULT_OF_SPOOLS:FIELD_REMOVED`. The field is a
compile-time constant, so classes that are already compiled have it inlined and
keep working. The break is source-level, for plugins that reference the field.
One option: keep a `@Deprecated` constant that points to `DEFAULT_SPOOLS_MODE`
for one release. The other: have a maintainer explicitly accept the SPI break
so the gate can be waived.
---
AI-generated review via IntentLab · Model: claude-opus-5-5
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/PinotLogicalQueryPlanner.java:
##########
@@ -106,16 +117,19 @@ public static Pair<SubPlan,
PlanFragmentAndMailboxAssignment.Result> makePlanV2(
}
private static PlanFragment planNodeToPlanFragment(
- PlanNode node, @Nullable TransformationTracker.Builder<PlanNode,
RelNode> tracker, boolean useSpools,
- String hashFunction) {
+ PlanNode node, @Nullable TransformationTracker.Builder<PlanNode,
RelNode> tracker, SpoolMode spoolMode,
+ boolean spoolsFeedback, String hashFunction) {
PlanFragmenter fragmenter = new PlanFragmenter();
PlanFragmenter.Context fragmenterContext = fragmenter.createContext();
node = node.visit(fragmenter, fragmenterContext);
- if (useSpools) {
+ List<SpoolSubstitution> substitutions = List.of();
+ if (spoolMode.isEnabled()) {
Review Comment:
**MAJOR, correctness of the new default.** `auto` shares stages that call
volatile functions. `EquivalentStagesFinder` compares `RexExpression`s
structurally and never looks at volatility. Calcite inlines CTEs, so this also
merges two *textually independent* subqueries, not only two references to one
CTE. Reproduced at this head with this query, which runs two independent 10%
samples:
```sql
SELECT x.c, y.c FROM
(SELECT COUNT(*) AS c FROM (SELECT col1 FROM a WHERE RAND() < 0.1 ORDER BY
col1 LIMIT 1000)) x
JOIN
(SELECT MAX(col1) AS c FROM (SELECT col1 FROM a WHERE RAND() < 0.1 ORDER
BY col1 LIMIT 1000)) y ON TRUE
```
The guard accepts the spool, and the shared stage is the leaf stage with
`LogicalFilter(RAND() < 0.1)`. With `off` there is no spool, so each subquery
draws its own sample. With the new default, both subqueries see the same
sample. The same happens with `SELECT col1, RAND() ...` and with `UUID_V4`.
Postgres and Spark never merge independent volatile expressions, and the PR
body's note on non-deterministic CTEs does not cover this case. A possible fix:
in `auto` (or in every mode), treat a stage that calls any
`FunctionVolatility.VOLATILE` function as not equivalent.
`PinotSqlFunction#isVolatile` and `FunctionRegistry` already carry that
information.
---
AI-generated review via IntentLab · Model: claude-opus-5-5
--
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]
