Jackie-Jiang commented on code in PR #19610:
URL: https://github.com/apache/pinot/pull/19610#discussion_r4174969598
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/PostQueryCommand.java:
##########
@@ -138,15 +177,104 @@ public String run()
payload.putAll(_additionalOptions);
}
String request = JsonUtils.objectToString(payload);
- return sendRequest("POST", url, request, AuthProviderUtils.makeAuthHeaders(
+ String response = sendRequest("POST", url, request,
AuthProviderUtils.makeAuthHeaders(
AuthProviderUtils.makeAuthProvider(_authProvider, _authTokenUrl,
_authToken, _user, _password)));
+ return formatResponse(response);
+ }
+
+ /// Renders the raw broker response according to `_outputFormat`. Never
throws for a
+ /// CSV-incompatible response: falls back to the raw response whenever the
response isn't valid
+ /// JSON, or has no `resultTable` (e.g. the query errored out), so no error
detail is ever lost
+ /// from the logged/returned string. Marks the response unsafe for file
output whenever this
+ /// fallback happens, so `execute()` can refuse to write a non-CSV body to
`-outputFile`
+ /// when CSV was requested. A partial response is still rendered for
logging, but is marked
+ /// unsafe for file output because CSV cannot preserve its response-level
diagnostics.
+ String formatResponse(String rawResponse) {
+ _lastResponseSafeForOutputFile = true;
+ if (_outputFormat != OutputFormat.CSV) {
+ return rawResponse;
+ }
+ JsonNode root;
+ try {
+ root = JsonUtils.stringToJsonNode(rawResponse);
+ } catch (IOException e) {
+ LOGGER.warn("Response is not valid JSON (e.g. a broker/proxy error
page); "
+ + "falling back to the raw response instead of CSV.", e);
+ _lastResponseSafeForOutputFile = false;
+ return rawResponse;
+ }
+ JsonNode resultTable = root.get("resultTable");
+ if (resultTable == null || resultTable.isNull()) {
+ LOGGER.warn("Response has no 'resultTable' (e.g. the query may have
errored out); "
+ + "falling back to JSON output instead of CSV.");
+ _lastResponseSafeForOutputFile = false;
+ return rawResponse;
+ }
+ JsonNode exceptions = root.path("exceptions");
+ if (exceptions.size() > 0 || root.path("partialResult").asBoolean(false)) {
+ LOGGER.warn("Response has a 'resultTable' but also reports exceptions
and/or a partial "
+ + "result; CSV output only renders resultTable rows, so this detail
is not reflected "
+ + "in the CSV. Use -outputFormat JSON to inspect the full
response.");
+ _lastResponseSafeForOutputFile = false;
+ }
+ JsonNode columnNames = resultTable.path("dataSchema").path("columnNames");
+ JsonNode rows = resultTable.path("rows");
+ StringWriter stringWriter = new StringWriter();
+ try (CSVPrinter csvPrinter = new CSVPrinter(stringWriter,
CSVFormat.DEFAULT)) {
+ for (JsonNode columnName : columnNames) {
+ csvPrinter.print(columnName.asText());
+ }
+ csvPrinter.println();
+ for (JsonNode row : rows) {
+ Iterator<JsonNode> cells = row.elements();
+ while (cells.hasNext()) {
+ JsonNode cell = cells.next();
+ // SQL NULL (JSON null) must render as an empty CSV field, matching
Pinot CLI's
+ // existing CSV output convention. Without this, a null cell and a
STRING cell holding
+ // the literal text "null" would be indistinguishable in the export.
Note:
+ // csvPrinter.print(null) (not print("")) is used here for
correctness in the general
+ // case: for a *leading* column, commons-csv's MINIMAL quote mode
quotes an explicit
+ // empty string as `""` (to keep it distinguishable from a
null/omitted leading field)
+ // but leaves print(null) unquoted, so only print(null) is
guaranteed to produce an
+ // unquoted empty field regardless of column position. (For
non-leading columns,
+ // print("") and print(null) already produce the same unquoted
output; a STRING cell
+ // holding an actual empty string in a non-leading column remains
indistinguishable
+ // from NULL either way -- a pre-existing, unrelated ambiguity this
change does not
+ // introduce or attempt to fix.)
+ if (cell.isNull()) {
+ csvPrinter.print(null);
+ } else {
+ csvPrinter.print(cell.isTextual() ? cell.asText() :
cell.toString());
+ }
+ }
+ csvPrinter.println();
+ }
+ } catch (IOException e) {
+ // CSVPrinter only throws IOException for the underlying Appendable; a
StringWriter never
+ // throws, so this is unreachable in practice. Fall back to JSON rather
than propagate.
+ LOGGER.warn("Unexpected error rendering CSV; falling back to JSON
output.", e);
+ _lastResponseSafeForOutputFile = false;
+ return rawResponse;
+ }
+ return stringWriter.toString();
}
@Override
public boolean execute()
throws Exception {
String result = run();
LOGGER.info("Result: {}", result);
+ if (_outputFile != null) {
+ if (_outputFormat == OutputFormat.CSV &&
!_lastResponseSafeForOutputFile) {
+ // The response either could not be rendered as CSV or the CSV would
omit failure metadata.
+ // Refuse the write so a file consumer cannot mistake fallback JSON or
incomplete rows for
+ // a successful CSV export. The diagnostic response remains visible in
the log above.
Review Comment:
Minor (non-blocking): For a response containing `resultTable` and
exceptions, `formatResponse()` returns CSV and logs a generic warning.
`execute()` then logs those CSV rows, so the actual broker exception messages
are not visible despite this comment. Could we log the exception details (or
raw response) before refusing the file, so users can diagnose the failure
without rerunning the query with `-outputFormat JSON`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
