>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 34: Verified+1 (16 comments) Patchset: PS31: > Do not forget to rebase the patch as well Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/compiler/provider/DefaultRuleSetFactory.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/2328d916_ce239f5a?usp=email : PS30, Line 54: } else if (ruleSetKind == RuleSetKind.RANGE_SAMPLING) { > We should use RuleSetKind. […] Yes, changed it after our discussion. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/AttachCompileTimeRangeMapRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/77132ab8_2e69345c?usp=email : PS31, Line 77: if (orderOp.getOperatorTag() == LogicalOperatorTag.ORDER > It is cleaner if you do all of this like: […] Refactored. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/1ecb499b_d2be3147?usp=email : PS31, Line 120: if (orderOp.getAnnotations().containsKey(OperatorAnnotations.USE_STATIC_RANGE) > This must be always false because we only get here when there is no > USE_STATIC_RANGE. […] yes, its redundant. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/bf3c5c97_df2cb0d7?usp=email : PS31, Line 139: if (scanOps.isEmpty()) { > Move this check up right after calling collectDataSourceScanOperators() Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/c69d9d0d_bf45d550?usp=email : PS31, Line 146: while (op != null && (op.getOperatorTag() == LogicalOperatorTag.EXCHANGE > Not sure how this while-loop can determine if it is "single tuple" or not. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/9e7bc359_d8a2c05e?usp=email : PS31, Line 156: return ((AggregateOperator) op).isGlobal(); > Are you sure this is correct meaning isGlobal() indicates a single tuple > output? This is not needed, removed it. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/734b55a9_a8257f9e?usp=email : PS31, Line 178: private static boolean hasIntervalMergeJoin(ILogicalOperator op) { > Add {} in the if-blocks. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/878b972a_1adbfb47?usp=email : PS31, Line 183: if (pop != null && pop.getOperatorTag().toString().contains("INTERVAL_MERGE_JOIN")) { > Why toString()? Why not compare the enum directly? Yes, enum can be compared directly. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/OperatorUtils.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/8ec383f0_523a22b9?usp=email : PS31, Line 410: return null; > Add {} Done 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/fc1d308e_06af007b?usp=email : PS31, Line 270: String plan = new ALogicalPlanImpl(topRef).toString(); > Why is this here? This toString() is expensive. It was just for debugging purpose, removed it. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/c238b6a9_f8fe62a9?usp=email : PS31, Line 280: DataSourceScanOperator scan; > DataSourceScanOperator scan = (DataSourceScanOperator) op; Done File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/java/AsterixJavaClient.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/8f3be2bc_85f81376?usp=email : PS30, Line 130: Set<String> alreadyInserted = new HashSet<>(); > It's better to add ANALYZE only for the optimizer tests instead of for all > calls of "compile()". […] Done File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/MetadataProvider.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/4bc0845b_184e7f6f?usp=email : PS30, Line 236: maxResultReads = DEFAULT_MAX_RESULT_READS; > What is this for? consider this query: insert into DBLP1 select element { 'id': o.id, 'dblpid': o.dblpid, 'title': o.title, 'authors': o.authors, 'misc': o.misc } from DBLP as o where test.contains(o.title, 'Multimedia') order by o.id; This is an INSERT with RETURNING. When it follows the normal execution path, it correctly picks up maxResultReads. However, when we compute the range map, the sampling plan for that step does not go through the insert/upsert settings logic. As a result, maxResultReads falls back to the default value of 0, which causes the following error: java.lang.IllegalArgumentException: maxReads must be >= 1 or -1 for unlimited reads 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/54aaac0d_697c6f3e?usp=email : PS30, Line 90: && targetNodeDomain.cardinality() != null && targetNodeDomain.cardinality() > 1; > Why did we remove ctx.getPhysicalOptimizationConfig(). […] We can add that check but it's not necessary, if RangeMap rm is not null it already implies that ctx.getPhysicalOptimizationConfig().getSortParallel() is true File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/plan/PlanStructureVerifier.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20441/comment/ea48e200_71585e72?usp=email : PS30, Line 218: // private void checkOperatorSchema(ILogicalOperator op) throws AlgebricksException { > Remove Done -- 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: 34 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: Thu, 11 Dec 2025 16:37:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Ali Alsuliman <[email protected]>
