>From Janhavi Tripurwar <[email protected]>: Attention is currently required from: Murtadha Hubail, Ali Alsuliman. Janhavi Tripurwar has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687 )
Change subject: [ASTERIXDB-3593]: Support individual responses for multi-statement Queries ...................................................................... Patch Set 18: Code-Review+1 (21 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/IRequestExecution.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/d06620c7_30c84caa PS6, Line 28: IRequestExecution > It does not feel like this interface is serving/defining anything for > implementation. […] Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/IStatementExecutor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/415fe001_41c1f3db PS6, Line 289: class : : > Some formatting needed here. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/e29f43d2_954903d2 PS6, Line 330: void > Why not make this return a list of responses for each statement? Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/RequestExecutionContext.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/58cfcd1b_1decfe1a PS6, Line 34: requestMessageId > What's this for? Not needed anymore. I was using it earlier—removed the getter but overlooked the setter. File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryServiceServlet.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/e6f49c69_2582d634 PS6, Line 231: setRequestParam(request, param, optionalParamProvider, executionStatus); : setDefaults(param); > Isn't this kind of backwards? Shouldn't we first set whatever defaults, then > override the defaults f […] Yes, my mistake. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/7017aa45_29014a6d PS6, Line 328: param.setMultiStatement(isAllowMultiStatement(request, param)); > We have already configured the param from the request just before. […] There was a reason to do it, not needed with the changes now. File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/ResultUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/9b873cc8_8e7955e5 PS6, Line 68: public static boolean multiStatement; > I don't think having this as static here is going to work when we are > processing queries concurrentl […] Yes, my bad. We will not need this if we decide to not indent tabs based on multi-statement values. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/00314b9a_92673d12 PS6, Line 313: multiStatement > Why not pass 'multiStatement' to the ResultDecorator() constructor? If this > is just to print 1 tab v […] This is no longer needed after our discussion of not formatting(one tab/ two tab) based on multi-statements. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/660ad2d1_9b4be3ac PS6, Line 315: int resultNo = -1; > Remove this if we are not going to use it anymore. Done File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/ExecuteStatementRequestMessage.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/42fe0f22_73e8dff8 PS6, Line 96: private final boolean reqReceivedByCc = false; > This instance field is not useful since it's kind of a constant. Also, this > is a serializable class. […] Yes, it's a constant with value false indicating the request was received by the NC. I've changed it to a static variable now. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/1d05e49b_1dc40951 PS6, Line 181: //Setting this here, so that in case of syntax error we don't return internal error because of null pointer. > How can we get syntax error here? the parser.parse() is already done by now. > […] It's not needed anymore, as we've made things less stateful now. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/7a3f9e08_b671daf6 PS6, Line 209: requestExecutionContext.setRequestMessageId(requestMessageId); : requestExecutionContext.setRequestReceivedByCc(reqReceivedByCc); : requestExecutionContext.setMaxWarnings(maxWarnings); : requestExecutionContext.setWarnings(stmtParseWarnings); > Aren't these repeating what was just done by getRequestExecutionContext()? not needed. File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/ExecuteStatementResponseMessage.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/6b790b62_ee84d9ed PS6, Line 55: statementId > Is this needed? No it's not needed. RequestExecutionContext.statementCounter is responsible for the statementIDs that we print in the response. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/9ea885d6_d27551a5 PS6, Line 221: <= > Why are some of them '<=' and some '<'? It's not needed anymore, as we've made things less stateful now. File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/result/ResponsePrinter.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/42801dcc_f2a588f1 PS6, Line 49: setMultiStatement > Can we remove this setter and instead have it as part of creating the > ResponsePrinter? Then you won' […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/7fdf7f33_d72ef399 PS6, Line 93: printFieldSeparator(sessionOutput.out()); > This assumes that printHeaders() has printed something and therefore we need > to print a field separa […] Yes. printHeaders print things like RequestId. When multi-statement is true, this printFieldSeparator adds a "," and a new line before printing the statements array. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/3b702e33_ae54107c PS6, Line 149: if (isMultiStatement) { : if ((headersPrinted || resultsPrinted) && fieldsCount > 0 : && (statementIdPrinted || printers.getFirst() instanceof StatusPrinter)) { : printFieldSeparator(sessionOutput.out()); : } else if (printers.getFirst() instanceof ErrorsPrinter) { : sessionOutput.out().print("\t{\n"); : } : } else { > It feels like there is too much things going on here. Just keep in mind the > following: […] I’ve added fieldCount > 0 as an check in the else block for safety. All of this logic is to ensure that we print the correct response format in case of a syntax error when multi-statements is enabled. Example: Dry Run for the Query selec 2;; Expected Response: { "requestID": "...", "statements": [ { "errors": [{ "code": 1, "msg": "ASX1001: Syntax error: In line 1 >>selec 2;<< Encountered <INTEGER_LITERAL> \"2\" at column 7. " } ], "status": "fatal", "metrics": { ... } } ] } First: printers.size() = 1 → RequestIdPrinter No condition is met, so it directly enters the for loop and processes normally. Second: printers.size() = 1 → ErrorsPrinter headersPrinted = true, fieldCount > 0 statementIdPrinted = false and this is not a StatusPrinter So it hits the else condition and prints: Third: printers.size() = 2 → StatusPrinter and MetricsPrinter headersPrinted = true, fieldCount > 0, statementIdPrinted = false Since this is a StatusPrinter, it matches the if condition and prints accordingly. File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/38e31bd0_eeb80a8a PS6, Line 4498: runTrackJob > The RequestTracker/ClientRequest need to be aware about the multi-statement > now. […] As discussed, I will address this in a separate patch. File asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/NCApplication.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/377cbc6f_a80d300a PS6, Line 249: { : } > What is this for? This isn't needed—NCQueryServiceServlet became abstract as a result of the changes I made while addressing issues. File asterixdb/asterix-app/src/main/java/org/apache/asterix/utils/PrintResultUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/8f5be29d_c4073eba PS6, Line 62: public static boolean isPrintFromCC > How will this work when there are concurrent requests some of which are > received by CC and others by […] Thanks for pointing this out! Most of the places already determine this using requestExecutionContext.isRequestReceivedByCc(). I’ve now replaced the usage of the static variable with the responseMessage value. While it's possible to use requestExecutionContext.isRequestReceivedByCc() in those locations as well, doing so would require threading the requestExecutionContext parameter through the function chain just for this purpose. To avoid that overhead, I'm instead using the already available responseMessage, which serves the same intent. File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/WarningCollector.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/c6497af9_5fcd24d3 PS6, Line 42: private final Map<Integer, Set<Warning>> stmtParseWarnings = new HashMap<>(); > It's better to keep track of this in the parser. […] Done. We return List<Pair<Statement, Set<Warning>>> from parser now as discussed. -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: Iecba6ed2a30f7a2e7d8ff21e995117ed505dfad5 Gerrit-Change-Number: 19687 Gerrit-PatchSet: 18 Gerrit-Owner: Janhavi Tripurwar <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Janhavi Tripurwar <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Attention: Murtadha Hubail <[email protected]> Gerrit-Attention: Ali Alsuliman <[email protected]> Gerrit-Comment-Date: Wed, 25 Jun 2025 11:09:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Ali Alsuliman <[email protected]> Gerrit-MessageType: comment
