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