>From Janhavi Tripurwar <[email protected]>: Attention is currently required from: Ali Alsuliman, Murtadha Hubail, Peeyush Gupta.
Janhavi Tripurwar has posted comments on this change by Janhavi Tripurwar. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441?usp=email ) Change subject: [ASTERIXDB-3665]: Compute RangeMap at Compile time ...................................................................... Patch Set 30: Verified+1 (15 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/RuleCollections.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/2c7c2c10_7762f958?usp=email : PS8, Line 408: physicalRewritesAllLevels.add(new CheckFullParallelSortRule()); > We can rename CheckFullParallelSortRule to AttachCompileTimeRangeMapRule and > put the logic there ins […] yes, done. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/AttachCompileTimeRangeMapRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/faa7e461_12ea298d?usp=email : PS8, Line 44: return true; > Return true if we changed the plan, false if nothing has changed. yes. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/JoinEnum.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/c9a20bff_fb01329a?usp=email : PS7, Line 1051: attachStaticRangeMapIfNeeded(); > What if CBO is disabled at the query level but we do have samples? (i.e. […] made the changes. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/6718f536_94cc9ca0?usp=email : PS7, Line 1072: for (Pair<OrderOperator.IOrder, Mutable<ILogicalExpression>> p : rootOrd.getOrderExpressions()) { > This is the same loop as the above. […] yes, done. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/JoinEnum.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/d05b4b1f_48df1abc?usp=email : PS8, Line 1045: RangeMapUtil.attachStaticRangeMapIfNeeded(rootOrderByOp, optCtx); > We don't need this anymore, right? We don't in cbo, taken care of in the AttachCompileTimeRangeMapRule rule. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/RangeMapUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/8c75e7ec_3151931a?usp=email : PS8, Line 200: public static void attachStaticRangeMapIfNeeded(ILogicalOperator rootOrderByOp, IOptimizationContext optCtx) > Do we check that we have samples? yes. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/Stats.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/3c52e02f_a2e43147?usp=email : PS7, Line 932: RangeMapUtil rangeMapUtil = new RangeMapUtil(BuiltinFunctions.RANGE_MAP, BuiltinFunctions.LOCAL_SAMPLING, > Those could be used directly from inside the RangeMapUtil class. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/25c5c3ed_83180dc1?usp=email : PS7, Line 938: ILogicalOperator parent = joinEnum.findDataSourceScanOperatorParent(newRoot); > What if we have a join? don't we need to replace both of the data-scans? yes, done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/66146df4_145e77ae?usp=email : PS7, Line 948: List<LogicalVariable> outVars = new ArrayList<>(sortExprs.size()); > Why not only use partitionFields? partitionFields should be enough, right? Yes, should be enough. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/fa38220e_1805b1f4?usp=email : PS7, Line 994: private RangeMap deserializeRangeMap(List<List<IAObject>> val) throws AlgebricksException { > We should probably have this logic (both the serialization and > deserialization) as part of the Range […] moved the logic to RangeMapUtil. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/c5ca4fb1_925117c9?usp=email : PS7, Line 996: ABinary ab = val.get(0).get(0) instanceof ANull ? null : ((ABinary) val.get(0).get(0)); > Is this ever going to be null? if yes, then it's going to cause NPE. […] not needed File asterixdb/asterix-app/src/test/resources/optimizerts/queries/btree-index/btree-secondary-68_ps.sqlpp: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/ab812063_7a6d7ca0?usp=email : PS8, Line 49: analyze dataset Orders; > We don't need set `compiler. […] We don't. We have deleted these files (_ps). File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/physical/AbstractStableSortPOperator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/e8bd2cd4_7abd210c?usp=email : PS7, Line 162: private String getFullParallelAnnotation(AbstractLogicalOperator sortOp, INodeDomain clusterDomain, > We could simplify this now, right? we could just remove this method and > instead use getRangeMap() di […] Yes, we can. But we still needs some checks. File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/EnforceStructuralPropertiesRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/ac9a0ce6_0d7a6ef1?usp=email : PS8, Line 688: return null; > We shouldn't reach here, correct? Maybe throw COMPILATION_ERROR saying no > range map found for order […] Done File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/util/RangeMapUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/254d5e56_97f961cf?usp=email : PS7, Line 122: boolean isTwoStep = buildRangeMapAtCompileTime ? false : true; > Remind me to ask you about this flag. Resolving at we discussed this. -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441?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: I51f99463d24596e74b5d435f718236de9016f2e8 Gerrit-Change-Number: 20441 Gerrit-PatchSet: 30 Gerrit-Owner: Janhavi Tripurwar <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Janhavi Tripurwar <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Peeyush Gupta <[email protected]> Gerrit-CC: Murtadha Hubail <[email protected]> Gerrit-Attention: Murtadha Hubail <[email protected]> Gerrit-Attention: Peeyush Gupta <[email protected]> Gerrit-Attention: Ali Alsuliman <[email protected]> Gerrit-Comment-Date: Mon, 08 Dec 2025 05:03:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Ali Alsuliman <[email protected]>
