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