Dmitry Lychagin has posted comments on this change. Change subject: [NO ISSUE][API] added parse-only request parameter ......................................................................
Patch Set 2: (13 comments) https://asterix-gerrit.ics.uci.edu/#/c/3061/2/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"); As I mentioned in the spec comment, we need to finalize whether we need this wrapper 'parsevals' field or whether top-level 'statement-parameters' will be sufficient https://asterix-gerrit.ics.uci.edu/#/c/3061/2/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ParseValuesJsonPrintUtils.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ParseValuesJsonPrintUtils.java: Line 26: public class ParseValuesJsonPrintUtils { > MAJOR SonarQube violation: Let's consider moving this code into org.apache.asterix.api.http.server.ResultUtil. It already has several helper methods for result printing. Line 39: //TODO: The format is string. Should we use JSON? SessionConfig.PlanFormat is not applicable here. Query plans can either be returned as JSON or Text, but statement parameters will always be returned as JSON https://asterix-gerrit.ics.uci.edu/#/c/3061/2/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 453: param.setPlanFormat(request.getParameter(Parameter.PLAN_FORMAT.str())); add setParseOnly() in this branch too. This handles 'application/x-www-form-urlencoded' requests. Line 537: } //TODO: moved this out of try-catch block. we'll need to move it back into the try-catch block, because it needs to be handled by the catch clause code Line 542: if (param.isParseOnly()) { let's not duplicate this try-catch block. We don't want do have a copy of 'Access-Control' handling code, warning printing. Can we move this if (isParseOnly()) inside the try block? This way we'll also share the catch block Line 635: //assert (param.isMultiStatement() == false); > MAJOR SonarQube violation: We need to discuss how this 'parse-only' flag will work with multiple statements. We could either disallow it or support it by returning statement-parameters as array of arrays (one inner array for each statements). Let's add this to a list of open items in the spec. Line 643: } catch (Exception ce) { we don't need to catch exception here. Let handleRequest()'s catch block deal with it. https://asterix-gerrit.ics.uci.edu/#/c/3061/2/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java: Line 254: public List<Statement> getStatements() { Is this method used in your new code? If not then let's remove it. https://asterix-gerrit.ics.uci.edu/#/c/3061/2/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/parser/ParseValueParser.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/parser/ParseValueParser.java: Line 49: public List<Statement> parse() throws CompilationException { This helper class doesn't seem to add much value. Let's consider inlining the contents of this method into QueryServiceSerlet.parseStatements() Line 54: st.accept(erv, extVars); We'll need to use one output extVars set per statement if we decide to support multiple statements https://asterix-gerrit.ics.uci.edu/#/c/3061/2/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/parser/ParseValues.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/parser/ParseValues.java: Line 28: public class ParseValues implements Serializable { Where do we use toStringDebug()? Do we need to store it in this field at all? Can we rename this class to ParseOnlyResult to be a bit more precise about its purpose? and move it to from org.apache.asterix.lang.sqlpp.parser to org.apache.asterix.api.http.server package? Also this class does not need to be Serializable because it'll never be serialized https://asterix-gerrit.ics.uci.edu/#/c/3061/2/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/NamedParameterVisitor.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/NamedParameterVisitor.java: Line 44: public class NamedParameterVisitor extends AbstractSqlppQueryExpressionVisitor<Void, Collection<VariableExpr>> { Instead of implementing a new visitor can you try running org.apache.asterix.lang.sqlpp.visitor.FreeVariableVisitor on the root expression? ExternalVariables are free variables of the whole query so that visitor should return you all external variables -- To view, visit https://asterix-gerrit.ics.uci.edu/3061 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iefa0edd1d9491335965fa9e6a1a9094d8070c83c Gerrit-PatchSet: 2 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
