cgivre commented on PR #3086:
URL: https://github.com/apache/drill/pull/3086#issuecomment-6687984850
@letian-jiang Thanks, and you're right about the temporal example. I checked
the code again: `PlanCacheParameterizer.visit(SqlLiteral)` only opens a slot
for `BigDecimal`, `Boolean` and `NlsString`, and
`DrillSqlValidator.inferUnknownTypes` returns early for a
`SqlBoundDynamicParam` so the slot keeps its own literal's type and Calcite
never coerces it to DATE. My example doesn't reach `visitDynamicParam`.
Withdrawn.
A DECIMAL slot can still get there by another route, though.
`DrillSqlToRelConverter` calls `rexBuilder.makeLiteral(literalValue,
valueType)` with a DECIMAL type, and with `planner.enable_decimal_data_type`
off, `DrillOptiq.visitLiteral` takes its unsupported-type path. Same shape of
problem, visible only when the cache is on. I haven't built a failing case for
it yet, so treat that as a question rather than a claim. But if you believe
nothing can reach `doUnknown` from a bound parameter, please make that
explicit: throw `IllegalStateException` with the parameter type in the message,
or drop the branch. Falling through quietly to a `DrillRuntimeException` reads
like a path you expect to be taken.
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 `DrillRuntimeException` out of `DrillOptiq` because a cluster-wide
option got flipped is exactly the kind of error a user can't diagnose or fix,
and they have no reason to suspect caching at all.
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 `planner.enable_plan_cache`
at the system level, every query on that Drillbit inherits the risk, and "we'll
fix the causes as they surface" is a promise about the whole SQL surface rather
than the slice the tests cover.
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 `parameterization_failures` counter beside the
`hits`/`misses`/`invalidations` gauges you already added under
`drill.plan_cache.`. Operators get something they can alert on, developers get
something they can grep, and users get their query. If you want the loud
version during development, a hidden option that rethrows instead of falling
back would give the fuzzer and the regression suite a hard failure to assert on.
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. `WHERE x > CAST('0' AS
INTEGER)` is the strongest case for fallback anyone has made in this thread.
It's an ordinary predicate, it fails only with caching enabled, and it got past
seven new test classes until someone read the inference code closely. I don't
doubt you'll fix each one you find. I doubt we'll find them all before users do.
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.
--
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]
