Jackie-Jiang commented on code in PR #19610:
URL: https://github.com/apache/pinot/pull/19610#discussion_r4113737509
##########
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:
**Correctness — partial query results still become successful CSV file
exports.** A broker response can contain a `resultTable` plus exceptions (for
example, missing segments in `MultiStageBrokerRequestHandler`) or
`partialResult=true`. This code logs a warning, then marks the CSV render
successful; `execute()` writes the rows and returns `true`. The file contains
no indication that rows may be missing, so a file consumer can treat incomplete
data as complete. For `-outputFile`, could we report failure for partial
responses while preserving the diagnostic, and test rows plus exceptions and
rows plus `partialResult=true`?
##########
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:
**Minor dependency cleanup:** `pinot-tools` now imports Commons CSV
directly, but its POM only receives `commons-csv` transitively through
`pinot-csv`. Could we declare `commons-csv` directly in `pinot-tools/pom.xml`
so a later input-format plugin dependency change cannot break this command's
compile/runtime classpath?
##########
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:
**The rejected export does not signal failure in the official Docker
invocation.** `return false` maps to a nonzero command status, but the Pinot
Dockerfiles set `JAVA_OPTS` with `-Dpinot.admin.system.exit=false`;
`PinotAdministrator.main()` then returns normally, so `docker run ... PostQuery
-outputFormat CSV -outputFile ...` exits 0 on this fallback. An existing output
file is left untouched and can be mistaken for the new result. Could we make
the one-shot Docker path report the failed export, or document and verify the
required exit setting? This is inherited launcher behavior, but the new
file-output failure path relies on it.
--
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]
