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