Ildar Absalyamov has posted comments on this change. Change subject: ASTERIXDB-1221: - Fixed test regression and improved plans in insert\delete pipeline ......................................................................
Patch Set 2: (5 comments) https://asterix-gerrit.ics.uci.edu/#/c/549/2/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/LoadRecordFieldsRule.java File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/LoadRecordFieldsRule.java: Line 147 > Why the piece of code is no longer needed? Again leftover from previous version of patch This was an attempt to remove usages of context.addPrimaryKey/findPrimaryKey https://asterix-gerrit.ics.uci.edu/#/c/549/2/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SubstituteEquivalenceClassAssignRule.java File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SubstituteEquivalenceClassAssignRule.java: Line 56: * ============ > I'm not sure why this rule is needed. It seems to be the inverse of InlineV True, seems like it does the opposite. But we do already have a rule which does exactly that: ExtractCommonExpressionsRule, right? Maybe the right solution here would be to make ExtractCommonExpressionsRule aware of equivalence classes. Also note that the purpose of the rule was not only to put the right connector, but reuse common subexpressions, which are extracting PK from the record. https://asterix-gerrit.ics.uci.edu/#/c/549/2/asterix-app/src/main/java/org/apache/asterix/aql/translator/AqlTranslator.java File asterix-app/src/main/java/org/apache/asterix/aql/translator/AqlTranslator.java: Line 952: //check whether there exists another enforced index on the same field > Why does it matter? This is done to allow creating multiple open indexes with promotable types. See negative test case runtimets/queries/open-index-enforced/error-checking/index-type-promotion-collision/index-type-promotion-collision.1.ddl.aql for reference. I guess I'll better file a separate issue and patch for that https://asterix-gerrit.ics.uci.edu/#/c/549/2/asterix-app/src/test/resources/optimizerts/queries/scan-insert-secondary-index.aql File asterix-app/src/test/resources/optimizerts/queries/scan-insert-secondary-index.aql: Line 49 > Why do you delete this test case? This test case was kinda superseded by asterix-app/src/test/resources/optimizerts/queries/scan-insert-secondary-index-btree.aql https://asterix-gerrit.ics.uci.edu/#/c/549/2/asterix-app/src/test/resources/runtimets/queries/dml/insert-and-scan-dataset-with-index-on-open-field/insert-and-scan-dataset-with-index-on-open-field.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/dml/insert-and-scan-dataset-with-index-on-open-field/insert-and-scan-dataset-with-index-on-open-field.1.ddl.aql: Line 33: age:int64, > why int32 does not work? I've changed this test case was accidentally omitted (not included into testsuite) and missed the transition to int64 being default integer type. Query with int32 would have perfectly worked, but the adm result which would be produced would contain ugly "i32" suffixes in all integer fields. -- To view, visit https://asterix-gerrit.ics.uci.edu/549 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I304955b79c5813f7b99539da674ab519e124aae4 Gerrit-PatchSet: 2 Gerrit-Project: asterixdb Gerrit-Branch: release-0.8.8 Gerrit-Owner: Ildar Absalyamov <ildar.absalya...@gmail.com> Gerrit-Reviewer: Ildar Absalyamov <ildar.absalya...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Yingyi Bu <buyin...@gmail.com> Gerrit-HasComments: Yes
