>From Rithwik Koul <[email protected]>: Attention is currently required from: Ali Alsuliman, Ian Maxon, Murtadha Hubail, Murtadha Hubail.
Rithwik Koul 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 45: (25 comments) Patchset: PS45: > is this all the same as what's in APE 2 (https://cwiki.apache. […] Many things have changed since the first iteration outlined in APE2. Due to certain limitations, we have decided to only skip the optimization phase, as it is the most time-consuming part of the compilation process. Commit Message: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/88bdd17a_b397dd59?usp=email : PS45, Line 16: Ex-ref: MB-72415 > can we add Sushrut and perhaps Glenn as co-authors on this patch? or is it > not based on the previous […] Yes I'll add them File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/ClientRequest.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/e637a37f_f8d31c50?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 Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/IStatementExecutor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/1542a2cd_ac6ff2d7?usp=email : PS36, Line 171: planCacheHit > cachedPlan or something like that? Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/SessionConfig.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/1bfceafa_32c9a778?usp=email : PS36, Line 223: SessionConfig > We need to either filter the parameters that impact optimized plan or include > all properties. […] Done File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/APIFramework.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/3836425c_8f4c4667?usp=email : PS36, Line 154: private ILogicalPlan lastCompiledPlan; : private IOptimizationContext lastCompiledOptContext; > Create a wrapper for these (maybe a record) Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/04e5da3f_07539da4?usp=email : PS36, Line 353: if (cachedPlan != null) { > You don't need this block. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/93863cb5_593abef1?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 Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/3def7ecb_5963554d?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 Done 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/8bb3ce5b_546f9dbd?usp=email : PS36, Line 57: jsonResponse.put("entriesCleared", numberOfClearedCacheEntries); > we shouldn't report the same information twice Done File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cache/QueryCacheKey.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/97fc2aba_5df1b739?usp=email : PS36, Line 36: String userIdentity; > There is no concept of user currently Done File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cache/QueryCacheValue.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/feec0497_1325deb6?usp=email : PS36, Line 38: QueryCacheValue > This looks like a record Done 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/a74f4fda_316159d4?usp=email : PS36, Line 32: public class QueryCacheInvalidator { > I don't think this class is needed. […] Done File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/CcApplicationContext.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/afba7452_2b79bd10?usp=email : PS36, Line 128: QueryCache > This will probably be an IQueryCache after addressing the changes Done File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/ff45a9fb_622a022e?usp=email : PS36, Line 369: // Cast the getQueryCache() result, not appCtx: because in the Analytics runtime appCtx is a delegating : // AnalyticsCcApplicationContext > Remove this Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/69aa4a15_5839be31?usp=email : PS36, Line 371: (QueryCache) appCtx.getQueryCache(); > We shouldn't need this downcast if we remove the wild card from the query > cache Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/7df4930a_f88db6ed?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 Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/4ff40b2f_f5df451e?usp=email : PS36, Line 5683: stats.setPlanCacheHit(false); > should just default to false Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/ba236798_aede5908?usp=email : PS36, Line 5732: PlanToCache toCache > I don't like the fact that we are passing this empty value just to set here Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/9a9154da_3419b6aa?usp=email : PS36, Line 5751: stats.setPlanCacheHit(false) > This will default to false. […] Done File asterixdb/asterix-app/src/main/java/org/apache/asterix/utils/RebalanceUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/82ac259f_651b8e42?usp=email : PS36, Line 159: invalidatePlanCache(metadataProvider); > if we need to do this all the time, it should be in a finally block. Done File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/TestExecutor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/a72f7c89_a9b83ecb?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 Done File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/SqlppExecutionWithCacheTest.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21328/comment/86f4e4d2_304ffc71?usp=email : PS36, Line 45: SqlppExecutionWithCacheTest > we don' this. […] Done 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/a30723ab_fcc741f0?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 Done 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/be251658_5e2b883d?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 Done -- 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: 45 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: Ian Maxon <[email protected]> Gerrit-CC: Murtadha Hubail <[email protected]> Gerrit-Attention: Murtadha Hubail <[email protected]> Gerrit-Attention: Ian Maxon <[email protected]> Gerrit-Attention: Ali Alsuliman <[email protected]> Gerrit-Attention: Murtadha Hubail Gerrit-Comment-Date: Fri, 26 Jun 2026 06:34:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Murtadha Hubail <[email protected]> Comment-In-Reply-To: Ian Maxon <[email protected]>
