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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
