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[master]: ASTERIXDB-1109: Fixed deletion of records from open secondar...
Date Sat, 17 Oct 2015 19:52:06 GMT
Ildar Absalyamov has posted comments on this change.

Change subject: ASTERIXDB-1109: Fixed deletion of records from open secondary index
......................................................................


Patch Set 6:

(11 comments)

https://asterix-gerrit.ics.uci.edu/#/c/439/6/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/IntroduceMaterializationForInsertDeleteWithSelfScanRule.java
File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/IntroduceMaterializationForInsertDeleteWithSelfScanRule.java:

Line 44: public class IntroduceMaterializationForInsertDeleteWithSelfScanRule implements IAlgebraicRewriteRule
{
> Actually, I'm not sure whether we need to have this materization rule for t
Initially I was thinking along those line as well and did not put materialization in delete
pipeline, but that resulted in quirky behavior deadlocking the index. I agree there might
be a better way of doing that, rather then materialization. Once the patch is merged I will
file a JIRA issue on that.


Line 82:     private boolean checkIfInsertDeleteAndScanDatasetsSame(AbstractLogicalOperator
op, String insertDatasetName) {
> insertDeleteDataSetName?
Done


https://asterix-gerrit.ics.uci.edu/#/c/439/6/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/ReplaceSinkOpWithCommitOpRule.java
File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/ReplaceSinkOpWithCommitOpRule.java:

Line 83:                 DeleteOperator insertDeleteOperator = (DeleteOperator) descendantOp;
> deleteOperator?
Done


https://asterix-gerrit.ics.uci.edu/#/c/439/6/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SubstitutePKFromDataScanOrUnnestRule.java
File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/SubstitutePKFromDataScanOrUnnestRule.java:

Line 84: public class SubstitutePKFromDataScanOrUnnestRule implements IAlgebraicRewriteRule
{
> Wonderful rule! Generally, can we add more comments in the code itself? The
Should be better now, let me know if more explanation is needed


Line 99:         AssignOperator assign = (AssignOperator) op;
> assignOp?
Done


Line 103:         for (Mutable<ILogicalExpression> e : assign.getExpressions()) {
> e -> expr?
Done


Line 137:         AssignOperator newAssign = new AssignOperator(assign.getVariables(), newAssignExprs);
> newAssignOp?
Done


Line 144:     private boolean checkIfSubstitutable(Mutable<ILogicalOperator> op, Mutable<ILogicalOperator>
projectRef,
> Especially for this function, please put more comments inside of this funct
Done


https://asterix-gerrit.ics.uci.edu/#/c/439/6/asterix-app/src/test/resources/optimizerts/queries/nested-index/scan-delete-secondary-index-btree-composite.aql
File asterix-app/src/test/resources/optimizerts/queries/nested-index/scan-delete-secondary-index-btree-composite.aql:

Line 25:     l_orderkey: int32, 
> Can we remove these spaces?
Done


https://asterix-gerrit.ics.uci.edu/#/c/439/6/asterix-app/src/test/resources/optimizerts/results/disjunction-to-join-delete-1.plan
File asterix-app/src/test/resources/optimizerts/results/disjunction-to-join-delete-1.plan:

Line 13:                         -- STREAM_PROJECT  |PARTITIONED|
> Why these deletions happened?
That's the result of a SubstitutePKFromDataScanOrUnnestRule firing: assigns were optimized
out, because they contained unnecessary PK lookups


https://asterix-gerrit.ics.uci.edu/#/c/439/6/asterix-app/src/test/resources/optimizerts/results/nested-open-index/inverted-index-join/leftouterjoin-probe-pidx-with-join-edit-distance-check-idx_01.plan
File asterix-app/src/test/resources/optimizerts/results/nested-open-index/inverted-index-join/leftouterjoin-probe-pidx-with-join-edit-distance-check-idx_01.plan:

Line 78:                                                     -- SPLIT  |PARTITIONED|
> Actually here, below the SPLIT, they are not the same as the original opera
If you will look closely there are two SPILTs in this plan: lines 83 & 54 and lines 61.
Everything underneath here was deleted because it's a copy of lines 54-66.
It might be a bit confusing, cause the plan printer does not distinguish between SPLIT operators.
But I think generally we don't want to pull variable names into the plan, these is an old
issue about that https://issues.apache.org/jira/browse/ASTERIXDB-354


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/439
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I41bde91401f67918365de7df19dd2f0de20c73d2
Gerrit-PatchSet: 6
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Ildar Absalyamov <ildar.absalyamov@gmail.com>
Gerrit-Reviewer: Ian Maxon <imaxon@apache.org>
Gerrit-Reviewer: Ildar Absalyamov <ildar.absalyamov@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Taewoo Kim <wangsaeu@gmail.com>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: Young-Seok Kim <kisskys@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message