KazukiKandaKK commented on code in PR #19610:
URL: https://github.com/apache/pinot/pull/19610#discussion_r4109902194
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/PostQueryCommand.java:
##########
@@ -138,15 +172,71 @@ 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.
+ String formatResponse(String rawResponse) {
+ 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);
+ 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.");
+ return rawResponse;
Review Comment:
Fixed - added a flag tracking whether the CSV render actually succeeded, so
execute() skips the write and returns false on fallback instead of silently
writing raw JSON. Tests cover both the failure and success paths.
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/PostQueryCommand.java:
##########
@@ -138,15 +172,71 @@ 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.
+ String formatResponse(String rawResponse) {
+ 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);
+ 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.");
+ 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.");
+ }
+ 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();
+ csvPrinter.print(cell.isTextual() ? cell.asText() : cell.toString());
Review Comment:
Fixed - isNull() is handled separately now and prints as an empty field, so
it no longer collides with the literal string "null". Also documented the
print(null) vs print("") distinction in a comment. Test covers both cases in
one row.
--
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]
