feat(isthmus-cli): explain common SQL conversion errors instead of dumping stack traces

[nielspardon]

isthmus "SELECT * FROM foo" answered a beginner's first attempt with a 40-line CalciteContextException stack trace that never mentioned -c / --create. Every other way to get the input wrong behaved the same way, up to an outright NullPointerException when no query was passed at all.

$ isthmus "SELECT * FROM foo"
Error: From line 1, column 15 to line 1, column 17: Object 'FOO' not found

Hint: table definitions are not part of the query. Pass a CREATE TABLE
statement for each table it references using -c / --create:

  isthmus -c "CREATE TABLE FOO (col1 INT, col2 VARCHAR)" "SELECT * FROM FOO"

Unquoted identifiers are upper-cased unless --unquotedcasing says otherwise.

What changes

• Failures caused by the input are reported as a message, with a hint naming the option to reach for wherever the mistake is identifiable: an undefined table points at -c / --create, naming the table and the schema Calcite looked in; an unresolved column or an unresolved identifier in -e / --expression explains where columns come from; a CREATE TABLE given as the query, or a query given to -c, points at the other one; and a query swallowed by the greedy -e says so. Every hint that quotes an identifier back also notes that unquoted identifiers are upper-cased — unless --unquotedcasing was given, where the note would be both wrong and beside the point.
• Anything that is not recognizably an input problem keeps its stack trace, so defects still reach bug reports intact. A SqlParseException carrying no position did not come from the grammar, and counts as a defect rather than as a mistake in the SQL. The new --stacktrace prints the message and the full trace for the recognized failures.
• Missing and contradictory input are usage errors. With neither a query nor -e the CLI threw a NullPointerException; with both, it silently discarded the query. Both now print a message plus the usage and exit 2. main no longer parses arguments ahead of execute() either, so an unquoted or mistyped argument gets picocli's usage error instead of an UnmatchedArgumentException trace.
• A CREATE TABLE with no column list no longer throws a NullPointerException. Calcite's DDL grammar makes the column list and the AS query independently optional, so both -c "CREATE TABLE foo" and -c "CREATE TABLE foo AS SELECT 1" reached a @NonNull parameter with a null column list, on both of the parser's two paths — only one of which had a guard, and only against the CTAS half.
• Syntax errors report their position in the native image. Calcite assembles the expected-token list for a SqlParseException by reflectively calling three grammar productions on the parser, which the native image was never told to keep, so every syntax error came out as RuntimeException: While building token lists instead of as a position in the SQL. That was invisible while every error printed a trace anyway, and it is what made this feature a no-op for malformed SQL in the shipped binary.

The isthmus-cli readme now documents the three exit codes, and its --help block reflows beyond the added line because the new option widens picocli's option column.

Closes #113

BREAKING CHANGE: isthmus with no arguments reports the missing SQL on stderr and exits 2, where it used to print the usage on stdout and exit 0; --help still does the latter. Passing a query together with -e / --expression is now rejected instead of silently ignoring the query.

[andrew-coleman]

The direction here is right and the hint text reads well. Three things I think need to change before this lands, plus a set of smaller ones inline.

The NPE is still reachable. Calcite's grammar makes the column list optional independently of AS query, so isthmus -c "CREATE TABLE FOO" "SELECT * FROM FOO" still dies with the columnList NPE and a full stack trace — on both the query and the -e path.

Internal defects are now silently swallowed. SqlParser wraps whatever the parser threw into a SqlParseException, so isthmus "" prints Error: Index 0 out of bounds for length 0 with no trace and no hint, where it used to print a trace that located the bug. That contradicts the promise the README adds in this PR.

-e swallows the positional query. isthmus -e "1 + 1" "SELECT 1" parses the query as an expression, and isthmus "SELECT 1" -e "1 + 1" exits 0 having silently discarded the query.

Several hints also fire for the wrong command line — they are selected purely from message text while parseResult is passed in and never consulted, so -e advice appears when -e was not used, the --unquotedcasing note appears when the casing was explicitly chosen, and the column hint tells you to check a CREATE TABLE statement that may not exist.

_{How this was checked: built installDist at da2e943e and reproduced each case from the CLI, then applied the suggested fixes and re-ran. With all of them applied, :isthmus:test is 961/961 and :isthmus-cli:test is 21/21 (up from 11), with pmdMain, spotlessCheck and javadoc clean. Every suggestion block below comes from that tree, so they should apply as-is. The suggestions on IsthmusExecutionExceptionHandler touch overlapping methods (hint gains a parseResult parameter) and are easiest to take together. The one thing I did not run end to end is the --stacktrace addition to smoke.sh, since that needs a native build.}

[andrew-coleman]

