letian-jiang commented on code in PR #3086:
URL: https://github.com/apache/drill/pull/3086#discussion_r4193014085
##########
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:
Thanks for catching this. The context snapshot is now resolved from the
original SQL before parameterization, including checking that all participating
plugins support plan caching for their table selections. If the snapshot cannot
be established, planning uses the original SQL with its literals intact.
Parameterized planning is only used when a valid snapshot is available.
##########
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:
Agreed. I moved `handler.getPlan(...)` outside the cache fallback block, so
cache lookup or binding failures can fall back before planning, while planning
failures propagate without a cache-induced retry.
--
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]
