Repository navigation
DRILL-8555: Physical plan cache for parameterized SQL queries - #3086
letian-jiang wants to merge 17 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.
| } | ||
| TableMetadata metadata = ((HasTableOperations) table).operations().current(); | ||
| return metadata == null || metadata.uuid() == null ? null | ||
| : new PlanCacheTable(identifier, metadata.uuid() + ":" + metadata.currentSchemaId()); |
There was a problem hiding this comment.
[Correctness] A change to the provided schema doesn't invalidate the entry.
The version is uuid:currentSchemaId. However, the cached IcebergGroupScan also carries Drill's provided schema from .drill.schema when store.table.use_schema_file=true. After CREATE OR REPLACE SCHEMA ... FOR TABLE, hits keep using the old types and casts. The options fingerprint only covers the flag, not the file contents.
Could you fold a hash of the provided schema into the version (or the schema file's modification time and length), or return null when a schema provider is present?
Separately, tables without a UUID silently never cache. That's safe, but worth a line in the plugin guide.
There was a problem hiding this comment.
Provided schema contents now contribute to table compatibility. For example, changing id VARCHAR to id BIGINT invalidates the old entry. Added tests with and without provided schemas, including native schema changes. Tables without an Iceberg UUID still bypass caching; this is now explicit in the plugin guide.
| String identifier = ((HBaseScanSpec) selection).getTableName(); | ||
| try (Table table = getConnection().getTable(TableName.valueOf(identifier))) { | ||
| String version = Hashing.sha256().hashBytes( | ||
| TableDescriptorBuilder.toByteArray(table.getDescriptor())).toString(); |
There was a problem hiding this comment.
[Performance] Thanks for removing tableExists here. The descriptor is still fetched about three times per query:
- once by the existing
AbstractHBaseDrillTablelookup, which also opens anAdminit never closes; - once here;
- once in
HBaseGroupScan.init()on a hit.
Could this hash the descriptor DrillHBaseTable already holds, or memoize it briefly? Iceberg has the same pattern: HadoopTables.load runs here and again in IcebergGroupScan.initTableScan.
There was a problem hiding this comment.
Agreed. I’ll leave further metadata reuse and the existing Admin lifetime issue for a follow-up optimization.
| } | ||
| HBaseFilterBuilder builder = new HBaseFilterBuilder(this, pushedFilter); | ||
| HBaseScanSpec rebuilt = builder.parseTree(); | ||
| if (rebuilt == null || (pushedFilterFullyConverted && !builder.isAllExpressionsConverted())) { |
There was a problem hiding this comment.
Minor: this rebuild runs whenever an HBase scan is read from JSON, not only on a cache bind, and a failure is now a hard IllegalArgumentException. Plans submitted as physical-plan JSON, or replayed from a profile, take this path too.
Could non-cache reads fall back to the serialized hbaseScanSpec? Also, TestHBasePlanCache uses a single region and fully converted predicates only. A test for a region split between warm-up and hit would be valuable, as would a partially converted filter and a value that flips full conversion to partial.
There was a problem hiding this comment.
Cache hits now require scan reconstruction; ordinary physical-plan JSON uses its saved scan specification. Added tests for JSON replay, region splits, partial filtering and parameter changes that require replanning.
| return dynamicParamIndex >= 0; | ||
| } | ||
|
|
||
| public void setDynamicParamIndex(int dynamicParamIndex) { |
There was a problem hiding this comment.
Minor hardening: this public mutator is inherited by shared singletons (BooleanExpression.TRUE/FALSE, QuotedString.EMPTY_STRING). It's unreachable today because DrillOptiq allocates fresh literals. But if anything ever returns a singleton and sets a slot on it, every query in the JVM would serialize that constant as a slot.
A withDynamicParamIndex(i) that returns a copy would make this impossible. Relatedly, the Decimal9–38 classes now extend LiteralExpression, but their serializers would silently drop a slot. Throwing for slotted types without a wrapper would be safer.
There was a problem hiding this comment.
Thanks for pointing this out. The current binding paths allocate fresh literals and use VARDECIMAL for bound decimals. Given that these cases seem unlikely in the current implementation, I’d prefer to keep the design simple for now and revisit the additional hardening if those paths expand.
| | '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.
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.