asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ildar Absalyamov (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[release-0.8.8]: ASTERIXDB-1221: - Fixed test regression and improved plans ...
Date Fri, 11 Dec 2015 22:43:10 GMT
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.absalyamov@gmail.com>
Gerrit-Reviewer: Ildar Absalyamov <ildar.absalyamov@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message