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 <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
