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.
--
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]
