gortiz opened a new issue, #19557: URL: https://github.com/apache/pinot/issues/19557
## Summary Pinot's multi-stage planner currently folds constant expressions **twice, with two different semantics, in a single `QueryEnvironment.compile`**. One folder uses Calcite's semantics, the other uses Pinot's. We should decide which one wins and unify on it. Constant folding is only correct if it agrees with what the runtime would produce for the same expression applied to a column value. Pinot's runtime is Pinot's, not Calcite's — so every place we fold with Calcite semantics is a latent source of results that differ from the unfolded path. ## Where it stands today | Phase | Mechanism | Semantics | Codegen | |---|---|---|---| | validation | `BytesCastVisitor` | — (rejects, does not fold) | no | | SQL-to-rel | `RexExecutorImpl` / `PinotRexExecutor` (#19514) | Calcite | Janino | | SQL-to-rel | `RelBuilder` → `RexSimplify` | Calcite | Janino | | Hep | `PinotEvaluateLiteralRule` | **Pinot** (`FunctionRegistry` + `QueryFunctionInvoker`) | no | | Hep | `CoreRules.FILTER_REDUCE_EXPRESSIONS` → `RexSimplify` | Calcite | Janino | | Hep | `CoreRules.FILTER_REDUCE_EXPRESSIONS` → bulk reduce | *inert* (see gotcha below) | — | `BytesCastVisitor` is a symptom rather than a folder: it rejects `CAST(<string> AS BINARY)` during validation and tells the user to call `hexToBytes`, because Calcite would otherwise fold it with semantics Pinot does not share. `PinotQueryRuleSets` already carries a TODO next to the literal rules asking whether they can be replaced by `CoreRules.PROJECT_REDUCE_EXPRESSIONS` / `FILTER_REDUCE_EXPRESSIONS` — i.e. resolving the same split, in the opposite direction. This issue is where that decision should be made. ## Prior art Several Calcite users replace the default executor with one that evaluates using their own runtime: - **Druid** — `DruidRexExecutor`, javadoc: *"ensures that constant reduction is done in a manner consistent with the query runtime"* - **Hive** — `HiveRexExecutorImpl`, via `ConstantPropagateProcFactory.foldExpr` - **Drill** — own executor; the origin of [CALCITE-977](https://issues.apache.org/jira/browse/CALCITE-977), which made the executor configurable in `FrameworkConfig` Calcite's design rationale for the default (Janino codegen) is that it keeps a single implementation of the computation rules, in the runtime. That benefit is real, but it only accrues to engines whose runtime *is* Calcite's. The cautionary tale is [CALCITE-1653](https://issues.apache.org/jira/browse/CALCITE-1653) (and 1650, 1651), where Hive's executor diverged from Calcite's on BOOLEAN and DATE casts. Note the failure mode differs for us: Hive's divergence was unintended, whereas for Pinot divergence from Calcite is the goal. Our testable invariant is "folded path == Pinot runtime path", not "== Calcite". ## Proposed first step: measure the seam Before committing to a design, build a differential harness that folds a corpus of constant expressions both ways — Calcite's `RexExecutorImpl` versus Pinot's `FunctionRegistry` evaluation — and reports every divergence. That gives an inventory of how wide the gap actually is, for roughly a day of work, and makes the design decision evidence-based. Skipping this step is how Hive found its divergences one bug report at a time. Coverage should include at minimum: `CAST` across all scalar target types, arithmetic, `CASE`, null propagation and nullability, and implicit type coercion. ## Gotcha that blocks the obvious shortcut `ReduceExpressionsRule` reads the planner executor **twice, with different fallbacks**: - `reduceExpressions()` uses `Util.first(cluster.getPlanner().getExecutor(), RexUtil.EXECUTOR)` — never null, so its `RexSimplify` path Janino-compiles during the Hep phase today. - `reduceExpressionsInternal()` reads the same getter raw and returns early when it is null: *"Cannot reduce expressions: caller has not set an executor in their environment."* Since `PINOT_POST_RULES` and `PINOT_POST_RULES_V2` both contain `CoreRules.FILTER_REDUCE_EXPRESSIONS`, its bulk reduction path is **inert today** and begins firing the moment any executor is installed during `optimize()`. Both reads hit the same getter, so cache coverage and rule activation cannot be separated without patching Calcite. Consequence: any change that installs an executor for the Hep phase is plan-changing and must regenerate and review the resource-based plan files. This is why #19514 deliberately restores the original executor after SQL-to-rel — that restore is load-bearing, not caution. ## Relationship to #19514 #19514 makes Calcite's folding cheap by caching compiled cast templates keyed on types rather than values. It is a pure performance change with no plan delta, and it stands on its own regardless of how this issue is resolved. This issue is the continuation: deciding whether Calcite should be doing that folding at all. If the answer is "Pinot semantics win", the natural landing place is the same follow-up that extends executor coverage to the Hep phase, since that PR is already touching what runs during `optimize()`. ## Upstream context Raised on dev@calcite as [Reduce class loading during query optimizing](https://www.mail-archive.com/[email protected]/msg24885.html): >80k `ByteArrayClassLoader` instances at 1000 QPS with threads blocking in `BuiltinClassLoader.loadClassOrNull`, traced to `RexSimplify.simplify` → `RexExecutable.compile`. [Stamatis Zampetakis' reply](https://www.mail-archive.com/[email protected]/msg24891.html) suggested three directions: decouple `RexSimplify` from `RexExecutor`, add an interpretation-based `ScalarCompiler`, or extend caching to `RexExecutor`. #19514 is the third. This issue is closest to the second, done in Pinot rather than upstream. For the record, `RexInterpreter` in `calcite-core` is not a shortcut: its javadoc says *"intended for testing"* and *"not very efficient"*, and its `SUPPORTED_SQL_KIND` does not include `CAST`. -- 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]
