Till Westmann has posted comments on this change. Change subject: [NO ISSUE][API] added parse-only request parameter ......................................................................
Patch Set 1: (13 comments) Looks good. I added a few suggestions to simply this a bit more - mainly by returning the result object in the "result" field instead of adding a new top-level field to the result envelope. I might be contradicting myself here, but looking at this change it seems that using the result field make more sense. 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 "parsevals" seems a bit generic, But thinking about this a little more, I think that we don't need a new field. I would claim that the statement parameters are a valid result of a parse-only request and that we should return the parameters in the result field. The is very similar to the logical plan that one will get when running an "EXPLAIN .." query. 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 Yes, that makes sense. I thought that we already had that factored out at some point (but maybe that's just wishful thinking ...). Line 584: throw new CompilationException("MultiStatement not support"); //TODO: proper exception message > We need to fix this TODO item. We should probably use QueryTranslator.validateStatements for this. Interestingly that throws a RuntimeDataException while it looks more like a CompilationException to me. We should fix that (either in this change or in a separate change). PS1, Line 586: 0 And we'd need to get the last statement here. PS1, Line 691: printParseValues Rename to "printParseOnlyResult"? PS1, Line 694: PARSEVALS Another benefit of using the results filed would be that we probably don't need most of the additions in the test framework. 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: PS1, Line 384: ParseValuesJsonPrintUtils I think that we don't need this nested class. Wondering if it'd work to move this into ExecutionPlansJsonPrintUtil. The name doesn't seem to be a perfect match, but I think that printing information from the parse tree is pretty close to writing plans. Otherwise we could also create a new standalone class next to ExecutionPlansJsonPrintUtil. PS1, Line 399: quoteAndEscape I think that we should not quote and escape here. If we have an array inside a string field, the consumer of the result will have to first parse the JSON to get the string and the to parse the sting again. Instead, we should have an array of strings. PS1, Line 404: TODO Indeed 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: PS1, Line 157: PARSE_QUERY_TYPE E.g. we wouldn't need this, if we return the information in the result field (but we'd need to rename the test files). https://asterix-gerrit.ics.uci.edu/#/c/3085/1/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/ParseOnlyTest.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/ParseOnlyTest.java: PS1, Line 34: TODO Do we still need this? 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: PS1, Line 39: toStringDescription Rename this as toJson()? We're using this in other places for methods that return a JSON object as a string. https://asterix-gerrit.ics.uci.edu/#/c/3085/1/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/SqlppQueryRewriter.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/SqlppQueryRewriter.java: PS1, Line 261: TODO I think that it's fine in the rewriter. -- 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-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
