Michael Blow has posted comments on this change. Change subject: Add Maven Plugin for Grammar Extension ......................................................................
Patch Set 2: (14 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) { > CRITICAL SonarQube violation: This is a false-positive, but I suspect you can just do new ArrayList<>(Statement.KINDS) in the ctor and eliminate this block altogether. Seems like allowedStatements should be an immutable list as well. Line 53: protected List<Byte> getAllowedStatements() { Wouldn't this be better suited as a Set? 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. Line 48: protected List<Byte> getAllowedStatements() { Wouldn't this be better suited as a Set? 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. Line 44: protected List<Byte> getAllowedStatements() { Wouldn't this be better suited as a Set? 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? 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. Line 46: protected List<Byte> getAllowedStatements() { Wouldn't this be better suited as a Set? 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, you wouldn't be violating the constants in an interface rule, and also you would be able to eliminate a lot of the diff (since Statement.Kind.INSERT would still be Statement.Kind.INSERT), just a differing type. If you leave these here, the public static final modifiers are redundant. Line 82: while (b >= start && b <= end) { if you check that end > start, can't you just do: for (byte b = start; b <= end; b++) { bytes.add(b); } seems easier to understand (to me) 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; > MAJOR SonarQube violation: Sort of a strange rule, but it would go away if you move the constants under Statement.Kind 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/? Line 40: return -0x01; What is the significance of -0x01 here? -- 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 <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
