[
https://issues.apache.org/jira/browse/DRILL-8555?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18125057#comment-18125057
]
ASF GitHub Bot commented on DRILL-8555:
---------------------------------------
cgivre commented on PR #3086:
URL: https://github.com/apache/drill/pull/3086#issuecomment-6061539145
**Review findings**
Line numbers refer to the PR head. The first three are correctness issues
I'd like to see fixed before merge.
**Correctness**
1. **No fallback when planning the parameterized SQL fails**
(`DrillSqlWorker.java:354`). On a miss, the query is planned once from
`candidate.sql`. The `try/catch` covers lookup and preparation but not
`handler.getPlan(planningSql)`. Any query that only plans with real literals
then fails, but only when the cache is enabled. Examples: `JOIN ... ON TRUE`
(becomes `ON ?`), `PERCENTILE_CONT(0.5) WITHIN GROUP`, and functions needing a
literal operand that aren't on the hard-coded list. Please re-plan from the
original literal `SqlNode` on failure. The comment about validation mutating
the tree suggests keeping an unmodified copy (or re-parsing) for the fallback.
2. **HAVING/QUALIFY are parameterized while GROUP BY and the select list are
not** (`PlanCacheParameterizer.java:130`). `GROUP BY x || 'a' HAVING x || 'a' =
'ba'` becomes `HAVING x || ? = ?`, which no longer matches the grouped
expression. Calcite then rejects it with "Expression 'x' is not being grouped",
and because of #1 the query fails.
3. **Global change to BIGINT literal serialization**
(`ExpressionStringBuilder.java:191`). `visitLongConstant` now always emits
`cast(N as BIGINT)`, even with caching off. That changes EXPLAIN output, scan
digests and filter strings (e.g.
`AbstractGroupScanWithMetadata.getFilterString`) for everyone, and relies on a
special case in `ExprParser.createCast` to undo it. Could this be limited to
bound parameters?
**Performance / design**
4. **Context mismatch evicts instead of coexisting**
(`DrillSqlWorker.java:334`). The options/config fingerprint is checked inside
the entry rather than included in the key. Two sessions running the same
template with different options (e.g. `planner.slice_target`) invalidate each
other's entry on every run, so the hit rate is 0 and each query also pays to
republish. Including the fingerprints in the key fixes this.
5. **Remote metadata I/O on every lookup, including hits**
(`HBaseStoragePlugin.java:77`, `IcebergFormatPlugin.java:124`). HBase opens an
`Admin` and makes two master RPCs. Iceberg runs `HadoopTables.load`, and then
loads the table again in `IcebergGroupScan` during bind. A hit can cost more
than the planning it skips. Benchmarks of hit latency against planning time
would help.
6. **Entries are published without checking they can be rebound**
(`PlanCache.java:187`). `put()` only validates that the JSON reads back. An
entry whose `bind` always fails is published, fails on every hit, is
invalidated, gets replanned and is republished, forever. A trial bind before
publishing would catch this.
7. **The writer queue holds live plan graphs** (`PlanCache.java:162`). Up to
64 full `PhysicalPlan` object graphs (region maps, Iceberg task lists, plugin
references) stay alive until serialized. They're also serialized after
execution, which may have changed them. Serializing to a string right after
planning and queuing only the string avoids both.
8. **Hard-coded allowlist of literal-only function operands**
(`PlanCacheParameterizer.java:142`). Any function not on the list (format
strings, scale arguments, new UDFs) gets a slot and fails planning because of
#1. Could this come from operator operand metadata instead?
9. **On a miss, the executed plan loses literal-based optimization**
(`DrillSqlWorker.java:345`). `WHERE 1 = 1` and `flag = TRUE` become `? = ?` /
`flag = ?`, so constant reduction and selectivity estimates are lost for that
run. This happens even if the plan is never published.
**Observability and operations**
10. **No logging of cache activity.** Hits, misses, evictions and
context-mismatch invalidations aren't logged; only some failures are logged at
debug level. `PlanCache.getHitCount()` exists but nothing outside tests reads
it. Please add debug logging for hits, misses and invalidation reasons, and
expose hit/miss/eviction counts through Drill metrics or a `sys` table.
11. **No way to flush the cache.** Entries are only removed on expiry or
when found stale on lookup. Turning off `planner.enable_plan_cache` doesn't
clear them, and there's no admin command or REST endpoint. (Configurable size
and expiration are covered in my earlier comment.)
**Docs**
12. `PLAN_CACHE_DESIGN.md` and `PLAN_CACHE_PLUGIN_GUIDE.md` are at the repo
root. Developer docs belong under `docs/dev/` and should be linked from
`docs/dev/DevDocs.md`.
> 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
(v8.20.10#820010)
