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