>From Ali Alsuliman <[email protected]>: Attention is currently required from: Murtadha Hubail, Janhavi Tripurwar. Ali Alsuliman 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 6: (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/4d827ad5_73020f7c PS6, Line 28: IRequestExecution It does not feel like this interface is serving/defining anything for implementation. We could just separate the classes into separate files. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/IStatementExecutor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/8d2da5c2_c3ecfffb PS6, Line 289: class : : Some formatting needed here. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/6c87bf2d_8b2bb983 PS6, Line 330: void Why not make this return a list of responses for each statement? File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/RequestExecutionContext.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/616bd780_b14ab18e PS6, Line 34: requestMessageId What's this for? 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/2c2fa600_85415e5e 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 from whatever the user specified in the request? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/ae729724_3b4e43e4 PS6, Line 328: param.setMultiStatement(isAllowMultiStatement(request, param)); We have already configured the param from the request just before. We shouldn't need to parse the request again. 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/d7843035_ae49cd02 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 concurrently. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/f3b7a90b_0d41c7a2 PS6, Line 313: multiStatement Why not pass 'multiStatement' to the ResultDecorator() constructor? If this is just to print 1 tab vs 2 tabs, then you can pass the string directly to the constructor: just do ResultDecorator(multiStatement ? "\t\t\"" : "\t\""). On a different note, remind me to come back to this and think about whether we need to do printing with formatting or not. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/f7be9202_5c036c41 PS6, Line 315: int resultNo = -1; Remove this if we are not going to use it anymore. File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/ExecuteStatementRequestMessage.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/1645f5e2_fb6c8237 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. So, having such fields would be just a waste (not sure if there is any optimizations done by Java for such cases). https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/798e4c1f_3aee251a 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. Do you mean errors due to statements validations? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/264def98_880f11e9 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()? File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/ExecuteStatementResponseMessage.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/84203f9e_a75e9b78 PS6, Line 55: statementId Is this needed? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/5f42e84c_dca0104e PS6, Line 221: <= Why are some of them '<=' and some '<'? File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/result/ResponsePrinter.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/093b990f_c8f0e1a5 PS6, Line 49: setMultiStatement Can we remove this setter and instead have it as part of creating the ResponsePrinter? Then you won't need to expose printHeadersMultipleStatements(). It should be all internal how to print the headers. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/22c14fe8_73c232ff PS6, Line 93: printFieldSeparator(sessionOutput.out()); This assumes that printHeaders() has printed something and therefore we need to print a field separator. Is that always the case? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/b619db2a_f1fb6075 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: this print() is called from multiple places. "printers" may or may not have items based on the logic here. So, printers.getFirst() might throw an exception. Also, some callers to print() mark something after the call to indicate to the next print() call to print a field separator. For example, printHeaders() will mark headersPrinter which indicates that the headers have been printed or not so that the next print() call can figure out whether to add a field separator or not. So, have you thought about this part and whether it could throw off the next call to print()?: else if (printers.getFirst() instanceof ErrorsPrinter) { sessionOutput.out().print("\t{\n"); } File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/81f8adc8_1df9ec80 PS6, Line 4498: runTrackJob The RequestTracker/ClientRequest need to be aware about the multi-statement now. Currently, the clientRequest assumes there is a single statement. As things are, the information in the clientRequest will reflect only the information of the last statement executed. File asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/NCApplication.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/bec68a0e_79eb712d PS6, Line 249: { : } What is this for? File asterixdb/asterix-app/src/main/java/org/apache/asterix/utils/PrintResultUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/6fa861ac_4fdb672c 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 NC? Queries from all requests are going to share access/update this simultaneously. File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/WarningCollector.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19687/comment/5c24c490_3318f075 PS6, Line 42: private final Map<Integer, Set<Warning>> stmtParseWarnings = new HashMap<>(); It's better to keep track of this in the parser. You can simply have a list in the parser where each element is a collection of warnings for the corresponding statement. Here you can add one method to return the current list of warnings (or empty if there is none), then you clear the warning collector for the next statement. -- 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: 6 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: Janhavi Tripurwar <[email protected]> Gerrit-Comment-Date: Tue, 13 May 2025 00:32:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
