asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdullah alamoudi (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Add Support for Upsert Operation
Date Wed, 13 Jan 2016 15:23:30 GMT
abdullah alamoudi has posted comments on this change.

Change subject: Add Support for Upsert Operation
......................................................................


Patch Set 9:

(54 comments)

https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/CommitRuntime.java
File asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/CommitRuntime.java:

Line 58:     protected RecordDescriptor recDesc;
> This seems to be unused.
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/UpsertCommitRuntime.java
File asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/UpsertCommitRuntime.java:

Line 30:     private boolean isNull;
> Why is this a member? It seems the be used only in formLogRecord.
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/IntroduceSecondaryIndexInsertDeleteRule.java
File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/IntroduceSecondaryIndexInsertDeleteRule.java:

Line 168:         ARecordType originalRecType = (ARecordType) itemType;
> Just used recType instead of casting twice?
Done


Line 230:         // At first, op1 is the index insert op insertOp 
> WS
Done


Line 233:             // Why do we add these used variables to the projectVars?-> we want
to always keep the primary keys, the record, and the filtering values
> Line-break for the comment?
Done


Line 304:             ProjectOperator project = new ProjectOperator(projectVars);
> I'm a little confused about the projection variables e.g. in this rewriting
The output of the first upsert is:
$$0: the new record value.
$$3: the primary key value.
$$4: the new filter key value.
$$5: the old (to be deleted) record value.
$$6: the old filter key value.

We need to set the filter value on the deletes as well as the inserts since we use them to
know which components to search. we need them for both delete operations and insert operations.


Line 306:             // We don't actually need this additional assign since the Filter Variable
already exists (we need it for old filter variables though)
> Line-break for the comment?
Done


Line 307:             // Deleted.
> ?
Done


Line 450:                 // Get variables and expressions 
> WS
Done


Line 663:                         FunctionUtil.getFunctionInfo(AsterixBuiltinFunctions.FIELD_ACCESS_NESTED),
varRef, fieldRef);
> Couldn't we pull this constructor behind the 'if'?
I don't think we can since the two functions are different.


Line 682:             vars.add(newVar);
> It seems that the last 3 lines of the 'if' and the 'else' branch could be p
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/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 56:         // TODO Auto-generated method stub
> Remove the comment?
Done


Line 74:         AssignOperator upsertFlagAssign = null;
> Could this be a local variable in the INSERT_DELETE_UPSERT branch?
Unfortunately not since they are needed in lines 134-135


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/UpdateAPIServlet.java
File asterix-app/src/main/java/org/apache/asterix/api/http/servlet/UpdateAPIServlet.java:

Line 46:                 Kind.EXTERNAL_DATASET_REFRESH, Kind.RUN, Kind.UPSERT };
> Can we put UPSERT closer to INSERT?
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/filtered-dataset/filtered-dataset.1.ddl.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/filtered-dataset/filtered-dataset.1.ddl.aql:

