Dmitry Lychagin has posted comments on this change. Change subject: [NO ISSUE][API] added parse-only request parameter ......................................................................
Patch Set 1: (14 comments) https://asterix-gerrit.ics.uci.edu/#/c/3085/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/AbstractQueryApiServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/AbstractQueryApiServlet.java: Line 54: PARSEVALS("parsevals"); We need to come up with a better name than "parsevals". Let's discuss with Till https://asterix-gerrit.ics.uci.edu/#/c/3085/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryServiceServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryServiceServlet.java: Line 534: // CORS let's extract CORS header handling into a separate method and call it from both branches of this 'if' statement Line 579: protected ParseOnlyResult parseStatement(String statementsText) throws Exception { > CRITICAL SonarQube violation: Agree with SONAR on this one. Let's declare as "throws CompilationException". Seems like no other exception can be thrown by this method Line 584: throw new CompilationException("MultiStatement not support"); //TODO: proper exception message We need to fix this TODO item. https://asterix-gerrit.ics.uci.edu/#/c/3085/1/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ResultUtil.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ResultUtil.java: Line 391: appendNonNull(output, EXTERNAL_VARIABLE_LBL, parseOnlyResult.toStringDescription()); Pass this StringBuilder instance (output) to toStringDescription() and append to it there. https://asterix-gerrit.ics.uci.edu/#/c/3085/1/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/TestExecutor.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/TestExecutor.java: Line 1506: remove this commented out code. https://asterix-gerrit.ics.uci.edu/#/c/3085/1/asterixdb/asterix-app/src/test/resources/runtimets/results/parseonly/001/named_01.2.adm File asterixdb/asterix-app/src/test/resources/runtimets/results/parseonly/001/named_01.2.adm: Line 1: {"statement-parameters":"[?p_obj,?p_null,?p_bool,?p_dbl,?p_int,?p_str,?p_arr,?p_dec]"} I'd expect the field value to be a JSON array, not a string. So the whole result would be: {"statement-parameters": [ "p_obj", "p_null", "p_bool" ] } How come it's different here? https://asterix-gerrit.ics.uci.edu/#/c/3085/1/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_parseonly.xml File asterixdb/asterix-app/src/test/resources/runtimets/testsuite_parseonly.xml: Line 22: <test-group name="parseonly"> let's have tests with positional parameters too (and mixed with named too) Line 30: <output-dir compare="Text">named_02</output-dir> Change the expected output-dir to "001". "named_02" doesn't exist. https://asterix-gerrit.ics.uci.edu/#/c/3085/1/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/parser/ParseOnlyResult.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/parser/ParseOnlyResult.java: Line 20: package org.apache.asterix.lang.sqlpp.parser; This is a helper class for the API change, let's move it to the API package: org.apache.asterix.api.http.server Line 29: private String extVarsSerialized; We don't need this field. toStringDescription() is called once and should just return the result without storing it in this field (or even better: append to StringBuilder and do not return any value) Line 42: for (VariableExpr extVarRef : externalVariables) { We wanted to have positional variables returned as JSON integers, and named ones as JSON strings. This is what we have in the spec: "Positional params first, sorted, then named also sorted". I don't see it being done in this code. Line 43: extVarsSerialized += SqlppVariableUtil.toUserDefinedName(extVarRef.getVar().getValue()) + ","; Use StringBuilder (append()) when concatenating Strings. Line 45: extVarsSerialized = extVarsSerialized.replaceAll(",$", "]"); Let's not add "," for the last variable, so we won't need to replace it here. One way do it is to add "," before adding the variable name for all collection elements, except the first one. -- To view, visit https://asterix-gerrit.ics.uci.edu/3085 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idd2f461c22b05a5fcaa50a6e4f9b7dcd91acc184 Gerrit-PatchSet: 1 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: [email protected] Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-HasComments: Yes
