>From Preetham Poluparthi <[email protected]>: Attention is currently required from: Ali Alsuliman, Peeyush Gupta, [email protected].
Preetham Poluparthi has posted comments on this change by Preetham Poluparthi. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19604?usp=email ) Change subject: [ASTERIXDB-3606][COMP]: Refactor plan representation: split PlanNode, use DatasetRegistry, replace index-based tracking ...................................................................... Patch Set 27: Code-Review+1 (15 comments) Patchset: PS23: > It will be good to check the perf implications of this patch also as it will > cause creation of more […] yeah, but this change is only going to effect compile time File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/AbstractLeafInput.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19604/comment/f12cdea2_a1e406a7?usp=email : PS23, Line 40: public DataSourceScanOperator findDataSourceScanOperator() { : ILogicalOperator scanOp = op; : while (scanOp != null && scanOp.getOperatorTag() != LogicalOperatorTag.EMPTYTUPLESOURCE) { : if (scanOp.getOperatorTag().equals(LogicalOperatorTag.DATASOURCESCAN)) { : return (DataSourceScanOperator) scanOp; : } : scanOp = scanOp.getInputs().get(0).getValue(); : } : return null; : } > nit: This function doesn't really seem that much related here. […] moved to EnumerateJoinsRule and made it static File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/DatasetRegistry.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19604/comment/cb621b24_042baf99?usp=email : PS23, Line 45: public class DatasetSubset { : private Long bitset; : : public DatasetSubset(AbstractLeafInput dataset) { : this.bitset = (1L << datasetMap.get(dataset)); : } : : public DatasetSubset() { : bitset = 0L; : } : : public void addDataset(AbstractLeafInput dataset) { : bitset |= (1L << datasetMap.get(dataset)); : } : : public Long getBitset() { : return bitset; : } : : @Override : public boolean equals(Object obj) { : return obj instanceof DatasetSubset && Objects.equals(bitset, ((DatasetSubset) obj).bitset); : } : : public int size() { : int count = 0; : for (int i = 0; i < datasets.size(); i++) { : if ((bitset & (1L << i)) != 0) { : count++; : } : } : return count; : } : : @Override : public String toString() { : StringBuilder sb = new StringBuilder(); : for (int i = 0; i < datasets.size(); i++) { : if ((bitset & (1L << i)) != 0) { : sb.append(datasets.get(i).toString()); : } : } : return sb.toString(); : } : > Seems to me that having DatasetSubset class defined in its own file will > improve readability in this […] I need DatasetSubset to inherit from DatasetRegistry. Conceptually, a plan contains a single DatasetRegistry with all datasets, and a DatasetSubset represents a subset of those datasets using a compact long-based bitset, so tha subset comparisons are fast. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19604/comment/93baebf1_09fe8bc0?usp=email : PS23, Line 92: Union > generally we use function names starting with lower case Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19604/comment/c2099e89_a5341e90?usp=email : PS23, Line 98: Intersect > intersect Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19604/comment/1c373d75_8cbea090?usp=email : PS23, Line 92: public DatasetSubset Union(DatasetSubset ds1, DatasetSubset ds2) { : DatasetSubset result = new DatasetSubset(); : result.bitset = ds1.bitset | ds2.bitset; : return result; : } : : public DatasetSubset Intersect(DatasetSubset ds1, DatasetSubset ds2) { : DatasetSubset result = new DatasetSubset(); : result.bitset = ds1.bitset & ds2.bitset; : return result; : } > It seems these functions can be static as well These can’t be made static because of the design I mentioned above. Please take a look at my earlier comment for the context. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19604/comment/043e6962_21eb028f?usp=email : PS23, Line 109: public static boolean equals(DatasetSubset one, DatasetSubset two) { : return Objects.equals(one.bitset, two.bitset); : } > why is this function needed? Can't the user just call Object. […] I need to ensure that the objects' parent class is also same File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/JoinCondition.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19604/comment/841f6241_98d85c20?usp=email : PS23, Line 41: public DatasetRegistry.DatasetSubset datasetSubset; : // used for triangle detection; we strictly do not mean left and right here. : // first and second sides would be more appropriate : public AbstractLeafInput leftSide; : public AbstractLeafInput rightSide; : public DatasetRegistry.DatasetSubset leftSideBits; : public DatasetRegistry.DatasetSubset rightSideBits; > Why these need to be public? can they be protected or private? Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/JoinEnum.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19604/comment/cc10e5b2_6e08a9c2?usp=email : PS23, Line 866: DatasetRegistry > wont Object. […] Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/JoinNode.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19604/comment/f41ea014_c3f5fc28?usp=email : PS23, Line 1675: // PlanNode leftPlan = pn.getLeftPlanNode(); : // PlanNode rightPlan = pn.getRightPlanNode(); : // sb.append("planIndexes ").append(leftPlan.getIndex()).append(" ").append(rightPlan.getIndex()) : // .append('\n'); : // sb.append(dumpLeftRightPlanCosts(pn)); > This code might be used for debugging purposes. Check if it can be removed. > […] The main motivation behind the initial refactor is to eliminate index arrays and numeric identifiers, and replace them with meaningful Java classes. I’ll revisit this part and improve its debuggability after completing some additional refactoring. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/PlanRegistry.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19604/comment/fe823e17_a5166563?usp=email : PS22, Line 20: package org.apache.asterix.optimizer.rules.cbo; : : import java.util.ArrayList; : import java.util.List; : : public class PlanRegistry { : private final List<AbstractPlanNode> allPlans; : : public PlanRegistry() { : allPlans = new ArrayList<>(); : } : : public void addPlan(AbstractPlanNode plan) { : allPlans.add(plan); : } : } : > Are you planning to use this plan registry in the future or is for some > debugging purpose as there i […] fixed this, you can see I got rid of planIndexesArray by using PlanRegistry File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/RealLeafInput.java: PS23: > I think UnnestLeafInput is also a type of real leaf input. […] I wanted UnnestLeafInputs to be different from RealLeafInput. Say we have SELECT customers c, c.orders o, c.reviews r... Then customers will be one RealLeafInput, orders & reviews will be two unnestLeafInputs pointing to customers. I can remove this in this patch & have it when I refactor the Unnest part. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19604/comment/496e6128_f1f581c2?usp=email : PS23, Line 35: public boolean isUnnest() { > Are there chances there will be more types of leaf inputs? If yes, instead of > adding isUnnest we sho […] No, real and Unnest are the only possible leafinputs File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/Stats.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19604/comment/0d1cc76b_087195e7?usp=email : PS23, Line 184: (RealLeafInput) joinEnum.leafInputs.get(idx1 - 1), : (RealLeafInput) joinEnum.leafInputs.get(idx2 - 1) > Can't we use leaf1 and leaf2 here Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/UnnestLeafInput.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19604/comment/9d13ec41_3738cc20?usp=email : PS22, Line 29: this.op = op; > This line can be removed. Same as the UnnestLeafInput comment above -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19604?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: I5c5a670fb66b5d525af31a3e8eca1e0c429e7adc Gerrit-Change-Number: 19604 Gerrit-PatchSet: 27 Gerrit-Owner: Preetham Poluparthi <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Peeyush Gupta <[email protected]> Gerrit-Reviewer: Preetham Poluparthi <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-CC: Murtadha Hubail <[email protected]> Gerrit-Attention: Peeyush Gupta <[email protected]> Gerrit-Attention: Ali Alsuliman <[email protected]> Gerrit-Attention: [email protected] Gerrit-Comment-Date: Thu, 20 Nov 2025 18:45:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Peeyush Gupta <[email protected]>
