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 Maven Plugin for Grammar Extension
Date Fri, 22 Jul 2016 10:43:13 GMT
abdullah alamoudi has posted comments on this change.

Change subject: Add Maven Plugin for Grammar Extension
......................................................................


Patch Set 2:

(17 comments)

https://asterix-gerrit.ics.uci.edu/#/c/1011/2/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/AQLAPIServlet.java
File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/AQLAPIServlet.java:

Line 42:         for (Byte k : Statement.KINDS) {
> This is a false-positive, but I suspect you can just do new ArrayList<>(Sta
I have addressed this in another change


Line 53:     protected List<Byte> getAllowedStatements() {
> Wouldn't this be better suited as a Set?
I think it would be. Let's do it in another change?


https://asterix-gerrit.ics.uci.edu/#/c/1011/2/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/DDLAPIServlet.java
File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/DDLAPIServlet.java:

Line 31:     private static final List<Byte> STATEMENTS = Arrays.asList(new Byte[] {
Statement.DATAVERSE_DECL,
> Seems like STATEMENTS should be immutable.
I have addressed this in another change already


Line 48:     protected List<Byte> getAllowedStatements() {
> Wouldn't this be better suited as a Set?
let's do it in another change?
I created an issue for it.

https://issues.apache.org/jira/browse/ASTERIXDB-1542


https://asterix-gerrit.ics.uci.edu/#/c/1011/2/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/QueryAPIServlet.java
File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/QueryAPIServlet.java:

Line 31:     private static final List<Byte> STATEMENTS = Arrays.asList(new Byte[] {
Statement.DATAVERSE_DECL,
> Seems like STATEMENTS should be immutable.
Addressed in another change already


Line 44:     protected List<Byte> getAllowedStatements() {
> Wouldn't this be better suited as a Set?
issue created:
https://issues.apache.org/jira/browse/ASTERIXDB-1542


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

Line 233:     protected abstract List<Byte> getAllowedStatements();
> Wouldn't this be better suited as a Set?
Issue created for this as I think it should be addressed in a separate change:
https://issues.apache.org/jira/browse/ASTERIXDB-1542


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

Line 31:     private static final List<Byte> STATEMENTS = Arrays.asList(new Byte[] {
Statement.DATAVERSE_DECL, Statement.DELETE,
> Seems like STATEMENTS should be immutable.
addressed already in another change


Line 46:     protected List<Byte> getAllowedStatements() {
> Wouldn't this be better suited as a Set?
An issue created for this
https://issues.apache.org/jira/browse/ASTERIXDB-1542


https://asterix-gerrit.ics.uci.edu/#/c/1011/2/asterixdb/asterix-app/src/main/java/org/apache/asterix/aql/translator/QueryTranslator.java
File asterixdb/asterix-app/src/main/java/org/apache/asterix/aql/translator/QueryTranslator.java:

Issues in this page have been addressed already in another change


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

Line 26:     public static final byte DATASET_DECL = 0x00;
> If you kept the constants into a nested class 'Kind' inside of Statement, y
Done


Line 74:      * 
> MAJOR SonarQube violation:
Done


Line 82:         while (b >= start && b <= end) {
> if you check that end > start, can't you just do:
Done


https://asterix-gerrit.ics.uci.edu/#/c/1011/2/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/InsertStatement.java
File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/InsertStatement.java:

Line 31:     private final Query query;
> Sort of a strange rule, but it would go away if you move the constants unde
Done


https://asterix-gerrit.ics.uci.edu/#/c/1011/2/asterixdb/asterix-maven-plugins/asterix-grammar-extension-maven-plugin/src/main/java/org/apache/asterix/lang/extension/EchoStatement.java
File asterixdb/asterix-maven-plugins/asterix-grammar-extension-maven-plugin/src/main/java/org/apache/asterix/lang/extension/EchoStatement.java:

Line 25: public class EchoStatement implements Statement {
> Does this belong in src/test/?
Probably. done!


Line 40:         return -0x01;
> What is the significance of -0x01 here?
Each statement must have a unique byte representation. In java the byte range from -0x80 to
0x7f. The positive byte values are reserved for core statements while the negative range is
for extensions. 

When query translator identify a statement as an extension statement, it can then forward
it to the extension to handle it.


https://asterix-gerrit.ics.uci.edu/#/c/1011/2/asterixdb/asterix-maven-plugins/asterix-grammar-extension-maven-plugin/src/test/java/org/apache/asterix/extension/grammar/GrammarExtensionMojoTest.java
File asterixdb/asterix-maven-plugins/asterix-grammar-extension-maven-plugin/src/test/java/org/apache/asterix/extension/grammar/GrammarExtensionMojoTest.java:

Line 31:     protected void setUp() throws Exception {
> CRITICAL SonarQube violation:
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa2d11782d43dd8f27d69e347ed0fc8797d79dad
Gerrit-PatchSet: 2
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Michael Blow <mblow@apache.org>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message