asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Till Westmann (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Add Upsert Operation
Date Sat, 07 Nov 2015 03:24:51 GMT
Till Westmann has posted comments on this change.

Change subject: Add Upsert Operation
......................................................................


Patch Set 3:

(17 comments)

1) It would be nice to go through the files and fix the end-of-line whitespace and remove
the tabs.
2) Some new files have an UCI license header, they should have an ASF license header.
3) Some interfaces got new methods that seem to apply to only a small number of implementations.
Also, those methods are not well documented. It would be good to clarify why those methods
have to be on the interface and what they need to do.

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

Line 90:         
WS

generally seems that there should be no change in this file ...


https://asterix-gerrit.ics.uci.edu/#/c/477/3/asterix-common/src/main/java/org/apache/asterix/common/feeds/api/IDatasourceAdapter.java
File asterix-common/src/main/java/org/apache/asterix/common/feeds/api/IDatasourceAdapter.java:

Line 53:      */
The comment seems wrong. Also, do we really need to extend the interface in this way? Does
this make sense for all adapters?


https://asterix-gerrit.ics.uci.edu/#/c/477/3/asterix-external-data/src/main/java/org/apache/asterix/external/adapter/factory/PushBasedTwitterAdapterFactory.java
File asterix-external-data/src/main/java/org/apache/asterix/external/adapter/factory/PushBasedTwitterAdapterFactory.java:

Line 111:     @Override
Empty line above this?


https://asterix-gerrit.ics.uci.edu/#/c/477/3/asterix-lang-aql/src/main/javacc/AQL.jj
File asterix-lang-aql/src/main/javacc/AQL.jj:

Line 884:   upsert = InsertOrUpsert() "into" <DATASET> nameComponents = QualifiedName()
query = Query()
How about doing something like 

( "insert" { upsert = false; } | "upsert" { upsert = true; } )

here?


https://asterix-gerrit.ics.uci.edu/#/c/477/3/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 592:                 // ^^^^^^^^
This looks scary ...


Line 1273:     // Done with this one.
??


Line 1467:     // The only remaining one.
??


https://asterix-gerrit.ics.uci.edu/#/c/477/3/asterix-metadata/src/main/java/org/apache/asterix/metadata/external/IAdapterFactory.java
File asterix-metadata/src/main/java/org/apache/asterix/metadata/external/IAdapterFactory.java:

Line 101:     public void setFiles(List<ExternalFile> files) throws AlgebricksException;
Do we need this on the interface? It seems that most implementations do not implement this.
Would it make sense to create a sub-interface or an abstract implementation between this interface
and its implementations?


https://asterix-gerrit.ics.uci.edu/#/c/477/3/asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/AbstractDatasourceAdapter.java
File asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/AbstractDatasourceAdapter.java:

Line 33: public abstract class AbstractDatasourceAdapter implements IDatasourceAdapter {
Is this class used?


https://asterix-gerrit.ics.uci.edu/#/c/477/3/asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/AbstractFeedDatasourceAdapter.java
File asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/AbstractFeedDatasourceAdapter.java:

Line 25: public abstract class AbstractFeedDatasourceAdapter implements IDatasourceAdapter
{
Is this class used?


https://asterix-gerrit.ics.uci.edu/#/c/477/3/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/serde/ARecordSerializerDeserializer.java
File asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/serde/ARecordSerializerDeserializer.java:

Line 48: // This doesn't work well for deserialization. use ARecordPointable.
??


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

Line 14:  */
This should have an ASF license.


https://asterix-gerrit.ics.uci.edu/#/c/477/3/asterix-runtime/src/main/java/org/apache/asterix/runtime/dataflow/AsterixLSMSecondaryUpsertOperatorNodePushable.java
File asterix-runtime/src/main/java/org/apache/asterix/runtime/dataflow/AsterixLSMSecondaryUpsertOperatorNodePushable.java:

Line 14:  */
ASF license


https://asterix-gerrit.ics.uci.edu/#/c/477/3/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/LockThenSearchOperationCallback.java
File asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/LockThenSearchOperationCallback.java:

Line 14:  */
ASF license


https://asterix-gerrit.ics.uci.edu/#/c/477/3/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/LockThenSearchOperationCallbackFactory.java
File asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/LockThenSearchOperationCallbackFactory.java:

Line 14:  */
ASF license


https://asterix-gerrit.ics.uci.edu/#/c/477/3/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/UpsertOperationCallback.java
File asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/UpsertOperationCallback.java:

Line 14:  */
ASF license


https://asterix-gerrit.ics.uci.edu/#/c/477/3/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/UpsertOperationCallbackFactory.java
File asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/UpsertOperationCallbackFactory.java:

Line 14:  */
ASF license


-- 
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: 3
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-HasComments: Yes

Mime
View raw message