Repository navigation
DRILL-8555: Physical plan cache for parameterized SQL queries - #3086
letian-jiang wants to merge 21 commits into
Conversation
|
@shfshihuafeng Thank you for this PR. Did you take a look at #3023? I realize they are somewhat different, but it seems the overall goal is similar in trying to cache plans. Are there components of that PR that you could incorporate in yours? |
|
@cgivre Thanks for pointing me to #3023. I have reviewed it, and I agree that both PRs share the same underlying goal: reusing planning work to reduce the overhead of repeated queries. Before considering #3023 production-ready, I think we would need to establish the conditions for safe plan reuse, even for identical SQL. For example:
There is also a practical consideration: production workloads often repeat the same query shape with different literals. Restricting reuse to identical SQL text would substantially limit the benefit for those workloads. My implementation focuses on these correctness requirements through eligibility checks, context and table compatibility checks, and reconstruction of a fresh plan and scan state for each execution. It also preserves typed parameter slots so queries with different literals can reuse compatible cached plans. |
cgivre
left a comment
cgivre
left a comment
There was a problem hiding this comment.
A few issues with parameterized planning and the BIGINT literal change; details inline.
c74a235 to
f5e64d5
Compare
|
Hi @cgivre, thanks again for your review! I’ve pushed updates addressing your feedback on parameterized planning and BIGINT literal round trips. When you have a chance, could you take another look? Please let me know if there’s anything else you’d like me to address. Thanks! |
|
Suggestion: make the plan cache size and expiration configurable
Could these be boot options in A few related points:
|
|
Question: plans to support the other storage plugins? Right now only HBase and Iceberg opt in ( I understand why they can't simply be switched on. On a hit, the cache rebinds literals in Drill expressions. But most plugins turn those literals into native state during planning and keep only the result:
Reusing those plans without rebuilding that state would silently return wrong results. So each plugin needs to keep the source expression in its scan JSON and redo pushdown/pruning on bind, as A possible path, roughly in order of effort:
It would be worth measuring planning time against bind-time rebuild cost for Given how large this PR already is, I'm fine with plugin expansion coming in follow-ups. But I'd like to understand the roadmap, and to make sure the plugin API in this PR makes opting in as cheap as possible. Could you add the plan (or JIRAs for each plugin) to the design doc? |
|
Review findings Line numbers refer to the PR head. The first three are correctness issues I'd like to see fixed before merge. Correctness
Performance / design
Observability and operations
Docs
|
|
@cgivre Thank you for the detailed and thoughtful review! I’ve learned a lot from your comments. |
I’ve made the cache size and both expiration policies configurable.
|
cgivre
left a comment
cgivre
left a comment
There was a problem hiding this comment.
Line-by-line follow-up to my earlier review comment, with suggested changes where there's a concrete fix.
I applied all the suggestions together on top of 53306c1, and exec/java-exec (with its dependencies, including logical) builds cleanly, checkstyle included. Several depend on each other, so it's easiest to add them to a batch and commit them together:
- the
PlanCacheconstructor, its three imports, andnew PlanCache(config)inDrillbitContext writeAfterSuccess/putinPlanCacheand the serialization change inDrillSqlWorkerkeyFingerprint()inPlanCacheand the key inDrillSqlWorker
The drill-module.conf defaults are in the comment on the PlanCache fields, because the right spot in that file is outside the diff.
Tests: I couldn't find any plan-cache tests in this PR. PLAN_CACHE_PLUGIN_GUIDE.md describes the checks a plugin should pass (results with caching off vs. a hit with different literals, schema changes, plugin config changes, joins with unsupported plugins, etc.), but none are included for HBase, Iceberg or the engine. Could you add them? They should cover at least:
- the fallback path
- HAVING with GROUP BY
- two sessions with different options sharing a template
- a stale table version
- the new boot options
| select.getGroup(), visitNullable(select.getHaving()), | ||
| select.getWindowList(), visitNullable(select.getQualify()), |
There was a problem hiding this comment.
[Correctness] Keep HAVING and QUALIFY verbatim when GROUP BY / the select list are.
When structuralProjection is true, GROUP BY and the select list keep their literals, but HAVING and QUALIFY still get slots. So they no longer match the grouped or windowed expressions:
SELECT x || 'a', COUNT(*) FROM hbase.t GROUP BY x || 'a' HAVING x || 'a' = 'ba'HAVING becomes x || ? = ?, and Calcite raises "Expression 'x' is not being grouped". Without a fallback, the query fails only when the cache is on. Using the same condition for HAVING and QUALIFY keeps them consistent with what they have to match.
| select.getGroup(), visitNullable(select.getHaving()), | |
| select.getWindowList(), visitNullable(select.getQualify()), | |
| select.getGroup(), | |
| structuralProjection ? select.getHaving() : visitNullable(select.getHaving()), | |
| select.getWindowList(), | |
| structuralProjection ? select.getQualify() : visitNullable(select.getQualify()), |
|
I've updated the parameterizer to preserve HAVING and QUALIFY expressions whenever the SELECT expressions are preserved for grouping, ordering, or window definitions. This keeps matching expressions consistent across clauses. Eligible WHERE literals can still be rebound. SELECT x + 1 AS k,
ROW_NUMBER() OVER (ORDER BY x + 1) AS rn
FROM (VALUES (1, 1), (1, 2), (2, 3)) AS t(x, v)
WHERE v > 0
QUALIFY x + 1 = 2
AND ROW_NUMBER() OVER (ORDER BY x + 1) = 1
ORDER BY x + 1;With plan caching disabled, this returned one row: (k = 2, rn = 1). With caching enabled, it returned zero rows without any exception, both on initial planning and on a confirmed cache hit. An exception-triggered fallback would not catch this regression. |
This is a difficult problem, and I agree that a hard-coded list carries a maintenance risk. Given Drill’s current architecture, though, I don’t see a clearly better practical approach: DrillOptiq itself uses function-name switches for value-dependent rewrites, so the parameterizer needs explicit knowledge of which operands must remain literals. Moving this information into function metadata could improve organization, but would still require manually declaring and maintaining those rules. For now, I think keeping this list aligned with the existing rewrites and adding targeted regression tests is a reasonable approach, while acknowledging that it does not establish safety for every SQL pattern. |
|
Following up on the remaining points:
|
|
I’ve rerun both benchmarks on the latest commit, [6ee1c592b](letian-jiang@6ee1c59), using the same machine, datasets, queries and sampling method as the original measurements.
|
| SqlSelect select = (SqlSelect) call; | ||
| // Preserve matching expressions across SELECT, GROUP BY, HAVING and | ||
| // QUALIFY. Parameterizing only some occurrences breaks validation or rewrites. | ||
| boolean structuralProjection = preserveProjection |
There was a problem hiding this comment.
[Correctness] QUALIFY with an inline OVER and no ORDER BY is still parameterized.
The SELECT list, HAVING and QUALIFY are preserved only when there is a GROUP BY, ORDER BY or WINDOW clause. An inline OVER(...) is preserved on its own, but the expressions around it are not. Your zero-rows example from the last round was fixed because it ended in ORDER BY. The same query without it still gets x + ? = ? in both the select list and QUALIFY, and the root cause of the zero rows was never identified.
Could structuralProjection also be true when the SELECT contains any OVER call or a QUALIFY clause? Please also add the no-ORDER-BY variant to TestPlanCachePlanning.
There was a problem hiding this comment.
I now preserve literals in SELECT, HAVING and QUALIFY unconditionally. For example, SELECT x + 1 keeps the 1 in the template key. I expect most of the benefit to come from rebinding WHERE predicates, so this conservative restriction seems reasonable. Added tests for QUALIFY without an outer ORDER BY and inline windows.
| case FunctionNames.GE: { | ||
| LogicalExpression nameRef = call.args().get(0); | ||
| Expression expression = call.args().get(0).accept(this, null); | ||
| Expression expression = call.args().get(1).accept(this, null); |
There was a problem hiding this comment.
[Behaviour change for all queries] This changes Iceberg pushdown with the cache off too.
Several changes in this file and in IcebergPluginImplementor change pushdown for every Iceberg query:
>=previously readargs().get(0)and was never pushed; now it is.- AND/OR are now pushed.
- Successive filters are combined with AND.
- DATE constants become days, and TIME/TIMESTAMP constants become microseconds. Previously they were milliseconds, which could prune the wrong files.
These all look like genuine fixes, but there are no tests for any of them. When a format config sets ignoreResiduals: true, the newly pushed predicates mean rows that should have been filtered can come back.
Could you add translator unit tests and a few Iceberg query tests with the cache off? Cover >=, AND/OR, and date/timestamp filters. Alternatively, move these into their own PR so they can be reviewed separately. Either is fine with me.
There was a problem hiding this comment.
Added translator and cache-off query tests for >=, AND/OR and temporal values. Also fixed NULL handling and retained Drill filtering when ignoreResiduals=true.
| new DrillParseContext(PrelUtil.getPlannerSettings(filter.getCluster().getPlanner())), | ||
| filter.getInput(), | ||
| condition); | ||
| if (groupScan.getCondition() != null) { |
There was a problem hiding this comment.
Related to the translator comment: combining with the existing condition is the right behaviour, but it also affects queries with the cache off. A test with two stacked filters would pin it down.
There was a problem hiding this comment.
Added an implementor regression test for two stacked filters, such as id >= 2 AND id < 4. It checks values inside and outside the range to verify that both bounds are retained.
|
|
||
| // Plan exactly once, outside cache fallback. Validation can mutate the SQL tree, | ||
| // so retrying a failed planning attempt could reuse partially rewritten nodes. | ||
| PhysicalPlan planned = handler.getPlan(planningSql); |
There was a problem hiding this comment.
[Correctness] No fallback when the parameterized SQL fails to plan.
I understand why you don't want to retry on the same SqlNode, since validation mutates it. The fallback doesn't need to reuse it, though. On failure, set planner.enable_plan_cache=false for this query and call getQueryPlan again from the original SQL string. That parses a fresh tree.
Without this, queries that only plan with real literals fail only when the cache is on, with no workaround short of turning the cache off. Examples are ON TRUE, a function that needs a literal operand but isn't on the hard-coded list, and a new UDF.
There was a problem hiding this comment.
The two-line comment above getPlan is misleading and should be removed: it suggests that a failed planning attempt would retry a partially rewritten SQL tree, but that does not describe the current fallback path.
I checked the cases you raised.
- JOIN ... ON TRUE/FALSE works with the current parameterization; Drill’s Cartesian join setting applies equally with or without the cache.
- Functions that require a literal argument also work because the bound parameter retains the literal value through planning.
- A newly registered UDF takes the cache-miss path and can use Drill’s existing function-registry synchronization and planning retry. These cases do not require an additional fallback for parameterized SQL.
There was a problem hiding this comment.
Most functions work with the plan cache without special handling, including UDFs and functions that require literal arguments. We only need a special case when planning depends on the value of a literal.
For example, date_trunc uses its unit argument to choose a specific function during planning. The parameterizer must keep that unit in the template key. By contrast, now() is folded into a query-specific constant during planning, so the eligibility check excludes it from caching.
This gives us a narrow rule for reviewing functions: check whether the planner uses their values to shape the plan.
There was a problem hiding this comment.
My biggest concern are queries randomly failing and not giving the user a clear reason of what happened and how to fix it. But it sounds like that won't happen here, so that's good!
| return false; | ||
| } | ||
| // Confirm that standard PhysicalPlan JSON can be read back before publishing. | ||
| reader.readPhysicalPlan(json); |
There was a problem hiding this comment.
[Robustness] A bind that always fails cycles forever.
The read-back here proves the JSON parses, but not that Entry.bind succeeds. If a plan's bind always fails, each hit fails, invalidates and replans, and the entry is published again. An example is a non-expression text field that happens to contain bound_dynamic_param(.
A trial bind with the original literals before publishing would catch this. A small per-key failure counter that stops republishing would also work.
There was a problem hiding this comment.
Added trial binding before publication, replacing JSON-only validation. Unbindable templates—for example, ordinary text containing the parameter marker—are skipped without affecting the completed query.
| | 'false' | ||
| ; | ||
|
|
||
| BoundDynamicParam : 'bound_dynamic_param'; |
There was a problem hiding this comment.
Nit: this makes bound_dynamic_param a reserved word for all expression parsing, so an unquoted column or function with that name no longer parses. It's unlikely to matter, and date/as already behave this way. If you want to avoid it, match Identifier with a semantic predicate instead of a lexer token.
There was a problem hiding this comment.
Thanks for the suggestion. A semantic predicate could avoid this restriction. Since a naming conflict seems unlikely, I’d prefer to keep the simpler token-based approach for now and revisit it if a practical use case arises.
|
@cgivre Could you please take another look? I’ve narrowed the scope of this PR by removing the HBase and Iceberg integrations and their plugin-specific tests. The PR now uses single-file CSV/TSV scans to exercise the end-to-end cache path, with predicates remaining in Drill’s Filter operator. |
There was a problem hiding this comment.
Thanks very much for this contribution, Letian — this is a substantial and well-executed piece of work, and the design doc made it much easier to follow. I went through all 57 files carefully, and I want to say up front that we are very close to being able to merge this. What's below is a short list of things I'd like resolved, not a request for rework.
Should be addressed before merge
1. No fallback when parameterized planning fails — DrillSqlWorker.java:377
On a cache miss, planningSql is swapped to the parameterized tree and planned exactly once. The cache's try/catch (RuntimeException) ends before this line, so a failure here isn't degraded to ordinary planning — it surfaces to the user. That means any construct that plans fine with literals but not with bound dynamic params turns a working query into a failure as soon as planner.enable_plan_cache=true.
A concrete path: DrillOptiq.visitDynamicParam falls through to doUnknown (→ DrillRuntimeException) whenever RexBoundDynamicParam.getLiteral().accept(this) doesn't yield a LiteralExpression — e.g. a parameter Calcite types as DATE/TIME/TIMESTAMP, per item 2 below.
I see commit 3abcce8 removed the fallback deliberately, citing tree mutation during validation, so this needs a different fix rather than a revert — re-planning the original sqlNode with a fresh SqlConverter on failure would sidestep the mutated-tree problem. Given that this is the blast radius of a feature flag flip, I think a fallback path is worth having.
2. Temporal literals make the cache permanently unusable, and do so silently — PlanCache.java:770
bindExpression accepts LiteralExpression, FunctionCall/BooleanOperator, CastExpression, ConvertExpression, IfExpression, AnyValueExpression, SchemaPath, NullExpression and TypedNullConstant. But ValueExpressions.DateExpression, TimeExpression, TimeStampExpression, IntervalYearExpression, IntervalDayExpression and ParameterExpression were not reparented onto LiteralExpression in this PR, so any plan expression containing a temporal literal alongside a parameter hits the final throw.
For example:
SELECT * FROM dfs.tmp.`t.csv` WHERE name = 'bob' AND CAST(d AS DATE) > DATE '2020-01-01'The trial bind in put() always throws, so the entry is never published — and every execution then pays snapshot resolution plus parameterized planning for a cache that can never hit. Only logger.debug records it, so this is invisible in practice. Either extend bindExpression to pass these through unchanged, or exclude such expressions during eligibility so the overhead isn't paid.
Smaller items
3. Decimal visitors drop the bound-param marker — ExpressionStringBuilder.java:237
Decimal9/18/28/38Expression were changed to extend LiteralExpression so they can carry dynamicParamIndex, but their visitors — unlike visitIntConstant, visitVarDecimalConstant and friends — don't call startBoundLiteral/endBoundLiteral. If a bound parameter is ever materialized as one of these, the marker is dropped, collectTypes finds no slot in the reparsed template, rewrite returns the string unchanged, and the first query's literal value stays baked into the cached plan — wrong results for every later bind, with no error.
This is unreachable today (DrillOptiq.visitLiteral maps DECIMAL to VARDECIMAL or FLOAT8, and bindLiteral never builds these), so it's not a blocker. But it's a silent-wrong-answer failure mode rather than a loud one, and I'd rather it be loud: consider throwing from startBoundLiteral for any literal type whose visitor doesn't emit the wrapper.
4. Mutable dynamicParamIndex on process-wide singletons — ValueExpressions.java:184, :700
BooleanExpression.TRUE/FALSE and QuotedString.EMPTY_STRING are shared singletons that now inherit a mutable dynamicParamIndex, and setDynamicParamIndex mutates in place (called from DrillOptiq.visitDynamicParam and PlanCache.bindLiteral). QuotedString.EMPTY_STRING is handed out by the concat rewrite in DrillOptiq. If any path ever binds one of these instances, every expression in the JVM sharing it is permanently serialized as bound_dynamic_param(...). A withDynamicParamIndex(int) factory returning a copy, with the field final per instance, removes the hazard entirely.
5. bound_dynamic_param token collides with Identifier — ExprLexer.g4:97
BoundDynamicParam : 'bound_dynamic_param'; is declared before the Identifier rule and matches the same 19 characters, so ANTLR's earliest-rule tie-break makes the exact token win. A logical expression referencing a column or UDF literally named bound_dynamic_param — a SchemaPath in a serialized plan, or a LogicalExpressionParser.parse call from a provided-schema path — now fails to parse where it previously worked. A less collidable spelling, or placing the rule after Identifier behind a semantic predicate, avoids it.
6. ALTER SYSTEM CLEAR PLAN CACHE is node-local — ClearPlanCacheHandler.java:40
The statement is admin-gated and spelled with SYSTEM scope, but getPlanCache().clear() clears only the Drillbit that happened to plan it (the response message does say so). On a multi-node cluster an admin has no reliable way to invalidate stale plans, since repeated invocations land on whichever foreman the client connects to. Broadcasting via the control plane would be ideal; at minimum I'd document the per-node semantics in PLAN_CACHE_DESIGN.md.
Thanks again — nice work on this. Happy to take another look as soon as 1 and 2 are sorted.
@cgivre Thanks for the detailed review. I’d like to clarify the design choice around fallback and the temporal-parameter example. 1. General fallback I agree that retrying ordinary planning could protect users from parameterization regressions. However, we cannot reliably distinguish a parameterization failure from an error that the original SQL would also produce. A general fallback would therefore plan some invalid queries twice, potentially doubling their planning cost. A fresh parse tree and 2. The DATE/TIME/TIMESTAMP example The SQL parameterizer deliberately leaves DATE, TIME, TIMESTAMP and INTERVAL literals unchanged. Their values remain part of the cache key; other eligible literals in the same query can still be parameterized. Consequently, these temporal literals do not become 3. A parameterization bug we did find Further investigation did uncover a concrete issue: WHERE x > CAST('0' AS INTEGER)After parameterization, the existing CAST inference treated the bound parameter like an unbound parameter and assigned it the target type, INTEGER. Its stored value remained the string The fix preserves the bound parameter’s source string type and retains the outer CAST. On a cache hit, only the string value is replaced; CAST still performs the conversion. Regression tests verify that the CAST survives, compatible strings reuse the plan, and numeric literals do not select the string-parameter template. This is the kind of regression I would prefer to expose and fix rather than conceal behind a general retry. 4. Broader testing Differential SQL fuzzing could help expand coverage: generate queries and compare cache-disabled execution with cache-enabled misses and hits, including executions with different compatible literals. Comparing both results and failures would help identify parameterization regressions while distinguishing them from queries that already fail without caching. |
|
@letian-jiang Thanks, and you're right about the temporal example. I checked the code again: A DECIMAL slot can still get there by another route, though. Let me back up and say where my concern actually comes from. A lot of work has gone into Drill over the years to make queries behave the way users expect, and to make the errors clear enough that users know what to do about them when they don't. Plenty of that work was unglamorous: reworking messages, adding context, chasing down cases where Drill failed in a way nobody could act on. I don't want a plan cache to quietly undo it. A query that worked last week and now throws a On the fallback, then, I think we're arguing past each other, because distinguishing the two failure causes isn't the goal. Re-plan the original tree whenever parameterized planning throws, without trying to classify the exception. If the original SQL was broken to begin with, the user gets the identical error they'd have gotten with the cache off, a few milliseconds later; the doubled planning cost only ever lands on queries that were already failing. What I'm asking for is narrow: turning on an optimization must not change which queries succeed. Once an operator sets You don't have to give up that containment to keep the failures visible. Log the fallback at WARN with the parameterized SQL and the exception, and register a Your October QUALIFY example argues something different from what you're using it for. Silent wrong results need an eligibility fix, and no exception-triggered retry will ever catch them; I agree. That makes fallback insufficient rather than harmful. The retry contains exceptions, eligibility contains incorrect results, and doing both covers strictly more than doing either. Which brings me to the CAST bug in your point 3. Yes to differential fuzzing on point 4, and I'd like it to be a gate rather than a follow-up: comparing cache-off against miss and hit, over several compatible literal values per template, landing in CI before the plugin opt-ins start arriving. If that runs for a while across the existing test queries and turns up nothing new, it makes a far better case for exposing failures than the current test suite can. One question on scope now that the HBase and Iceberg integrations are out. With CSV/TSV single-file scans and predicates staying in the Filter operator, what do the benchmarks look like? The HBase numbers carried most of the argument for the feature, and I'd like to see what the narrowed PR claims on its own. |
|
@cgivre I’ve added a general fallback: when parameterized planning fails, Drill logs the SQL template and exception at WARN, then retries the original SQL with a fresh parser, converter and handler, bypassing the cache.
|
DRILL-8555: Physical plan cache for parameterized SQL queries
Description
This PR adds a Drillbit-scoped physical plan cache with HBase and Iceberg support, reusing plans across connections to reduce repeated validation and optimization. It is disabled by default; enable it with
ALTER SESSION SET planner.enable_plan_cache = true.Method
flowchart LR SQL[SQL] --> Template[SQL template] Template --> Cache{Plan cache} Cache -->|Hit| Bind[Bind literals] Cache -->|Miss| Planner[Plan query] Bind --> Plan[Physical plan] Planner --> PlanEligible literals become parameter slots in the SQL template. A hit binds current values to a fresh copy of the cached plan; a miss plans the query normally and populates the cache after successful execution.
Safety guarantees
Benchmark
Measured on one local Drillbit with a Ryzen 7 9700X, 30 GiB RAM and OpenJDK 21. HBase used a 1,000-row mini-cluster for point reads, 50-row range scans and column filters. Iceberg ran all 22 TPC-H queries over eight SF0.01 tables (Q15 used a derived table; Q19 exposed the common equijoin).
Cache hits
The benefit is largest when planning dominates latency: it accounts for roughly 73–76% of the reported HBase baseline latency, and hits reduce end-to-end latency by 50–56%. Analytical queries also benefit: Iceberg planning drops 83.6%, reducing aggregate end-to-end latency by 10.2%.
HBase values are medians of three run medians (15 pairs per workload per run). Iceberg values are sums of per-query medians (three pairs per query), not suite wall time. Percentages are latency reductions relative to cache-off execution.
Cache misses
A separate warmed comparison cleared the plan cache before each cache-on query and drained background writes before both modes. Misses added a median 3–5 ms of paired end-to-end latency for HBase (45 pairs per workload). For Iceberg, the sum of query end-to-end medians changed from 20,624 to 21,806 ms (+5.7%). Miss overhead was modest in these local measurements, while hits provided the largest benefit for short queries.
Documentation
PLAN_CACHE_DESIGN.md: basic principles and supported scope.PLAN_CACHE_PLUGIN_GUIDE.md: plugin APIs and scan reconstruction requirements.