You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "MartijnVisser (via GitHub)" <gi...@apache.org> on 2026/09/29 05:18:57 UTC
[PR] [FLINK-24908][sql-client] Print the outermost exception that ends with the root cause [flink]
MartijnVisser opened a new pull request, #29334:
URL: https://github.com/apache/flink/pull/29334
## What is the purpose of the change
Follow-up to #29333; the first two commits are that PR's. The client printed the deepest cause of an error, which for parse and validation errors is a Calcite class with the bare text, so the position the planner now adds never reached the terminal. The client now prints the outermost exception whose message ends with the root cause's message, in the in-JVM chain, in verbose mode and in the `Caused by:` segments of a gateway stack trace: `ELECT 1;` prints `SqlParserException: SQL parse failed. From line 1, column 1 to line 1, column 5: Non-query expression encountered in illegal context` instead of `CalciteException: Non-query expression encountered in illegal context`. Validation errors and `LOAD`/`USE`/`UNLOAD MODULE` failures change the same way; anything no wrapper prefixes is unchanged, including planning rejections, which put the plan after the reason. A computed column that fails validation now prints the `CalciteContextException` line with the position instead of the bare
`SqlValidatorException` line, since `ParserImpl#parseSqlExpression` does not quote its cause; showing the Flink class there is a follow-up.
## Brief change log
- `CliStrings#messageError` picks the outermost exception that ends with the root cause's message, in the chain and in the `RestClientException` text, also in verbose mode
- the `Caused by:` split is anchored at line start and stops at the first frame line
- a cycle in the cause chain no longer hangs the client
## Verifying this change
This change added tests and can be verified as follows:
- Added `CliStringsTest`: six cases red on the planner-only head, seven that pin today's behavior, one cycle case that hangs without the change
- `CliClientITCase`: fourteen golden blocks change and `select.q` gains `ELECT 1;`, the same behind a comment, `SELECT * FROM (t);` and a streaming `ORDER BY` rejection, all copied from the run
- `ScriptExecutorITCase` and `SqlGatewayServiceStatementITCase` pass unchanged
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not applicable
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (please specify the tool below)
Generated-by: Claude Code (Claude Fable 5.1)
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-24908][sql-client] Print the outermost exception that ends with the root cause [flink]
Posted by "twalthr (via GitHub)" <gi...@apache.org>.
twalthr commented on PR #29334:
URL: https://github.com/apache/flink/pull/29334#issuecomment-6367826491
Thanks for the change, the echoed line and caret are a clear UX gain. Feedback, most important first:
**Correctness**
- **Tab truncation:** `FIRST_FRAME` (`\R\t`) cuts a headline at the first line that starts with a tab. Echoed multi-line SQL with tab indentation is now part of the message, so the output gets truncated and the `endsWith` comparison breaks. Anchor on `\tat ` and `\t... \d+ more` instead, and add a test.
- **Segment without frames:** if the last `Caused by:` segment has no `\tat` lines, the trailing `End of exception on server side>]` ends up in the printed root. Strip the trailer explicitly, and add a test.
- **`isParseError(Throwable)`:** it calls `candidate.getCause().getClass()`. This is safe only because of the loop bound, and `SqlParserException` has a cause-less constructor. Make the invariant explicit with a null guard.
- **`INCOMPLETE_STATEMENT`:** the line-anchored regex can still match multi-line echoed SQL that starts a line with the class name. Contrived, but the new test covers only a same-line comment.
**Duplication**
- `findReason` (Throwable chain) and `findReasonInStackTrace` (string) implement the same selection algorithm twice, and `isParseError` exists in two overloads. Consider converting both inputs into one frame list of `(className, message)` and running a single selection routine. `ExecutorImpl` could reuse the parsed frames instead of its own regex.
- The two scans start at different places (the chain scan includes the `RestClientException`, the string scan skips the preamble). Please make this consistent.
**Readability**
- `isParseError(c) || addsContext(...) && !msg.equals(cause.toString())` mixes `||` and `&&` without parentheses and is repeated in both variants. Extract local booleans, e.g. `isParseError`, `quotesRootCause`, `merelyRepeatsCause`.
- About 100 lines of exception analysis now sit in `CliStrings`, a string-constants class. A small package-private class (e.g. `ErrorReasonFinder`) would fit better.
- Move `CALCITE_PARSE_EXCEPTION` up to the other constants, or use the class directly if the dependency is available (`SqlParserException` is imported directly).
- Trim comments that restate the code.
**Tests**
- `CliStringsTest` has 15 tests / ~290 lines with overlap. A parameterized `(input, expected)` table would shrink it, and a helper that renders frames would replace the long concatenated REST bodies. Running the same table through both the chain and REST paths is easy once they are unified.
- `ExecutorImplITCase` adds two mini-cluster tests. `testIncompleteStatementIsReported` is probably covered by existing EOF tests, so keeping only the negative one may be enough.
- Missing cases: tab inside a message, a segment without frames, a `SqlParserException` without a cause, a `null` message in a REST candidate.
**Behavior and process**
- `table.q` prints `CalciteContextException` while `module.q` prints `ValidationException: SQL validation failed…` for similar errors. Is this a planner wrapping inconsistency, or intended?
- Verbose mode now starts at the chosen exception instead of the deepest cause. Please mention this in the PR description.
- Please confirm the merge order with #29333.
*AI generated. Feel free to ignore.*
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-24908][sql-client] Print the outermost exception that ends with the root cause [flink]
Posted by "MartijnVisser (via GitHub)" <gi...@apache.org>.
MartijnVisser commented on code in PR #29334:
URL: https://github.com/apache/flink/pull/29334#discussion_r4362253882
##########
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliStrings.java:
##########
@@ -27,12 +28,29 @@
import org.jline.utils.AttributedStringBuilder;
import org.jline.utils.AttributedStyle;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.IdentityHashMap;
import java.util.LinkedHashMap;
+import java.util.List;
import java.util.Map;
+import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
/** Utility class that contains all strings for CLI commands and messages. */
public final class CliStrings {
+ /**
+ * A cause caption at the start of a line; a caption inside a message does not count, and only a
+ * line feed starts a line, since a message may carry other separators in echoed SQL.
+ */
+ private static final Pattern CAUSED_BY =
+ Pattern.compile("^Caused by: ", Pattern.MULTILINE | Pattern.UNIX_LINES);
+
+ /** The first frame line of a stack trace segment, "\tat ..." or "\t... n more". */
+ private static final Pattern FIRST_FRAME = Pattern.compile("\\R\\t");
Review Comment:
Done, the headline now ends at `\tat ` or `\t... n more`, with a test. The echoed SQL couldn't hit this since the snippet turns tabs into spaces, but other messages could.
##########
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliStrings.java:
##########
@@ -313,25 +331,98 @@ public static AttributedString messageWarning(String message) {
}
public static AttributedString messageError(String message, Throwable t, boolean isVerbose) {
- while (t.getCause() != null
- && t.getCause().getMessage() != null
- && !t.getCause().getMessage().isEmpty()) {
- t = t.getCause();
- }
-
+ final Throwable reason = findReason(t);
if (isVerbose) {
- return messageError(message, ExceptionUtils.stringifyException(t));
+ return messageError(message, ExceptionUtils.stringifyException(reason));
+ } else if (reason instanceof RestClientException) {
+ // TODO: Remove this after RestClientException supports to get RootCause.
+ return messageError(message, findReasonInStackTrace(reason.getMessage()));
} else {
- if (t instanceof RestClientException) {
- // TODO: Remove this after RestClientException supports to get RootCause.
- String[] splitExceptions = t.getMessage().split("Caused by: ");
- return messageError(
- message,
- splitExceptions[splitExceptions.length - 1].split("\tat ")[0].trim());
- } else {
- return messageError(message, t.getClass().getName() + ": " + t.getMessage());
+ return messageError(message, reason.getClass().getName() + ": " + reason.getMessage());
+ }
+ }
+
+ private static final String CALCITE_PARSE_EXCEPTION =
Review Comment:
Moved. Calcite isn't on the client's runtime classpath, so the class can't be referenced here.
##########
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliStrings.java:
##########
@@ -313,25 +331,98 @@ public static AttributedString messageWarning(String message) {
}
public static AttributedString messageError(String message, Throwable t, boolean isVerbose) {
- while (t.getCause() != null
- && t.getCause().getMessage() != null
- && !t.getCause().getMessage().isEmpty()) {
- t = t.getCause();
- }
-
+ final Throwable reason = findReason(t);
if (isVerbose) {
- return messageError(message, ExceptionUtils.stringifyException(t));
+ return messageError(message, ExceptionUtils.stringifyException(reason));
+ } else if (reason instanceof RestClientException) {
+ // TODO: Remove this after RestClientException supports to get RootCause.
+ return messageError(message, findReasonInStackTrace(reason.getMessage()));
} else {
- if (t instanceof RestClientException) {
- // TODO: Remove this after RestClientException supports to get RootCause.
- String[] splitExceptions = t.getMessage().split("Caused by: ");
- return messageError(
- message,
- splitExceptions[splitExceptions.length - 1].split("\tat ")[0].trim());
- } else {
- return messageError(message, t.getClass().getName() + ": " + t.getMessage());
+ return messageError(message, reason.getClass().getName() + ": " + reason.getMessage());
+ }
+ }
+
+ private static final String CALCITE_PARSE_EXCEPTION =
+ "org.apache.calcite.sql.parser.SqlParseException";
+
+ /**
+ * The root cause, or the outermost exception whose message ends with the root message and adds
+ * context in front of it, as Calcite does with the position of a validation error. A planning
+ * rejection, where the plan follows the reason, does not qualify. The planner's parse exception
+ * is picked by type, since it echoes the line inside the Calcite text. The search starts at a
+ * {@link RestClientException}, whose message is the whole server trace.
+ */
+ private static Throwable findReason(Throwable t) {
Review Comment:
I'd keep both. A `RestClientException` for a server error has no cause, so it's always the root and never a candidate.
##########
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliStrings.java:
##########
@@ -313,25 +331,98 @@ public static AttributedString messageWarning(String message) {
}
public static AttributedString messageError(String message, Throwable t, boolean isVerbose) {
- while (t.getCause() != null
- && t.getCause().getMessage() != null
- && !t.getCause().getMessage().isEmpty()) {
- t = t.getCause();
- }
-
+ final Throwable reason = findReason(t);
if (isVerbose) {
- return messageError(message, ExceptionUtils.stringifyException(t));
+ return messageError(message, ExceptionUtils.stringifyException(reason));
+ } else if (reason instanceof RestClientException) {
+ // TODO: Remove this after RestClientException supports to get RootCause.
+ return messageError(message, findReasonInStackTrace(reason.getMessage()));
} else {
- if (t instanceof RestClientException) {
- // TODO: Remove this after RestClientException supports to get RootCause.
- String[] splitExceptions = t.getMessage().split("Caused by: ");
- return messageError(
- message,
- splitExceptions[splitExceptions.length - 1].split("\tat ")[0].trim());
- } else {
- return messageError(message, t.getClass().getName() + ": " + t.getMessage());
+ return messageError(message, reason.getClass().getName() + ": " + reason.getMessage());
+ }
+ }
+
+ private static final String CALCITE_PARSE_EXCEPTION =
+ "org.apache.calcite.sql.parser.SqlParseException";
+
+ /**
+ * The root cause, or the outermost exception whose message ends with the root message and adds
+ * context in front of it, as Calcite does with the position of a validation error. A planning
+ * rejection, where the plan follows the reason, does not qualify. The planner's parse exception
+ * is picked by type, since it echoes the line inside the Calcite text. The search starts at a
+ * {@link RestClientException}, whose message is the whole server trace.
+ */
+ private static Throwable findReason(Throwable t) {
+ final List<Throwable> chain = new ArrayList<>();
+ final Set<Throwable> seen = Collections.newSetFromMap(new IdentityHashMap<>());
+ int outermost = 0;
+ for (Throwable current = t;
+ current != null && seen.add(current);
+ current = current.getCause()) {
+ chain.add(current);
+ if (current instanceof RestClientException) {
+ outermost = chain.size() - 1;
+ }
+ final Throwable cause = current.getCause();
+ if (cause == null || cause.getMessage() == null || cause.getMessage().isEmpty()) {
+ break;
+ }
+ }
+ final Throwable root = chain.get(chain.size() - 1);
+ for (int i = outermost; i < chain.size() - 1; i++) {
+ final Throwable candidate = chain.get(i);
+ if (isParseError(candidate)
Review Comment:
Done, both loops now use `parseError`, `quotesRootCause` and `repeatsCause`.
##########
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliStrings.java:
##########
@@ -313,25 +331,98 @@ public static AttributedString messageWarning(String message) {
}
public static AttributedString messageError(String message, Throwable t, boolean isVerbose) {
- while (t.getCause() != null
- && t.getCause().getMessage() != null
- && !t.getCause().getMessage().isEmpty()) {
- t = t.getCause();
- }
-
+ final Throwable reason = findReason(t);
if (isVerbose) {
- return messageError(message, ExceptionUtils.stringifyException(t));
+ return messageError(message, ExceptionUtils.stringifyException(reason));
+ } else if (reason instanceof RestClientException) {
+ // TODO: Remove this after RestClientException supports to get RootCause.
+ return messageError(message, findReasonInStackTrace(reason.getMessage()));
} else {
- if (t instanceof RestClientException) {
- // TODO: Remove this after RestClientException supports to get RootCause.
- String[] splitExceptions = t.getMessage().split("Caused by: ");
- return messageError(
- message,
- splitExceptions[splitExceptions.length - 1].split("\tat ")[0].trim());
- } else {
- return messageError(message, t.getClass().getName() + ": " + t.getMessage());
+ return messageError(message, reason.getClass().getName() + ": " + reason.getMessage());
+ }
+ }
+
+ private static final String CALCITE_PARSE_EXCEPTION =
+ "org.apache.calcite.sql.parser.SqlParseException";
+
+ /**
+ * The root cause, or the outermost exception whose message ends with the root message and adds
+ * context in front of it, as Calcite does with the position of a validation error. A planning
+ * rejection, where the plan follows the reason, does not qualify. The planner's parse exception
+ * is picked by type, since it echoes the line inside the Calcite text. The search starts at a
+ * {@link RestClientException}, whose message is the whole server trace.
+ */
+ private static Throwable findReason(Throwable t) {
+ final List<Throwable> chain = new ArrayList<>();
+ final Set<Throwable> seen = Collections.newSetFromMap(new IdentityHashMap<>());
+ int outermost = 0;
+ for (Throwable current = t;
+ current != null && seen.add(current);
+ current = current.getCause()) {
+ chain.add(current);
+ if (current instanceof RestClientException) {
+ outermost = chain.size() - 1;
+ }
+ final Throwable cause = current.getCause();
+ if (cause == null || cause.getMessage() == null || cause.getMessage().isEmpty()) {
+ break;
+ }
+ }
+ final Throwable root = chain.get(chain.size() - 1);
+ for (int i = outermost; i < chain.size() - 1; i++) {
+ final Throwable candidate = chain.get(i);
+ if (isParseError(candidate)
+ || addsContext(candidate.getMessage(), root.getMessage())
+ && !candidate.getMessage().equals(candidate.getCause().toString())) {
+ return candidate;
+ }
+ }
+ return root;
+ }
+
+ /** Applies the rule of {@link #findReason} to a stringified stack trace. */
+ private static String findReasonInStackTrace(String stackTrace) {
+ final String[] segments = CAUSED_BY.split(stackTrace);
+ final String root = headline(segments[segments.length - 1]);
+ // The first segment is the response preamble, never a cause worth showing on its own.
+ for (int i = 1; i < segments.length - 1; i++) {
+ final String candidate = headline(segments[i]);
+ if (isParseError(candidate, headline(segments[i + 1]))
+ || addsContext(messageOf(candidate), messageOf(root))
+ && !messageOf(candidate).equals(headline(segments[i + 1]))) {
+ return candidate;
}
}
+ return root;
+ }
+
+ private static boolean isParseError(Throwable candidate) {
+ return candidate instanceof SqlParserException
+ && candidate.getCause().getClass().getName().equals(CALCITE_PARSE_EXCEPTION);
Review Comment:
A candidate always has its cause next in `chain`, and a cause-less `SqlParserException` is the root, which is never checked. I now pass `chain.get(i + 1)` to make that visible, with a test for the cause-less case.
##########
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliStrings.java:
##########
@@ -313,25 +331,98 @@ public static AttributedString messageWarning(String message) {
}
public static AttributedString messageError(String message, Throwable t, boolean isVerbose) {
- while (t.getCause() != null
- && t.getCause().getMessage() != null
- && !t.getCause().getMessage().isEmpty()) {
- t = t.getCause();
- }
-
+ final Throwable reason = findReason(t);
if (isVerbose) {
- return messageError(message, ExceptionUtils.stringifyException(t));
+ return messageError(message, ExceptionUtils.stringifyException(reason));
+ } else if (reason instanceof RestClientException) {
+ // TODO: Remove this after RestClientException supports to get RootCause.
+ return messageError(message, findReasonInStackTrace(reason.getMessage()));
} else {
- if (t instanceof RestClientException) {
- // TODO: Remove this after RestClientException supports to get RootCause.
- String[] splitExceptions = t.getMessage().split("Caused by: ");
- return messageError(
- message,
- splitExceptions[splitExceptions.length - 1].split("\tat ")[0].trim());
- } else {
- return messageError(message, t.getClass().getName() + ": " + t.getMessage());
+ return messageError(message, reason.getClass().getName() + ": " + reason.getMessage());
+ }
+ }
+
+ private static final String CALCITE_PARSE_EXCEPTION =
+ "org.apache.calcite.sql.parser.SqlParseException";
+
+ /**
+ * The root cause, or the outermost exception whose message ends with the root message and adds
+ * context in front of it, as Calcite does with the position of a validation error. A planning
+ * rejection, where the plan follows the reason, does not qualify. The planner's parse exception
+ * is picked by type, since it echoes the line inside the Calcite text. The search starts at a
+ * {@link RestClientException}, whose message is the whole server trace.
+ */
+ private static Throwable findReason(Throwable t) {
+ final List<Throwable> chain = new ArrayList<>();
+ final Set<Throwable> seen = Collections.newSetFromMap(new IdentityHashMap<>());
+ int outermost = 0;
+ for (Throwable current = t;
+ current != null && seen.add(current);
+ current = current.getCause()) {
+ chain.add(current);
+ if (current instanceof RestClientException) {
+ outermost = chain.size() - 1;
+ }
+ final Throwable cause = current.getCause();
+ if (cause == null || cause.getMessage() == null || cause.getMessage().isEmpty()) {
+ break;
+ }
+ }
+ final Throwable root = chain.get(chain.size() - 1);
+ for (int i = outermost; i < chain.size() - 1; i++) {
+ final Throwable candidate = chain.get(i);
+ if (isParseError(candidate)
+ || addsContext(candidate.getMessage(), root.getMessage())
+ && !candidate.getMessage().equals(candidate.getCause().toString())) {
+ return candidate;
+ }
+ }
+ return root;
+ }
+
+ /** Applies the rule of {@link #findReason} to a stringified stack trace. */
+ private static String findReasonInStackTrace(String stackTrace) {
+ final String[] segments = CAUSED_BY.split(stackTrace);
+ final String root = headline(segments[segments.length - 1]);
Review Comment:
Good catch, master did the same. The headline now also ends at `End of exception on server side>`, with a test.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-24908][sql-client] Print the outermost exception that ends with the root cause [flink]
Posted by "twalthr (via GitHub)" <gi...@apache.org>.
twalthr commented on code in PR #29334:
URL: https://github.com/apache/flink/pull/29334#discussion_r4360950135
##########
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliStrings.java:
##########
@@ -27,12 +28,29 @@
import org.jline.utils.AttributedStringBuilder;
import org.jline.utils.AttributedStyle;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.IdentityHashMap;
import java.util.LinkedHashMap;
+import java.util.List;
import java.util.Map;
+import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
/** Utility class that contains all strings for CLI commands and messages. */
public final class CliStrings {
+ /**
+ * A cause caption at the start of a line; a caption inside a message does not count, and only a
+ * line feed starts a line, since a message may carry other separators in echoed SQL.
+ */
+ private static final Pattern CAUSED_BY =
+ Pattern.compile("^Caused by: ", Pattern.MULTILINE | Pattern.UNIX_LINES);
+
+ /** The first frame line of a stack trace segment, "\tat ..." or "\t... n more". */
+ private static final Pattern FIRST_FRAME = Pattern.compile("\\R\\t");
Review Comment:
`\R\t` matches the first line of a *message* that starts with a tab, not only the first stack frame. The parse error now echoes the SQL, so tab-indented multi-line SQL would truncate the headline and break the `endsWith` comparison. Anchor on `\tat ` / `\t... \d+ more` instead, and add a test with a tab in the message.
*AI generated. Feel free to ignore.*
##########
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliStrings.java:
##########
@@ -313,25 +331,98 @@ public static AttributedString messageWarning(String message) {
}
public static AttributedString messageError(String message, Throwable t, boolean isVerbose) {
- while (t.getCause() != null
- && t.getCause().getMessage() != null
- && !t.getCause().getMessage().isEmpty()) {
- t = t.getCause();
- }
-
+ final Throwable reason = findReason(t);
if (isVerbose) {
- return messageError(message, ExceptionUtils.stringifyException(t));
+ return messageError(message, ExceptionUtils.stringifyException(reason));
+ } else if (reason instanceof RestClientException) {
+ // TODO: Remove this after RestClientException supports to get RootCause.
+ return messageError(message, findReasonInStackTrace(reason.getMessage()));
} else {
- if (t instanceof RestClientException) {
- // TODO: Remove this after RestClientException supports to get RootCause.
- String[] splitExceptions = t.getMessage().split("Caused by: ");
- return messageError(
- message,
- splitExceptions[splitExceptions.length - 1].split("\tat ")[0].trim());
- } else {
- return messageError(message, t.getClass().getName() + ": " + t.getMessage());
+ return messageError(message, reason.getClass().getName() + ": " + reason.getMessage());
+ }
+ }
+
+ private static final String CALCITE_PARSE_EXCEPTION =
+ "org.apache.calcite.sql.parser.SqlParseException";
+
+ /**
+ * The root cause, or the outermost exception whose message ends with the root message and adds
+ * context in front of it, as Calcite does with the position of a validation error. A planning
+ * rejection, where the plan follows the reason, does not qualify. The planner's parse exception
+ * is picked by type, since it echoes the line inside the Calcite text. The search starts at a
+ * {@link RestClientException}, whose message is the whole server trace.
+ */
+ private static Throwable findReason(Throwable t) {
+ final List<Throwable> chain = new ArrayList<>();
+ final Set<Throwable> seen = Collections.newSetFromMap(new IdentityHashMap<>());
+ int outermost = 0;
+ for (Throwable current = t;
+ current != null && seen.add(current);
+ current = current.getCause()) {
+ chain.add(current);
+ if (current instanceof RestClientException) {
+ outermost = chain.size() - 1;
+ }
+ final Throwable cause = current.getCause();
+ if (cause == null || cause.getMessage() == null || cause.getMessage().isEmpty()) {
+ break;
+ }
+ }
+ final Throwable root = chain.get(chain.size() - 1);
+ for (int i = outermost; i < chain.size() - 1; i++) {
+ final Throwable candidate = chain.get(i);
+ if (isParseError(candidate)
+ || addsContext(candidate.getMessage(), root.getMessage())
+ && !candidate.getMessage().equals(candidate.getCause().toString())) {
+ return candidate;
+ }
+ }
+ return root;
+ }
+
+ /** Applies the rule of {@link #findReason} to a stringified stack trace. */
+ private static String findReasonInStackTrace(String stackTrace) {
+ final String[] segments = CAUSED_BY.split(stackTrace);
+ final String root = headline(segments[segments.length - 1]);
+ // The first segment is the response preamble, never a cause worth showing on its own.
+ for (int i = 1; i < segments.length - 1; i++) {
+ final String candidate = headline(segments[i]);
+ if (isParseError(candidate, headline(segments[i + 1]))
+ || addsContext(messageOf(candidate), messageOf(root))
+ && !messageOf(candidate).equals(headline(segments[i + 1]))) {
+ return candidate;
}
}
+ return root;
+ }
+
+ private static boolean isParseError(Throwable candidate) {
+ return candidate instanceof SqlParserException
+ && candidate.getCause().getClass().getName().equals(CALCITE_PARSE_EXCEPTION);
Review Comment:
`candidate.getCause()` is only non-null because of the loop bound in the caller, and `SqlParserException` has a cause-less constructor. Please make that explicit with a null guard.
*AI generated. Feel free to ignore.*
##########
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliStrings.java:
##########
@@ -313,25 +331,98 @@ public static AttributedString messageWarning(String message) {
}
public static AttributedString messageError(String message, Throwable t, boolean isVerbose) {
- while (t.getCause() != null
- && t.getCause().getMessage() != null
- && !t.getCause().getMessage().isEmpty()) {
- t = t.getCause();
- }
-
+ final Throwable reason = findReason(t);
if (isVerbose) {
- return messageError(message, ExceptionUtils.stringifyException(t));
+ return messageError(message, ExceptionUtils.stringifyException(reason));
+ } else if (reason instanceof RestClientException) {
+ // TODO: Remove this after RestClientException supports to get RootCause.
+ return messageError(message, findReasonInStackTrace(reason.getMessage()));
} else {
- if (t instanceof RestClientException) {
- // TODO: Remove this after RestClientException supports to get RootCause.
- String[] splitExceptions = t.getMessage().split("Caused by: ");
- return messageError(
- message,
- splitExceptions[splitExceptions.length - 1].split("\tat ")[0].trim());
- } else {
- return messageError(message, t.getClass().getName() + ": " + t.getMessage());
+ return messageError(message, reason.getClass().getName() + ": " + reason.getMessage());
+ }
+ }
+
+ private static final String CALCITE_PARSE_EXCEPTION =
+ "org.apache.calcite.sql.parser.SqlParseException";
+
+ /**
+ * The root cause, or the outermost exception whose message ends with the root message and adds
+ * context in front of it, as Calcite does with the position of a validation error. A planning
+ * rejection, where the plan follows the reason, does not qualify. The planner's parse exception
+ * is picked by type, since it echoes the line inside the Calcite text. The search starts at a
+ * {@link RestClientException}, whose message is the whole server trace.
+ */
+ private static Throwable findReason(Throwable t) {
Review Comment:
`findReason` and `findReasonInStackTrace` implement the same selection algorithm twice (root pick, outer-to-inner scan, parse-error check, `addsContext && !sameAsCause`), once on `Throwable`s and once on strings. Consider converting both inputs into one list of `(className, message)` frames and running a single routine. That also makes it easy to run one test table through both paths. This much exception analysis may also fit better in a small package-private class than in `CliStrings`.
Small inconsistency: this scan starts at the `RestClientException` and includes it as a candidate, while the string variant skips the preamble segment.
*AI generated. Feel free to ignore.*
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/gateway/ExecutorImplITCase.java:
##########
@@ -218,6 +220,43 @@ void testCompleteStatement() {
}
}
+ @Test
+ void testIncompleteStatementIsReported() {
Review Comment:
This is probably covered by the existing EOF tests (`CliClientITCase` / multi-line parser tests). Each of these starts a mini cluster, so keeping only the negative test below, which is the new behavior, would save build time.
*AI generated. Feel free to ignore.*
##########
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliStrings.java:
##########
@@ -313,25 +331,98 @@ public static AttributedString messageWarning(String message) {
}
public static AttributedString messageError(String message, Throwable t, boolean isVerbose) {
- while (t.getCause() != null
- && t.getCause().getMessage() != null
- && !t.getCause().getMessage().isEmpty()) {
- t = t.getCause();
- }
-
+ final Throwable reason = findReason(t);
if (isVerbose) {
- return messageError(message, ExceptionUtils.stringifyException(t));
+ return messageError(message, ExceptionUtils.stringifyException(reason));
+ } else if (reason instanceof RestClientException) {
+ // TODO: Remove this after RestClientException supports to get RootCause.
+ return messageError(message, findReasonInStackTrace(reason.getMessage()));
} else {
- if (t instanceof RestClientException) {
- // TODO: Remove this after RestClientException supports to get RootCause.
- String[] splitExceptions = t.getMessage().split("Caused by: ");
- return messageError(
- message,
- splitExceptions[splitExceptions.length - 1].split("\tat ")[0].trim());
- } else {
- return messageError(message, t.getClass().getName() + ": " + t.getMessage());
+ return messageError(message, reason.getClass().getName() + ": " + reason.getMessage());
+ }
+ }
+
+ private static final String CALCITE_PARSE_EXCEPTION =
Review Comment:
Please move this constant to the top with the other constants. Also, `SqlParserException` is imported directly, so using the Calcite class here (if the dependency is available) would be consistent.
*AI generated. Feel free to ignore.*
##########
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliStrings.java:
##########
@@ -313,25 +331,98 @@ public static AttributedString messageWarning(String message) {
}
public static AttributedString messageError(String message, Throwable t, boolean isVerbose) {
- while (t.getCause() != null
- && t.getCause().getMessage() != null
- && !t.getCause().getMessage().isEmpty()) {
- t = t.getCause();
- }
-
+ final Throwable reason = findReason(t);
if (isVerbose) {
- return messageError(message, ExceptionUtils.stringifyException(t));
+ return messageError(message, ExceptionUtils.stringifyException(reason));
+ } else if (reason instanceof RestClientException) {
+ // TODO: Remove this after RestClientException supports to get RootCause.
+ return messageError(message, findReasonInStackTrace(reason.getMessage()));
} else {
- if (t instanceof RestClientException) {
- // TODO: Remove this after RestClientException supports to get RootCause.
- String[] splitExceptions = t.getMessage().split("Caused by: ");
- return messageError(
- message,
- splitExceptions[splitExceptions.length - 1].split("\tat ")[0].trim());
- } else {
- return messageError(message, t.getClass().getName() + ": " + t.getMessage());
+ return messageError(message, reason.getClass().getName() + ": " + reason.getMessage());
+ }
+ }
+
+ private static final String CALCITE_PARSE_EXCEPTION =
+ "org.apache.calcite.sql.parser.SqlParseException";
+
+ /**
+ * The root cause, or the outermost exception whose message ends with the root message and adds
+ * context in front of it, as Calcite does with the position of a validation error. A planning
+ * rejection, where the plan follows the reason, does not qualify. The planner's parse exception
+ * is picked by type, since it echoes the line inside the Calcite text. The search starts at a
+ * {@link RestClientException}, whose message is the whole server trace.
+ */
+ private static Throwable findReason(Throwable t) {
+ final List<Throwable> chain = new ArrayList<>();
+ final Set<Throwable> seen = Collections.newSetFromMap(new IdentityHashMap<>());
+ int outermost = 0;
+ for (Throwable current = t;
+ current != null && seen.add(current);
+ current = current.getCause()) {
+ chain.add(current);
+ if (current instanceof RestClientException) {
+ outermost = chain.size() - 1;
+ }
+ final Throwable cause = current.getCause();
+ if (cause == null || cause.getMessage() == null || cause.getMessage().isEmpty()) {
+ break;
+ }
+ }
+ final Throwable root = chain.get(chain.size() - 1);
+ for (int i = outermost; i < chain.size() - 1; i++) {
+ final Throwable candidate = chain.get(i);
+ if (isParseError(candidate)
Review Comment:
`a || b && c` without parentheses is hard to read, and the condition is repeated in the string variant. Please extract local booleans, e.g. `isParseError`, `quotesRootCause`, `merelyRepeatsCause`.
*AI generated. Feel free to ignore.*
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliStringsTest.java:
##########
@@ -0,0 +1,289 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.table.client.cli;
+
+import org.apache.flink.runtime.rest.util.RestClientException;
+import org.apache.flink.table.api.SqlParserEOFException;
+import org.apache.flink.table.api.SqlParserException;
+import org.apache.flink.table.api.ValidationException;
+import org.apache.flink.table.client.gateway.SqlExecutionException;
+
+import org.apache.flink.shaded.netty4.io.netty.handler.codec.http.HttpResponseStatus;
+
+import org.apache.calcite.sql.parser.SqlParseException;
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/** Tests for the exception line that {@link CliStrings#messageError} picks. */
+class CliStringsTest {
Review Comment:
15 tests / ~290 lines with overlap (`...SameMessage`, `...OnlyAddsItsClassName`, `...StopsAtAnEmptyMessage`, the two `...RootCauseWhenNothingQuotesIt`). A parameterized `(input, expected)` table, plus a helper that renders frames instead of the long concatenated REST bodies, would shrink it and make it extensible. Missing cases: tab inside a message, a segment without frames, a `SqlParserException` without a cause, a `null` message in a REST candidate.
*AI generated. Feel free to ignore.*
##########
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliStrings.java:
##########
@@ -313,25 +331,98 @@ public static AttributedString messageWarning(String message) {
}
public static AttributedString messageError(String message, Throwable t, boolean isVerbose) {
- while (t.getCause() != null
- && t.getCause().getMessage() != null
- && !t.getCause().getMessage().isEmpty()) {
- t = t.getCause();
- }
-
+ final Throwable reason = findReason(t);
if (isVerbose) {
- return messageError(message, ExceptionUtils.stringifyException(t));
+ return messageError(message, ExceptionUtils.stringifyException(reason));
+ } else if (reason instanceof RestClientException) {
+ // TODO: Remove this after RestClientException supports to get RootCause.
+ return messageError(message, findReasonInStackTrace(reason.getMessage()));
} else {
- if (t instanceof RestClientException) {
- // TODO: Remove this after RestClientException supports to get RootCause.
- String[] splitExceptions = t.getMessage().split("Caused by: ");
- return messageError(
- message,
- splitExceptions[splitExceptions.length - 1].split("\tat ")[0].trim());
- } else {
- return messageError(message, t.getClass().getName() + ": " + t.getMessage());
+ return messageError(message, reason.getClass().getName() + ": " + reason.getMessage());
+ }
+ }
+
+ private static final String CALCITE_PARSE_EXCEPTION =
+ "org.apache.calcite.sql.parser.SqlParseException";
+
+ /**
+ * The root cause, or the outermost exception whose message ends with the root message and adds
+ * context in front of it, as Calcite does with the position of a validation error. A planning
+ * rejection, where the plan follows the reason, does not qualify. The planner's parse exception
+ * is picked by type, since it echoes the line inside the Calcite text. The search starts at a
+ * {@link RestClientException}, whose message is the whole server trace.
+ */
+ private static Throwable findReason(Throwable t) {
+ final List<Throwable> chain = new ArrayList<>();
+ final Set<Throwable> seen = Collections.newSetFromMap(new IdentityHashMap<>());
+ int outermost = 0;
+ for (Throwable current = t;
+ current != null && seen.add(current);
+ current = current.getCause()) {
+ chain.add(current);
+ if (current instanceof RestClientException) {
+ outermost = chain.size() - 1;
+ }
+ final Throwable cause = current.getCause();
+ if (cause == null || cause.getMessage() == null || cause.getMessage().isEmpty()) {
+ break;
+ }
+ }
+ final Throwable root = chain.get(chain.size() - 1);
+ for (int i = outermost; i < chain.size() - 1; i++) {
+ final Throwable candidate = chain.get(i);
+ if (isParseError(candidate)
+ || addsContext(candidate.getMessage(), root.getMessage())
+ && !candidate.getMessage().equals(candidate.getCause().toString())) {
+ return candidate;
+ }
+ }
+ return root;
+ }
+
+ /** Applies the rule of {@link #findReason} to a stringified stack trace. */
+ private static String findReasonInStackTrace(String stackTrace) {
+ final String[] segments = CAUSED_BY.split(stackTrace);
+ final String root = headline(segments[segments.length - 1]);
Review Comment:
If the last `Caused by:` segment has no `\tat` lines, `headline()` returns the whole segment including the trailing `End of exception on server side>]`. Please strip the trailer explicitly and add a test for a last segment without frames.
*AI generated. Feel free to ignore.*
##########
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/gateway/ExecutorImpl.java:
##########
@@ -110,6 +111,15 @@
/** Executor to connect to {@link SqlGateway} and execute statements. */
public class ExecutorImpl implements Executor {
+ /**
+ * The planner's own class for an incomplete statement, as a headline of the server trace. The
+ * message text is not searched, since the planner echoes the statement in it.
+ */
+ private static final Pattern INCOMPLETE_STATEMENT =
Review Comment:
The regex is line-anchored, so multi-line echoed SQL where a line starts with this class name would still match. Contrived, but the new test only covers a same-line comment. Also, stack-trace headline parsing now exists here and in `CliStrings`; a shared helper over parsed frames would avoid two regex-based parsers of the same server trace.
*AI generated. Feel free to ignore.*
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-24908][sql-client] Print the outermost exception that ends with the root cause [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #29334:
URL: https://github.com/apache/flink/pull/29334#issuecomment-5884224024
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "d9a3a37c2c3cadb522372dc85b9c9c2dd157f5f0",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "d9a3a37c2c3cadb522372dc85b9c9c2dd157f5f0",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* d9a3a37c2c3cadb522372dc85b9c9c2dd157f5f0 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-24908][sql-client] Print the outermost exception that ends with the root cause [flink]
Posted by "MartijnVisser (via GitHub)" <gi...@apache.org>.
MartijnVisser commented on code in PR #29334:
URL: https://github.com/apache/flink/pull/29334#discussion_r4362255921
##########
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/gateway/ExecutorImpl.java:
##########
@@ -110,6 +111,15 @@
/** Executor to connect to {@link SqlGateway} and execute statements. */
public class ExecutorImpl implements Executor {
+ /**
+ * The planner's own class for an incomplete statement, as a headline of the server trace. The
+ * message text is not searched, since the planner echoes the statement in it.
+ */
+ private static final Pattern INCOMPLETE_STATEMENT =
Review Comment:
The snippet indents the echoed line by four spaces, so it can't start with the class name. The old check was a `contains` on the whole trace, and a shared helper would only replace this one regex, so I'd leave it.
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/gateway/ExecutorImplITCase.java:
##########
@@ -218,6 +220,43 @@ void testCompleteStatement() {
}
}
+ @Test
+ void testIncompleteStatementIsReported() {
Review Comment:
You're right, `CliClientMultiStatementITTest` fails without the check because of the multi-line `EXECUTE STATEMENT SET` in `statement_set.q`. I've removed the positive test.
##########
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliStringsTest.java:
##########
@@ -0,0 +1,289 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.table.client.cli;
+
+import org.apache.flink.runtime.rest.util.RestClientException;
+import org.apache.flink.table.api.SqlParserEOFException;
+import org.apache.flink.table.api.SqlParserException;
+import org.apache.flink.table.api.ValidationException;
+import org.apache.flink.table.client.gateway.SqlExecutionException;
+
+import org.apache.flink.shaded.netty4.io.netty.handler.codec.http.HttpResponseStatus;
+
+import org.apache.calcite.sql.parser.SqlParseException;
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/** Tests for the exception line that {@link CliStrings#messageError} picks. */
+class CliStringsTest {
Review Comment:
I'd keep them separate, they cover different branches: the length check, the `cause.toString()` check, the empty-message stop, and chain vs REST body. I've added the tab, no-frames, cause-less and REST class-name cases. A REST candidate without a message has an empty message, which never matches.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-24908][sql-client] Print the outermost exception that ends with the root cause [flink]
Posted by "MartijnVisser (via GitHub)" <gi...@apache.org>.
MartijnVisser commented on PR #29334:
URL: https://github.com/apache/flink/pull/29334#issuecomment-6370513600
@twalthr Verbose output only changes for chains built in the client. Through the gateway the `RestClientException` stays the root, so I've corrected the description and the commit. In `table.q` the computed column is validated in `MergeTableLikeUtil`, so it never passes `FlinkPlannerImpl.validate()`, which adds `SQL validation failed.`. That's a planner follow-up. #29333 is merged and this is rebased on master.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-24908][sql-client] Print the outermost exception that ends with the root cause [flink]
Posted by "MartijnVisser (via GitHub)" <gi...@apache.org>.
MartijnVisser merged PR #29334:
URL: https://github.com/apache/flink/pull/29334
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org