>From Murtadha Hubail <[email protected]>: Attention is currently required from: Ali Alsuliman, Murtadha Hubail, Rithwik Koul.
Murtadha Hubail has posted comments on this change by Rithwik Koul. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328?usp=email ) Change subject: [ASTERIXDB-3183][COMP] Add query plan cache ...................................................................... Patch Set 36: (26 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/ClientRequest.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/98f67002_89f1a303?usp=email : PS36, Line 175: getUserKey Not sure if we should introduce a user key here as we don't have a concept of a user File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/IStatementExecutor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/948d287d_de9fdcec?usp=email : PS36, Line 171: planCacheHit cachedPlan or something like that? File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/SessionConfig.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/4a221338_01e09cee?usp=email : PS36, Line 223: SessionConfig We need to either filter the parameters that impact optimized plan or include all properties. Not sure if this should be a constructor yet. File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/APIFramework.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/39007996_fb884fee?usp=email : PS36, Line 154: private ILogicalPlan lastCompiledPlan; : private IOptimizationContext lastCompiledOptContext; Create a wrapper for these (maybe a record) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/d5768d12_cd6a0ec7?usp=email : PS36, Line 353: if (cachedPlan != null) { You don't need this block. Just use the else part https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/b9188b02_a7036b05?usp=email : PS36, Line 364: IOptimizationContext cachedOptContext = cachedPlan.getOptContext(); : IOptimizationContext optContext = : OptimizationContextFactory.INSTANCE.cloneOptimizationContext(cachedOptContext); : optContext.setMetadataDeclarations(metadataProvider); : optContext.setCompilerFactory(compilerFactory); : compiler = compilerFactory.createCompiler(plan, optContext, ruleSetKind, : PrinterBasedWriterFactory.INSTANCE); Refactor this to something like createdCachedPlanCompiler https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/430e1aff_c8183660?usp=email : PS36, Line 472: // Capture the optimized plan and its optimization context so handleQuery can store both in the : // query plan cache; a later hit clones the context to skip rewrite/optimize and type re-inference. Remove this File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/PlanCacheApiServlet.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/e3d2b960_f35a0e5c?usp=email : PS36, Line 57: jsonResponse.put("entriesCleared", numberOfClearedCacheEntries); we shouldn't report the same information twice File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cache/QueryCache.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/5acee962_f82389d6?usp=email : PS36, Line 29: Map Not sure if we need to implement Map here or just have the map as a variable File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cache/QueryCacheKey.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/5c2c30ff_32c2ac54?usp=email : PS36, Line 36: String userIdentity; There is no concept of user currently https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/536e3cfa_ab0d769f?usp=email : PS36, Line 38: QueryCacheKey As discussed, we need to find all variables that can impact the optimized plan File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cache/QueryCacheValue.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/d353130b_ec53d83b?usp=email : PS36, Line 38: QueryCacheValue This looks like a record File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cache/clear/QueryCacheInvalidator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/27343e3e_0607303a?usp=email : PS36, Line 32: public class QueryCacheInvalidator { I don't think this class is needed. Maybe those should be part of the query cache API File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/CcApplicationContext.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/b99d79a6_d70d787f?usp=email : PS36, Line 128: QueryCache This will probably be an IQueryCache after addressing the changes File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/1dc2c6ef_d198b98b?usp=email : PS36, Line 369: // Cast the getQueryCache() result, not appCtx: because in the Analytics runtime appCtx is a delegating : // AnalyticsCcApplicationContext Remove this https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/c3c483e6_7b003002?usp=email : PS36, Line 371: (QueryCache) appCtx.getQueryCache(); We shouldn't need this downcast if we remove the wild card from the query cache https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/a231c7c5_6c95b3eb?usp=email : PS36, Line 5673: boolean isPlanCacheEnabled = : metadataProvider.getBooleanProperty(CompilerProperties.COMPILER_QUERY_PLAN_CACHE_KEY, : appCtx.getCompilerProperties().isQueryPlanCacheEnabled()); : boolean ignoreCache = !isPlanCacheEnabled || sessionConfig.is(SessionConfig.SKIP_QUERYPLAN_CACHE) : || requestParameters.getStatementParameters() != null; refactor this https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/8060c499_f7f0c0ec?usp=email : PS36, Line 5683: stats.setPlanCacheHit(false); should just default to false https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/ecba0229_618086b0?usp=email : PS36, Line 5724: PlanToCache Do we really need this? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/fa369ad0_9641145c?usp=email : PS36, Line 5732: PlanToCache toCache I don't like the fact that we are passing this empty value just to set here https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/b2ac1af9_9528a549?usp=email : PS36, Line 5751: stats.setPlanCacheHit(false) This will default to false. We should only care about setting it to true when it's a cache hit File asterixdb/asterix-app/src/main/java/org/apache/asterix/utils/RebalanceUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/8e12e7ae_e616e763?usp=email : PS36, Line 159: invalidatePlanCache(metadataProvider); if we need to do this all the time, it should be in a finally block. File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/TestExecutor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/55fbf6a0_51b1fe91?usp=email : PS36, Line 2280: int loopIterations = 1; : // run query files 3 times to test the query plan cache : if (testCaseCtx.isTestCache() && testCaseCtx.isFileRepeated(ctx)) { : loopIterations = 3; : } : for (int i = 0; i < loopIterations; i++) { : BitSet warningsCopy = (BitSet) testCaseCtx.expectedWarnings.clone(); : executeTestFile(testCaseCtx, ctx, variableCtx, statement, isDmlRecoveryTest, pb, cUnit, : queryCount, expectedResultFileCtxs, testFile, actualPath); : if (i < loopIterations - 1) { : queryCount.decrement(); : testCaseCtx.expectedWarnings = warningsCopy; : } : } We shouldn't need this File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/SqlppExecutionWithCacheTest.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/d37f5919_d16e2453?usp=email : PS36, Line 45: SqlppExecutionWithCacheTest we don' this. We can just add tests File asterixdb/asterix-test-framework/src/main/java/org/apache/asterix/testframework/context/TestCaseContext.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/25e1a1cb_57eea609?usp=email : PS36, Line 91: blic boolean isTestCache() { : return isTestCache; : } : : public boolean isFileRepeated(TestFileContext testFileContext) { : return repeatFilePredicate.test(testFileContext); : } : : public void setRepeatFilePredicate(Predicate<TestFileContext> repeatFilePredicate) { : this.isTestCache = true; : this.repeatFilePredicate = repeatFilePredicate; : } I don't think we need those File hyracks-fullstack/algebricks/algebricks-compiler/src/main/java/org/apache/hyracks/algebricks/compiler/api/HeuristicCompilerFactoryBuilder.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/4f0e869d_1c49ba18?usp=email : PS36, Line 112: IRuleSetKind ruleSetKind) { : if (newOptContext.getCompilerFactory() != this) { : throw new IllegalStateException(); : } : return new CompilerImpl(this, plan, newOptContext, logicalRewritesByKind.apply(ruleSetKind), : physicalRewrites.get(), SerializedDataWriterFactory.WITHOUT_RECORD_DESCRIPTOR); this should reuse the new method -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328?usp=email To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: I116e5fa9d16e5c4a5cc6dfbdf8db230221086a9d Gerrit-Change-Number: 21328 Gerrit-PatchSet: 36 Gerrit-Owner: Rithwik Koul <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Murtadha Hubail Gerrit-CC: Murtadha Hubail <[email protected]> Gerrit-Attention: Rithwik Koul <[email protected]> Gerrit-Attention: Ali Alsuliman <[email protected]> Gerrit-Attention: Murtadha Hubail Gerrit-Comment-Date: Mon, 22 Jun 2026 11:43:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No
