[
https://issues.apache.org/jira/browse/DRILL-8555?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18123404#comment-18123404
]
ASF GitHub Bot commented on DRILL-8555:
---------------------------------------
cgivre commented on code in PR #3086:
URL: https://github.com/apache/drill/pull/3086#discussion_r4185387805
##########
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java:
##########
@@ -295,9 +301,116 @@ private static PhysicalPlan getQueryPlan(QueryContext
context, String sql, Point
context.getOptions().setLocalOption(ExecConstants.RETURN_RESULT_SET_FOR_DDL,
true);
}
+ if (context.getOptions().getOption(PlannerSettings.PLAN_CACHE)
+ && !context.getSession().hasTemporaryTables()
+ && !hasMutableAliases(context)
+ && PlanCacheEligibility.isSafeToCache(sqlNode,
context.getFunctionRegistry())) {
+ try {
+ PlanCacheParameterizer.Candidate candidate =
PlanCacheParameterizer.parameterize(sqlNode);
+ PlanCache cache = context.getDrillbitContext().getPlanCache();
+ String key = context.getQueryUserName() + '\n'
+ + context.getSession().getDefaultSchemaPath() + '\n'
+ + candidate.template;
+ PlanCache.Entry entry = cache.get(key);
+ String optionsFingerprint = null;
+ if (entry != null) {
+ try {
+ optionsFingerprint = optionFingerprint(context);
+ if (entry.matchesContext(optionsFingerprint,
context.getStorage())) {
+ PhysicalPlan bound = entry.bind(candidate.literals,
+ context.getDrillbitContext().getPlanReader());
+ if (textPlan != null) {
+ textPlan.value =
PlanCache.textWithBindings(entry.getTextPlan(), candidate.literals);
+ }
+ cache.recordBind();
+ context.setPlanCacheHit();
+ return bound;
+ }
+ } catch (RuntimeException | IOException e) {
+ logger.debug("Cached plan could not be rebound", e);
+ }
+ cache.invalidate(key);
+ }
+ // Capture the context before planning. Changes during planning
invalidate
+ // this entry on its first hit.
+ PlanCache.ContextSnapshot snapshot = PlanCache.ContextSnapshot.resolve(
+ parser.getDefaultSchema(), sqlNode,
+ context.getStorage());
+ if (snapshot != null) {
+ if (optionsFingerprint == null) {
+ optionsFingerprint = optionFingerprint(context);
+ }
+ snapshot = snapshot.withOptionsFingerprint(optionsFingerprint);
+ }
+ PhysicalPlan planned = handler.getPlan(candidate.sql);
+ final String templateTextPlan = textPlan == null ? null :
textPlan.value;
+ if (textPlan != null) {
+ textPlan.value = PlanCache.textWithBindings(templateTextPlan,
candidate.literals);
+ }
+ if (snapshot != null && cache.canCache(planned)) {
+ final org.apache.drill.exec.planner.PhysicalPlanReader cacheReader =
+ context.getDrillbitContext().getPlanReader();
+ final PlanCache.ContextSnapshot cacheContext = snapshot;
+ context.setPendingPlanCacheInsert(() -> cache.writeAfterSuccess(key,
planned,
+ templateTextPlan, cacheReader, cacheContext));
+ }
+ return planned;
+ } catch (Exception e) {
Review Comment:
This also swallows genuine planning failures from
`handler.getPlan(candidate.sql)` (validation errors, planner timeouts), so
failing queries are planned twice. The retry reuses `sqlNode` subtrees that the
first validation already rewrote in place.
Fix: catch only cache-specific failures here (or rethrow
`ValidationException`/`UserException`/planner timeouts).
##########
logical/src/main/java/org/apache/drill/common/expression/ExpressionStringBuilder.java:
##########
@@ -147,7 +197,16 @@ public Void visitSchemaPath(SchemaPath path, StringBuilder
sb) throws RuntimeExc
@Override
public Void visitLongConstant(LongExpression lExpr, StringBuilder sb) throws
RuntimeException {
- sb.append(lExpr.getLong());
+ if (startBoundLiteral(lExpr, sb)) {
+ return null;
+ }
+ if (lExpr.getDynamicParamIndex() < 0) {
+ // Small BIGINT values otherwise parse back as INT during a plan round
trip.
+ sb.append("cast(").append(lExpr.getLong()).append(" as BIGINT)");
Review Comment:
This applies to all plans, not just cached ones: after a JSON round trip,
BIGINT literals come back as `CastExpression`. `DrillExprToPaimonTranslator`
has no `visitCastExpression`, so in multi-fragment plans the Paimon predicate
is dropped after Drill has already removed the Filter, and `bigint_col = 5`
returns unfiltered rows. Only the Iceberg translator was updated.
Fix: either unwrap cast-of-literal in the pushdown translators (Paimon and
any others), or have the parser read the round-tripped value back as a BIGINT
literal instead of emitting a cast.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java:
##########
@@ -295,9 +301,116 @@ private static PhysicalPlan getQueryPlan(QueryContext
context, String sql, Point
context.getOptions().setLocalOption(ExecConstants.RETURN_RESULT_SET_FOR_DDL,
true);
}
+ if (context.getOptions().getOption(PlannerSettings.PLAN_CACHE)
+ && !context.getSession().hasTemporaryTables()
+ && !hasMutableAliases(context)
+ && PlanCacheEligibility.isSafeToCache(sqlNode,
context.getFunctionRegistry())) {
+ try {
+ PlanCacheParameterizer.Candidate candidate =
PlanCacheParameterizer.parameterize(sqlNode);
+ PlanCache cache = context.getDrillbitContext().getPlanCache();
+ String key = context.getQueryUserName() + '\n'
+ + context.getSession().getDefaultSchemaPath() + '\n'
+ + candidate.template;
+ PlanCache.Entry entry = cache.get(key);
+ String optionsFingerprint = null;
+ if (entry != null) {
+ try {
+ optionsFingerprint = optionFingerprint(context);
+ if (entry.matchesContext(optionsFingerprint,
context.getStorage())) {
+ PhysicalPlan bound = entry.bind(candidate.literals,
+ context.getDrillbitContext().getPlanReader());
+ if (textPlan != null) {
+ textPlan.value =
PlanCache.textWithBindings(entry.getTextPlan(), candidate.literals);
+ }
+ cache.recordBind();
+ context.setPlanCacheHit();
+ return bound;
+ }
+ } catch (RuntimeException | IOException e) {
+ logger.debug("Cached plan could not be rebound", e);
+ }
+ cache.invalidate(key);
+ }
+ // Capture the context before planning. Changes during planning
invalidate
+ // this entry on its first hit.
+ PlanCache.ContextSnapshot snapshot = PlanCache.ContextSnapshot.resolve(
+ parser.getDefaultSchema(), sqlNode,
+ context.getStorage());
+ if (snapshot != null) {
+ if (optionsFingerprint == null) {
+ optionsFingerprint = optionFingerprint(context);
+ }
+ snapshot = snapshot.withOptionsFingerprint(optionsFingerprint);
+ }
+ PhysicalPlan planned = handler.getPlan(candidate.sql);
Review Comment:
This plans from the parameterized SQL even when `snapshot == null`, i.e. for
queries that can never be cached. Two consequences once
`planner.enable_plan_cache=true`:
1. JDBC: `JdbcExpressionCheck.visitDynamicParam` returns true, so `WHERE id
= ?` is pushed down and `JdbcBatchReader` executes it with no bound value.
`SELECT * FROM mysql.db.t WHERE id = 5` fails at execution time, past the
fallback.
2. Plugins that require literals lose pushdown: `FindPartitionConditions`
returns `NO_PUSH` (no `dir0` pruning), Mongo, etc. These queries pay the cost
with no cache benefit.
Fix: only plan from `candidate.sql` when `snapshot != null` and all tables
are cacheable; otherwise use `handler.getPlan(sqlNode)`.
> 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)
