letian-jiang commented on code in PR #3086:
URL: https://github.com/apache/drill/pull/3086#discussion_r4386471161
##########
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java:
##########
@@ -295,7 +305,95 @@ private static PhysicalPlan getQueryPlan(QueryContext
context, String sql, Point
context.getOptions().setLocalOption(ExecConstants.RETURN_RESULT_SET_FOR_DDL,
true);
}
- return handler.getPlan(sqlNode);
+ // Keep the original SQL if no safe snapshot is available or cache
preparation fails.
+ SqlNode planningSql = sqlNode;
+ Consumer<PhysicalPlan> prepareCacheInsert = null;
+ if (context.getOptions().getOption(PlannerSettings.PLAN_CACHE)
+ && !context.getSession().hasTemporaryTables()
+ && !hasMutableAliases(context)
+ && PlanCacheEligibility.isSafeToCache(sqlNode,
context.getFunctionRegistry())) {
+ try {
+ PlanCache cache = context.getDrillbitContext().getPlanCache();
+ long cacheGeneration = cache.getGeneration();
+ // Capture the context once, before lookup and planning. Changes during
+ // planning invalidate this entry on its first hit.
+ PlanCache.ContextSnapshot snapshot = PlanCache.ContextSnapshot.resolve(
+ parser.getDefaultSchema(), sqlNode, context);
+ if (snapshot != null) {
+ PlanCacheParameterizer.Candidate candidate =
+ PlanCacheParameterizer.parameterize(sqlNode,
parser.getTypeFactory());
+ String key = context.getQueryUserName() + '\n'
+ + context.getSession().getDefaultSchemaPath() + '\n'
+ + snapshot.keyFingerprint() + '\n'
+ + candidate.template;
+ PlanCache.Entry entry = cache.get(key);
+ if (entry != null) {
+ try {
+ if (entry.matchesContext(snapshot)) {
+ PhysicalPlan bound = entry.bind(candidate.literals,
+ context.getDrillbitContext().getPlanReader());
+ PlanCache.bindTextPlan(textPlan, entry.getTextPlan(),
candidate.literals);
+ cache.recordBind();
+ context.setPlanCacheHit();
+ logger.debug("Plan cache hit");
+ return bound;
+ }
+ logger.debug("Plan cache entry invalidated because its context
changed; replanning");
+ } catch (RuntimeException | IOException e) {
+ logger.debug("Plan cache entry invalidated because rebinding
failed; replanning", e);
+ }
+ cache.invalidate(key);
+ } else {
+ logger.debug("Plan cache miss");
+ }
+ cache.recordMiss();
+ final PhysicalPlanReader cacheReader =
+ context.getDrillbitContext().getPlanReader();
+ // Prepare insertion after planning; Foreman publishes it only after
the query succeeds.
+ prepareCacheInsert = planned -> {
+ String templateTextPlan = PlanCache.bindTextPlan(textPlan,
candidate.literals);
+ final String json;
+ try {
+ // Capture before parallelization/execution mutates the
operators.
+ // Pending publication and the writer queue retain only
immutable JSON.
+ json = cacheReader.writeJson(planned);
+ } catch (IOException | RuntimeException e) {
+ logger.debug("Plan could not be serialized for the plan cache;
skipping publication", e);
+ return;
+ }
+ context.setPendingPlanCacheInsert(() ->
cache.writeAfterSuccess(key, json,
+ templateTextPlan, cacheReader, snapshot, cacheGeneration));
+ };
+ // On a cache miss, plan the parameterized SQL to produce a reusable
template.
+ planningSql = candidate.sql;
+ }
+ } catch (RuntimeException e) {
+ logger.debug("Plan cache attempt fell back to ordinary planning", e);
+ }
+ }
+
+ // Plan exactly once, outside cache fallback. Validation can mutate the
SQL tree,
+ // so retrying a failed planning attempt could reuse partially rewritten
nodes.
+ PhysicalPlan planned = handler.getPlan(planningSql);
Review Comment:
The two-line comment above getPlan is misleading and should be removed: it
suggests that a failed planning attempt would retry a partially rewritten SQL
tree, but that does not describe the current fallback path.
I checked the cases you raised.
1. JOIN ... ON TRUE/FALSE works with the current parameterization; Drill’s
Cartesian join setting applies equally with or without the cache.
2. Functions that require a literal argument also work because the bound
parameter retains the literal value through planning.
3. A newly registered UDF takes the cache-miss path and can use Drill’s
existing function-registry synchronization and planning retry. These cases do
not require an additional fallback for parameterized SQL.
--
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]
