KazukiKandaKK commented on code in PR #19610:
URL: https://github.com/apache/pinot/pull/19610#discussion_r4113961735
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/PostQueryCommand.java:
##########
@@ -138,15 +178,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. Sets `_lastResponseRenderedAsRequested`
to `false` whenever
+ /// this fallback happens, so `execute()` can refuse to write a non-CSV body
to `-outputFile`
+ /// when CSV was requested.
+ String formatResponse(String rawResponse) {
+ if (_outputFormat != OutputFormat.CSV) {
+ _lastResponseRenderedAsRequested = true;
+ 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);
+ _lastResponseRenderedAsRequested = 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.");
+ _lastResponseRenderedAsRequested = 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.");
+ }
+ 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);
+ _lastResponseRenderedAsRequested = false;
+ return rawResponse;
+ }
+ _lastResponseRenderedAsRequested = true;
Review Comment:
Fixed - responses with exceptions or `partialResult=true` are now rejected
before `-outputFile` is written. The partial rows are still rendered in the
log, along with a warning to use JSON for the full response. Tests cover both
cases and confirm that an existing file is left unchanged.
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/PostQueryCommand.java:
##########
@@ -138,15 +178,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. Sets `_lastResponseRenderedAsRequested`
to `false` whenever
+ /// this fallback happens, so `execute()` can refuse to write a non-CSV body
to `-outputFile`
+ /// when CSV was requested.
+ String formatResponse(String rawResponse) {
+ if (_outputFormat != OutputFormat.CSV) {
+ _lastResponseRenderedAsRequested = true;
+ 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);
+ _lastResponseRenderedAsRequested = 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.");
+ _lastResponseRenderedAsRequested = 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.");
+ }
+ 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);
+ _lastResponseRenderedAsRequested = false;
+ return rawResponse;
+ }
+ _lastResponseRenderedAsRequested = true;
+ return stringWriter.toString();
}
@Override
public boolean execute()
throws Exception {
String result = run();
LOGGER.info("Result: {}", result);
+ if (_outputFile != null) {
+ if (_outputFormat == OutputFormat.CSV &&
!_lastResponseRenderedAsRequested) {
+ // The response could not be rendered as CSV (e.g. a broker error or
non-JSON body), so
+ // `result` is the raw fallback, not CSV. Do not let a file consumer
mistake this
+ // non-CSV body for a successful CSV export; refuse the write and
report failure instead.
+ // The raw response is still visible above via LOGGER.info("Result:
{}", ...).
+ LOGGER.warn("Not writing '{}': response could not be rendered as CSV.
Use "
+ + "-outputFormat JSON to inspect the full response.", _outputFile);
+ return false;
Review Comment:
Set `-Dpinot.admin.system.exit=true` in all three official Pinot
Dockerfiles. `PinotAdministrator` calls `System.exit` only for nonzero command
statuses, so successful commands are unchanged and rejected `PostQuery` exports
now exit nonzero. An existing output file remains unchanged when the export is
rejected.
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/PostQueryCommand.java:
##########
@@ -18,8 +18,17 @@
*/
package org.apache.pinot.tools.admin.command;
+import com.fasterxml.jackson.databind.JsonNode;
+import java.io.File;
+import java.io.IOException;
+import java.io.StringWriter;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
import java.util.HashMap;
+import java.util.Iterator;
import java.util.Map;
+import org.apache.commons.csv.CSVFormat;
+import org.apache.commons.csv.CSVPrinter;
Review Comment:
Added `commons-csv` as a direct dependency of `pinot-tools`. The root POM
still manages the version.
--
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]