This literal is a copy of the one in isthmus/src/main/java/io/substrait/isthmus/calcite/rel/DdlSqlToRelConverter.java:91 — a different Gradle module — matched with .equals(). Reword it there (a plausible, unrelated change) and isthmus "CREATE TABLE foo(a INT)" silently goes back to dumping a stack trace, with everything still compiling and every test in isthmus still green. NOT_A_CREATE_TABLE and CTAS_NOT_SUPPORTED have the same problem against the inline literals in SubstraitCreateStatementParser.

A dedicated exception type is the real fix; failing that, public static final constants exposed by the throwing classes would at least turn a rename into a compile error.

Minor, same area: isPlainCreateTableQuery(ex) is evaluated twice per failure — once in isInputError (line 105) and again at the top of hint() (line 127).

[nielspardon]

Kept the literals, because CI does catch the rename: the CLI tests assert on all three messages end to end (ctasPassedToCreateOptionSuggestsQueryArgument, queryPassedToCreateOptionSuggestsQueryArgument, and createTableAsQuerySuggestsCreateOption for CTAS_ONLY, which only prints its hint when the message matches exactly), so rewording any of them turns :isthmus-cli:test red in the same build. Exposing them as public static final in :isthmus would make error-message text part of that module's API, which I would rather not do for a coupling the tests already pin — happy to add the constants, or a dedicated exception type, if you disagree.

Left the double isPlainCreateTableQuery evaluation: it is an instanceof plus a String.equals on a path that is about to print and exit, and hoisting it means either threading a boolean through hint() or reordering the two checks into one method that does both jobs.

[andrew-coleman]

Recognised input errors return ExitCode.SOFTWARE — picocli's internal software error code — which is also what an unrecognised crash returns:

$ isthmus "SELECT * FROM foo"                          # bad input, message printed -> 1
$ isthmus -c "CREATE TABLE FOO" "SELECT * FROM FOO"    # NPE, trace printed        -> 1

Meanwhile missing input deliberately exits 2 (ExitCode.USAGE). So two members of the newly created "the input is wrong" category get different codes, and a script wrapping isthmus cannot tell "fix your SQL" from "file a bug". The new tests bake ExitCode.SOFTWARE in, so the split is now pinned.

Worth deciding explicitly: either input errors join missing input on 2, or the distinction gets documented. Not a blocker, but easier to settle before release than after.

[nielspardon]

Settled it as documentation rather than a move to 2: 1 for "the SQL could not be converted", 2 for "the command line itself was wrong". That keeps --stacktrace-worthy defects and bad SQL on the same code, which is the part your example is really about, but it is the split a compiler makes too — bad source is a normal failure, not a usage error — and it means adding a hint for a failure never silently changes a script's exit code. The three codes are now a table in the isthmus-cli readme.

[nielspardon]

Thanks — everything reproduced, and all of it is fixed in 696ae64 except the two threads I replied to inline (the cross-module message literals, which the CLI tests already pin, and the exit-code split, which is now documented rather than unified).

Two things worth calling out beyond the review:

Bare isthmus now reports Missing SQL to convert on stderr and exits 2 instead of printing usage to stdout and exiting 0. That is your option 1 from the "two spellings disagree" thread, and it is a behavior change for anyone running the binary with no arguments to get its usage — --help still does that.

Chasing your note about not having run the --stacktrace addition against a native build turned up a native-only defect that the feature was quietly sitting on top of. Calcite builds a syntax error's expected-token list by reflectively calling three grammar productions on the parser (SqlAbstractParserImpl.MetadataImpl.initList), and those were not registered, so in the native image every syntax error came out as RuntimeException: While building token lists caused by NoSuchMethodException: SqlDdlParserImpl.ReservedFunctionName() — never as a SqlParseException, and so never as a friendly message. It was invisible before this PR because every error printed a trace anyway. RegisterAtRuntime now registers the three productions, and smoke.sh asserts a syntax error names its position, since the JVM tests cannot see this.

[andrew-coleman]

Approving. All fourteen threads are addressed, and the two you pushed back on I now agree with.

I re-ran everything rather than reading the diff. Each of the original reproductions against the new binary: the bare CREATE TABLE NPE is gone on both parser paths and also for IF NOT EXISTS and OR REPLACE, while CREATE TABLE FOO (a INT) AS SELECT 1 still reports CTAS not supported.; isthmus "" keeps its trace; the query-after--e case gets the new hint and query-plus--e exits 2 naming the discarded query; the -e hint no longer fires without -e; the qualified name comes out as S.U; the casing note disappears once --unquotedcasing is given; bare isthmus exits 2 while --help still exits 0 on stdout. :isthmus:test 961/961, :isthmus-cli:test 21/21, pmdMain, spotlessCheck, javadoc clean. I also emulated both new smoke.sh assertion blocks against the JVM build and they pass.