Line 1: /*
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/filtered-dataset/filtered-dataset.2.update.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/filtered-dataset/filtered-dataset.2.update.aql:

Line 1: use dataverse test;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/filtered-dataset/filtered-dataset.3.query.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/filtered-dataset/filtered-dataset.3.query.aql:

Line 1: use dataverse test;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/multiple-secondaries/multiple-secondaries.1.ddl.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/multiple-secondaries/multiple-secondaries.1.ddl.aql:

Line 1: drop dataverse test if exists;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/multiple-secondaries/multiple-secondaries.2.update.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/multiple-secondaries/multiple-secondaries.2.update.aql:

Line 1: use dataverse test;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/multiple-secondaries/multiple-secondaries.3.query.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/multiple-secondaries/multiple-secondaries.3.query.aql:

Line 1: use dataverse test;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/nested-index/nested-index.1.ddl.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/nested-index/nested-index.1.ddl.aql:

Line 1: drop dataverse test if exists;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/nested-index/nested-index.2.update.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/nested-index/nested-index.2.update.aql:

Line 1: use dataverse test;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/nested-index/nested-index.3.query.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/nested-index/nested-index.3.query.aql:

Line 1: use dataverse test;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/nullable-index/nullable-index.1.ddl.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/nullable-index/nullable-index.1.ddl.aql:

Line 1: drop dataverse test if exists;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/nullable-index/nullable-index.2.update.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/nullable-index/nullable-index.2.update.aql:

Line 1: use dataverse test;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/nullable-index/nullable-index.3.query.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/nullable-index/nullable-index.3.query.aql:

Line 1: use dataverse test;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/open-index/open-index.1.ddl.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/open-index/open-index.1.ddl.aql:

Line 1: drop dataverse test if exists;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/open-index/open-index.2.update.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/open-index/open-index.2.update.aql:

Line 1: use dataverse test;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/open-index/open-index.3.query.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/open-index/open-index.3.query.aql:

Line 1: use dataverse test;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/primary-index/primary-index.1.ddl.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/primary-index/primary-index.1.ddl.aql:

Line 1: /*
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/primary-index/primary-index.2.update.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/primary-index/primary-index.2.update.aql:

Line 1: /*
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/primary-index/primary-index.3.query.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/primary-index/primary-index.3.query.aql:

Line 1: /*
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-btree/primary-secondary-btree.1.ddl.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-btree/primary-secondary-btree.1.ddl.aql:

Line 1: /*
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-btree/primary-secondary-btree.2.update.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-btree/primary-secondary-btree.2.update.aql:

Line 1: /*
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-btree/primary-secondary-btree.3.query.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-btree/primary-secondary-btree.3.query.aql:

Line 1: /*
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-inverted/primary-secondary-inverted.1.ddl.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-inverted/primary-secondary-inverted.1.ddl.aql:

Line 1: drop dataverse test if exists;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-inverted/primary-secondary-inverted.2.update.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-inverted/primary-secondary-inverted.2.update.aql:

Line 1: use dataverse test;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-inverted/primary-secondary-inverted.3.query.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-inverted/primary-secondary-inverted.3.query.aql:

Line 1: use dataverse test;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-rtree/primary-secondary-rtree.1.ddl.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-rtree/primary-secondary-rtree.1.ddl.aql:

Line 1: drop dataverse test if exists;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-rtree/primary-secondary-rtree.2.update.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-rtree/primary-secondary-rtree.2.update.aql:

Line 1: use dataverse test;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-rtree/primary-secondary-rtree.3.query.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/primary-secondary-rtree/primary-secondary-rtree.3.query.aql:

Line 1: use dataverse test;
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/upsert-with-self-read/upsert-with-self-read.1.ddl.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/upsert-with-self-read/upsert-with-self-read.1.ddl.aql:

Line 1: /*
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/upsert-with-self-read/upsert-with-self-read.2.update.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/upsert-with-self-read/upsert-with-self-read.2.update.aql:

Line 1: /*
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-app/src/test/resources/runtimets/queries/upsert/upsert-with-self-read/upsert-with-self-read.3.query.aql
File asterix-app/src/test/resources/runtimets/queries/upsert/upsert-with-self-read/upsert-with-self-read.3.query.aql:

Line 1: /*
> License header missing
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-common/src/main/java/org/apache/asterix/common/transactions/ILogRecord.java
File asterix-common/src/main/java/org/apache/asterix/common/transactions/ILogRecord.java:

Line 140:     public void formEntityUpsertCommitLogRecord(ITransactionContext txnCtx, int
datasetId, int PKHashValue,
> Can we put all the the form...() methods in one place?
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-common/src/main/java/org/apache/asterix/common/transactions/LogRecord.java
File asterix-common/src/main/java/org/apache/asterix/common/transactions/LogRecord.java:

Line 757:     public void formEntityUpsertCommitLogRecord(ITransactionContext txnCtx, int
datasetId, int PKHashValue,
> Can we put all the the form...() methods in one place?
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/base/Statement.java
File asterix-lang-common/src/main/java/org/apache/asterix/lang/common/base/Statement.java:

Line 57:         UPSERT
> Can we put UPSERT closer to INSERT? Or will that impact some serialization?
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlMetadataProvider.java
File asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlMetadataProvider.java:

Line 2313:             // Need to follow this rabbit hole --> got out of the hole :)
> ??
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/FieldAccessByIndexEvalFactory.java
File asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/FieldAccessByIndexEvalFactory.java:

Line 65:     public void setFieldIndex(int fieldIndex) {
> Seems to be unused.
I have removed it. I think I will introduce it later. if we want to use the same field index
throughout the operation, it doesn't make sense to re-set it all the time.


Line 94:                 } else {
> as presetFieldIndex is never set, this branch is never executed
Done


Line 122:                     if (presetFieldIndex < 0) {
> as presetFieldIndex is never set, this branch is always executed
Done


https://asterix-gerrit.ics.uci.edu/#/c/477/9/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/AsterixLSMPrimaryUpsertOperatorNodePushable.java
File asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/AsterixLSMPrimaryUpsertOperatorNodePushable.java:

Line 59:     private ArrayTupleBuilder nullTupleBuild;
> nullTupleBuilder?
Done


Line 102:     // we need to use the primary index opTracker and secondary indexes callbacks
for insert/delete since the lock will be obtained through
> line-break for the comment?
Done


Line 269:         // prepare the 
> WS
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8999000331795a5949d621d2dd003903e057a521
Gerrit-PatchSet: 9
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: abdullah alamoudi <bamousaa@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-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message