>From Wail Alkowaileet <wael....@gmail.com>: Attention is currently required from: Ali Alsuliman, Vijay Sarathy, Till Westmann. Wail Alkowaileet has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17313 )
Change subject: PLEASE EDIT to provide a meaningful commit message! Add cost/card to explain string plan ...................................................................... Patch Set 8: (9 comments) Commit Message: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17313/comment/d9c47748_2aa82c1b PS8, Line 7: LEASE EDIT to provide a meaningful commit message! : Add cost/card to explain string plan Let's reference an issue for this patch and conform to the standard message. See an example here: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17318 File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/AbstractLogicalOperatorPrettyPrintVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17313/comment/64172d09_7108a4d3 PS8, Line 40: protected Double planCard = 0.0; : protected Double planCost = 0.0; : protected Double opCard = 0.0; : protected Double opLocalCost = 0.0; : protected Double opTotalCost = 0.0; Remove and declare them as local variables. They don't need to be members. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17313/comment/79a1c312_9abfb1c0 PS8, Line 104: opTotalCost = (Double) getAnnotationValue(op, OperatorAnnotations.OP_COST_TOTAL); You can inline with the declaration: Double opTotalCost =(Double) getAnnotationValue(op, OperatorAnnotations.OP_COST_TOTAL) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17313/comment/38781c4c_f527b552 PS8, Line 109: Object getAnnotationValue Let's make this returns a "Double" and rename the method to getAnnotationDoubleValue .. This would protect (in a usability sense) against using it for values other than Doubles :) File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/LogicalOperatorPrettyPrintVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17313/comment/da002b96_8dbfc56d PS8, Line 130: planCard Declare it locally and avoid using the member ones (as we should remove them) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17313/comment/750f1b8f_b83276bb PS8, Line 132: toLowerCase() Both are already in lowercase. You can remove the call toLowerCase() https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17313/comment/ec616ef2_5450cded PS8, Line 135: toLowerCase same as above https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17313/comment/958a1710_db77a16a PS8, Line 150: OperatorAnnotations.OP_COST_LOCAL We can have something similar to protected static final String CARDINALITY = "cardinality"; to OperatorAnnotations.OP_COST_TOTAL, OperatorAnnotations.OP_COST_LOCAL, etc... So we can be consistent File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/LogicalOperatorPrettyPrintVisitorJson.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17313/comment/a7b182d4_5f567c7e PS8, Line 279: OperatorAnnotations.OP_COST_LOCAL Same as the STRING plan... we should use the declared ones. -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17313 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: Ie697b90cd4396208ab8f59c68e20ced236903fb8 Gerrit-Change-Number: 17313 Gerrit-PatchSet: 8 Gerrit-Owner: Vijay Sarathy <vijay.sara...@couchbase.com> 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: Till Westmann <t...@couchbase.com> Gerrit-Reviewer: Vijay Sarathy <vijay.sara...@couchbase.com> Gerrit-Reviewer: Wail Alkowaileet <wael....@gmail.com> Gerrit-Attention: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-Attention: Vijay Sarathy <vijay.sara...@couchbase.com> Gerrit-Attention: Till Westmann <t...@couchbase.com> Gerrit-Comment-Date: Tue, 24 Jan 2023 00:26:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