On the message literals — your argument holds and I checked both halves of it. pr.yml runs ./gradlew build --rerun-tasks, one build across all modules, so a rename in :isthmus does turn :isthmus-cli:test red in the same run; and the pinning is real, including for CTAS_ONLY, where asserting the hint pins the message because the hint only prints on an exact match. Dropping it. One note for whenever a typed exception does happen: COLUMNS_REQUIRED makes it four call sites rather than three.

On the exit codes — the README table matches the binary, and "bad source is a normal failure, not a usage error" is the line a compiler draws too. Settled.

On the native-image find — good catch, and your account is exact. I could not build a native image locally (Temurin here, not GraalVM), so I corroborated the chain from bytecode instead: SqlDdlParserImpl.getMetadata() constructs MetadataImpl, whose constructor calls initList with exactly those three production names, which reaches virtualCall → Class.getMethod → Method.invoke, and whose catch (Throwable) handler is literally new RuntimeException("While building token lists", cause) — the message you quoted, verbatim. All three productions are public final no-arg on SqlDdlParserImpl, so the getMethod call in registerMethods is a real build-time guard; it throws NoSuchMethodException, which the enclosing catch (Exception) rethrows as IllegalStateException out of beforeAnalysis, so the javadoc's "fails to build" promise holds. compileOnly is the right scope, since :isthmus already declares calcite-server as implementation. And it explains itself: getMetadata() is only reached for a ParseException, never a CalciteContextException, which is why the existing select * from lineitem smoke case never exposed it.

One inline comment on the casing note, and one thing that has no line to attach to:

The PR body has fallen behind the branch, and it is what gets committed. AGENTS.md lines 163-165: the title and body together become the squash-merge commit message that semantic-release builds CHANGELOG.md from, so the fix commit's (very good) message is discarded on merge. As it stands the body:

• claims the -e identifier hint mentions --unquotedcasing, which is not true today — see the inline comment;
• does not mention that bare isthmus changed from usage/exit 0 to error/exit 2. You called it out in a comment here, but a comment is not the commit message, and this is the change most likely to surprise someone scripting the binary;
• does not mention that a query passed together with -e is now rejected;
• does not mention the native-image reflection fix at all — a distinct defect from the one in the title, and the kind of thing that wants its own changelog line;
• still ends with 🤖 Generated with AI, which AGENTS.md lines 174-176 lists among the tool-attribution lines to keep out of commit bodies.

None of that blocks the code, so I am approving rather than holding it — but the body is worth a pass before you squash.

[andrew-coleman]

Non-blocking, but this is the one identifier hint that did not pick up withCasingNote, and it is arguably where casing bites hardest. isthmus -e "col + 1" reports Unknown identifier 'COL' and then suggests -e "COL + 1" — the input has been silently upper-cased in the very command being recommended, with nothing to explain why:

Error: From line 1, column 1 to line 1, column 3: Unknown identifier 'COL'

Hint: identifiers in a -e / --expression must be columns of a table defined
with -c / --create:

  isthmus -c "CREATE TABLE T (COL INT)" -e "COL + 1"

The other three hints all carry the note now, and the PR body still promises it for this one specifically ("an unresolved column or an unresolved identifier in -e / --expression ... mentions --unquotedcasing").

Suggested change

	return Optional.of(EXPRESSION_HINT.formatted(unknownIdentifier.group(1)));
	return Optional.of(
	withCasingNote(EXPRESSION_HINT.formatted(unknownIdentifier.group(1)), parseResult));

Verified: spotlessCheck and pmdMain clean, :isthmus-cli:test still 21/21, and the note appears.

Separately, on the comment just above — agreed that gating on -e is the right call, and noting for the record that the residual case is -e plus a bad -c: isthmus -e "1" -c "CREATE TABLE foo(a NOSUCHTYPE)" still suggests CREATE TABLE T (NOSUCHTYPE INT). Lower severity than before since the user did pass -e, and the real cure is the typed exception, so I would leave it.

[nielspardon]

Taken in c35c7ed — you are right that this is where the casing bites hardest, since the hint hands back a spelling the user never typed. Extended unknownIdentifierInExpressionSuggestsCreateOption to assert the note so it cannot drift off again.

Agreed on leaving the -e plus bad -c residual: the identifier hint is wrong there, but the user did ask for expressions, and separating the two needs the exception type rather than another message match.

[nielspardon]

Thanks for re-running it all rather than reading the diff — and for corroborating the native-image chain from bytecode, which is a better check than my "it works now" was.

The casing note is on the -e hint in c35c7ed. The PR body has had the pass you asked for: the AI attribution line is gone, the --unquotedcasing claim is true again rather than dropped, and it now covers the bare-isthmus change, the query-plus--e rejection, and the native-image reflection fix as its own item. The two script-visible changes are also a BREAKING CHANGE footer, so they get a line in the release notes instead of only living in the body.

Noted on COLUMNS_REQUIRED being a fourth call site whenever the typed exception happens.