>From Preetham Poluparthi <preetha...@apache.org>: Attention is currently required from: Peeyush Gupta, Ali Alsuliman. Preetham Poluparthi has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763 )
Change subject: [ASTERIXDB-3632]: Add Index Advisor ...................................................................... Patch Set 32: (32 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AbstractIntroduceAccessMethodRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/e48435e6_c6dd5132 PS31, Line 118: setIndexProvider > Maybe we should have this done as part of setMetadataDeclarations()? Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceJoinAccessMethodRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/cdb7cff7_3e559336 PS31, Line 129: getFakeIndexProvider() > This can be null, right? When AdvisorPlanParser. […] Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/EnumerateJoinsRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/00c9ebdb_922dde89 PS31, Line 144: boolean adviseIndex = context.getIndexAdvisor().getAdvise(); > Move it to next where it's used. Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/indexadvisor/AdviseIndexRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/f9300517_5cb06bae PS31, Line 41: public class AdviseIndexRule > Can you add a documentation up that describes what this rule is about and the > purpose? Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/c8dab27f_fc8501a3 PS31, Line 57: / if > Remove Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/f77e8053_1b8c8e12 PS31, Line 70: ((AbstractLogicalOperator) opRef.getValue()).disableJobGen(); > Do you need this? Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/6100dbf0_cfd424a1 PS31, Line 88: LogicalOperatorTag.UNNEST_MAP > How about the left-outer case? […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/69e19f7b_2dce0f53 PS31, Line 94: UnnestingFunctionCallExpression unnestExpr = (UnnestingFunctionCallExpression) expr; > Use AccessMethodJobGenParams. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/272a8af1_b56aea5a PS31, Line 105: DataverseName.createSinglePartName > Use createFromCanonicalForm? Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/83bcdfff_cc4bb06d PS31, Line 108: (Index.ValueIndexDetails) > Shouldn't we check if it's actually of ValueIndexDetails before casting? > include array indexes in yo […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/7c104d17_085d292e PS31, Line 139: List<String> field = fields.get(i); > Check how it is presented when you have nested fields. Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/indexadvisor/AdvisorConditionParser.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/c43cd166_70983a4c PS31, Line 102: ILogicalExpression constantExpression = expr.getArguments().get(1).getValue(); > How about if the 0th argument is the constant expression? Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/86e9be43_08cbdb84 PS31, Line 128: e) ((ConstantExpression) functionCallExpr.getArguments() : .get(1).getValue()).getValue()).getObject()).getStringValue(); > See if you can call one of the methods in ConstantExpressionUtil Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/d6f7b4fb_418d8574 PS31, Line 179: for (int i = 0; i < assignOp.getVariables().size(); i++) { : OperatorManipulationUtil.replaceVarWithExpr((AbstractFunctionCallExpression) filterExpr.getValue(), : assignOp.getVariables().get(i), assignOp.getExpressions().get(i).getValue()); : } > Think about the case of a chain of assign operators. replaceExprsWithAssign is invoked in a top-down order. It processes the root assign operators first, followed by their children. This approach should prevent incomplete replacements. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/21b9f9e0_5c381bab PS31, Line 198: LogicalExpression lhs = expr.getArguments().get(0).getValue(); > Return null if the num args != 2 Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/indexadvisor/AdvisorPlanParser.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/d1ec32ed_bd66761a PS31, Line 67: return null; > Add missing { } in the if-blocks. There are few here. Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/indexadvisor/AdvisorScanPlanNode.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/6c1df9f4_09e94168 PS31, Line 54: List<AdvisorScanPlanNode> leafs = new ArrayList<>(); > Same thing if callers are not modifying, return a singleton collection Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/4ac0c984_16095794 PS31, Line 61: new ArrayList<>(); > If callers are not modifying the returned list, then return > Collections.emptyList(). Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/indexadvisor/FakeIndex.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/0494c720_ce2eecb9 PS31, Line 38: Collections.nCopies(fieldNames.size(), 0) > This is related to the meta record. […] Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/indexadvisor/FakeIndexProvider.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/1e359a85_0bcca44d PS31, Line 47: public FakeIndexProvider(CBOPlanStateTree planStateTree) > Is it possible to refactor each logic into a separate method instead of > having everything here? Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/d3d6dd37_b188a330 PS31, Line 48: Triple<String, DataverseName, String> > Use DatasetFullyQualifiedName Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/263869ed_2f28ef15 PS31, Line 55: DataSource dataSource = (DataSource) scanOperator.getDataSource(); > Use DatasetDataSource and get the required information from there. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/d713c50f_c4d356d3 PS31, Line 68: singleDataSourceFieldNamesMap.get(key).getSecond() > Can you reuse from above instead of looking up the value again? Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/23f2b187_d06f1a3a PS31, Line 91: LogicalVariable rhsVar = filterCondition.getRhsVar(); : DataSourceScanOperator rhsOp = planStateTree.getDataSourceScanVariableMap().get(rhsVar); > Refactor the below since it's the same as above. Done File asterixdb/asterix-app/src/test/resources/runtimets/results/cbo-join/index-advisor/join-advise/join-advise.4.adm: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/0e727cba_60c8bd20 PS31, Line 1: Default > We need to backtick those identifiers Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/6df3b254_e28bd852 PS31, Line 1: b_1 > Make sure this builds nested fields correctly with proper backticks. Done, added this in nested-fields test File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/Query.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/58c876a4_7f57fec1 PS31, Line 34: advise > Shouldn't we include "advise" in equals() and hashCode() similar to "explain"? Done File asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/d1289043_93b57073 PS31, Line 1017: ADVISE is not supported for this kind of statement > ADVISE is not supported for statement kind + stmt. […] Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/base/IndexAdvisor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/a72e5aa2_0dfefdaa PS31, Line 32: boolean isAdvise; > Make it final. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/36302d7e_66fc608b PS31, Line 41: mapper > If "mapper" isn't going to be used, replace it with new ObjectMapper() so > that we don't have to keep […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/08395000_33809aa7 PS31, Line 55: > Remove this space Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763/comment/6ab97a3a_4976c755 PS31, Line 117: // Enforced to be unique within a dataverse. > Remove Done -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19763 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: Iac773ebb49bdde18cfd340ef15d483dc324bdced Gerrit-Change-Number: 19763 Gerrit-PatchSet: 32 Gerrit-Owner: Preetham Poluparthi <preetha...@apache.org> Gerrit-Reviewer: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Peeyush Gupta <peeyush.gu...@couchbase.com> Gerrit-Reviewer: Preetham Poluparthi <preetha...@apache.org> Gerrit-Attention: Peeyush Gupta <peeyush.gu...@couchbase.com> Gerrit-Attention: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-Comment-Date: Fri, 08 Aug 2025 11:29:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-MessageType: comment
