[
https://issues.apache.org/jira/browse/DRILL-8555?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18139574#comment-18139574
]
ASF GitHub Bot commented on DRILL-8555:
---------------------------------------
letian-jiang commented on PR #3086:
URL: https://github.com/apache/drill/pull/3086#issuecomment-6683662587
> 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.
@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 `SqlConverter` would address the mutation concern,
but would not resolve this distinction. It could also mask parameterization
bugs: queries would succeed through fallback, making those regressions harder
to detect and fix. For these reasons, I prefer to keep parameterized-planning
failures visible and fix their causes directly.
**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 `RexBoundDynamicParam`
nodes and do not reach the `visitDynamicParam` failure described here. Temporal
parameter binding is outside the current supported scope.
**3. A parameterization bug we did find**
Further investigation did uncover a concrete issue:
```sql
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 `"0"`, so conversion attempted to
construct an INTEGER literal from a Java String and failed.
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.
> Physical plan cache for parameterized SQL queries
> -------------------------------------------------
>
> Key: DRILL-8555
> URL: https://issues.apache.org/jira/browse/DRILL-8555
> Project: Apache Drill
> Issue Type: New Feature
> Components: Query Planning & Optimization
> Reporter: Letian Jiang
> Priority: Major
>
> h2. Motivation
> Repeated queries that differ only in literals are a common workload, yet each
> query currently requires full planning. This adds unnecessary latency and
> wastes CPU time.
> h2. Brief Design
> Add a per-Drillbit, in-memory plan cache. Drill automatically parameterizes
> eligible SQL queries and reuses a cached physical plan with the current
> literal values, without changes to client queries.
> h2. Plan reuse safety
> * *Engine:* matches the SQL template and checks relevant option changes,
> plugin configuration changes, and whether base table versions still match.
> * *Storage plugin:* explicitly opts in, provides a plan-compatible table
> version, and is responsible for rebuilding its predicate-dependent scan work
> from the rebound expressions.
> h2. Expected benefit
> Short, frequently executed queries such as HBase reads can benefit
> substantially. Analytical queries with significant planning cost can benefit
> as well.
> h2. Implementation status
> A working implementation is available, including HBase, Paimon and Iceberg
> integrations. A pull request will follow.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
