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 (with its own name in the example), an unresolved column or an unresolved identifier in -e / --expression explains where columns come from and mentions --unquotedcasing, and a CREATE TABLE given as the query — or a query given to -c — points at the other one.
• Anything that is not recognizably an input problem keeps its stack trace, so defects still reach bug reports intact. The new --stacktrace prints the message and the full trace for the recognized failures.
• Missing input is a usage error. With neither a query nor -e the CLI threw a NullPointerException; it now prints Missing SQL to convert plus the usage. Relatedly, main no longer parses arguments ahead of execute(), so an unquoted or mistyped argument gets picocli's usage error rather than an UnmatchedArgumentException trace. Both exit 2, picocli's invalid-input code, where the uncaught exceptions previously exited 1.
• Guards processCreateStatementsToSchema against CTAS, as its sibling processCreateStatements already did. Without the guard, -c "CREATE TABLE foo AS SELECT 1" dereferenced a null column list — a parameter declared @NonNull — and failed with an NPE instead of CTAS not supported.

The --help block in the isthmus-cli readme reflows beyond the added line because the new option widens picocli's option column.

Closes #113

🤖 Generated with AI

[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.