From da2e943e7ae5366a9427997552b0d197c4424256 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Wed, 5 Aug 2026 13:26:48 +0200 Subject: [PATCH 1/3] feat(isthmus-cli): explain common SQL conversion errors instead of dumping stack traces Failures caused by the SQL given to the CLI are now reported as a message plus a hint naming the option to reach for, rather than as a raw Calcite stack trace. An undefined table points at -c / --create, an unresolved column or identifier explains where columns come from, and a CREATE TABLE passed as the query -- or a query passed to -c -- points at the other one. Anything that is not recognizably an input problem keeps its stack trace, and --stacktrace restores it for the recognized ones. Missing input is a usage error now: with neither a query nor -e the CLI reported a NullPointerException, and main no longer parses arguments ahead of execute(), so a mistyped argument gets picocli's usage error instead of an UnmatchedArgumentException trace. Also guards processCreateStatementsToSchema against CREATE TABLE AS SELECT, as its sibling processCreateStatements already did; without it a CTAS statement in -c dereferenced a null column list. Closes #113 --- isthmus-cli/README.md | 43 +++-- .../isthmus/cli/IsthmusEntryPoint.java | 57 ++++-- .../cli/IsthmusExecutionExceptionHandler.java | 165 ++++++++++++++++++ .../isthmus/cli/IsthmusEntryPointTest.java | 139 ++++++++++++++- isthmus-cli/src/test/script/smoke.sh | 16 ++ .../sql/SubstraitCreateStatementParser.java | 5 + .../SubstraitCreateStatementParserTest.java | 9 + 7 files changed, 400 insertions(+), 34 deletions(-) create mode 100644 isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusExecutionExceptionHandler.java diff --git a/isthmus-cli/README.md b/isthmus-cli/README.md index c5af2b2f7..973131e39 100644 --- a/isthmus-cli/README.md +++ b/isthmus-cli/README.md @@ -27,26 +27,47 @@ isthmus 0.1 ``` $ ./isthmus-cli/build/native/nativeCompile/isthmus --help -Usage: isthmus [-hV] [--outputformat=] +Usage: isthmus [-hV] [--stacktrace] [--outputformat=] [--unquotedcasing=] [-c=]... [-e=...]... [] Convert SQL Queries and SQL Expressions to Substrait - [] A SQL query + [] A SQL query -c, --create= - One or multiple create table statements e.g. CREATE - TABLE T1(foo int, bar bigint) + One or multiple create table statements e.g. CREATE TABLE + T1(foo int, bar bigint) -e, --expression=... - One or more SQL expressions e.g. col + 1 - -h, --help Show this help message and exit. + One or more SQL expressions e.g. col + 1 + -h, --help Show this help message and exit. --outputformat= - Set the output format for the generated plan: - PROTOJSON, PROTOTEXT, BINARY + Set the output format for the generated plan: PROTOJSON, + PROTOTEXT, BINARY + --stacktrace Print the full stack trace of any error, not just its + message --unquotedcasing= - Calcite's casing policy for unquoted identifiers: - UNCHANGED, TO_UPPER, TO_LOWER - -V, --version Print version information and exit. + Calcite's casing policy for unquoted identifiers: + UNCHANGED, TO_UPPER, TO_LOWER + -V, --version Print version information and exit. ``` +### Errors + +A mistake in the SQL is reported as a message, along with a hint about the option to reach for where one applies: + +``` +$ ./isthmus-cli/build/native/nativeCompile/isthmus "SELECT lastName FROM Persons" + +Error: From line 1, column 22 to line 1, column 28: Object 'PERSONS' 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 PERSONS (col1 INT, col2 VARCHAR)" "SELECT * FROM PERSONS" + +Unquoted identifiers are upper-cased unless --unquotedcasing says otherwise. +``` + +Add `--stacktrace` to get the full stack trace as well. Anything that is not a recognizable problem with the input is always reported with its stack trace. + ## Example ### SQL to Substrait Plan diff --git a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java index 8c4a40098..cf007f981 100644 --- a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java +++ b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java @@ -17,8 +17,11 @@ import org.apache.calcite.prepare.Prepare; import picocli.CommandLine; import picocli.CommandLine.Command; +import picocli.CommandLine.Model.CommandSpec; import picocli.CommandLine.Option; +import picocli.CommandLine.ParameterException; import picocli.CommandLine.Parameters; +import picocli.CommandLine.Spec; /** Isthmus CLI entry point. */ @Command( @@ -59,33 +62,57 @@ enum OutputFormat { description = "Calcite's casing policy for unquoted identifiers: ${COMPLETION-CANDIDATES}") private Casing unquotedCasing = Casing.TO_UPPER; + @Option( + names = {"--stacktrace"}, + description = "Print the full stack trace of any error, not just its message") + private boolean stackTrace; + + @Spec private CommandSpec spec; + /** * Standard Java Main method invoked by the isthmus CLI command. * * @param args Isthmus CLI arguments. */ public static void main(String... args) { + CommandLine commandLine = createCommandLine(); + if (args.length == 0) { // If no arguments print usage help + commandLine.usage(commandLine.getOut()); + System.exit(CommandLine.ExitCode.OK); + } + System.exit(commandLine.execute(args)); + } + + /** + * Creates the {@link CommandLine} driving the isthmus CLI. Errors caused by the given SQL are + * reported by {@link IsthmusExecutionExceptionHandler} rather than as a stack trace. + * + * @return the configured {@link CommandLine} + */ + static CommandLine createCommandLine() { CommandLine commandLine = new CommandLine(new IsthmusEntryPoint()); commandLine.setCaseInsensitiveEnumValuesAllowed(true); - CommandLine.ParseResult parseResult = commandLine.parseArgs(args); - if (parseResult.originalArgs().isEmpty()) { // If no arguments print usage help - commandLine.usage(System.out); - System.exit(0); - } - if (commandLine.isUsageHelpRequested()) { - commandLine.usage(System.out); - System.exit(0); - } - if (commandLine.isVersionHelpRequested()) { - commandLine.printVersionHelp(System.out); - System.exit(0); - } - int exitCode = commandLine.execute(args); - System.exit(exitCode); + commandLine.setExecutionExceptionHandler(new IsthmusExecutionExceptionHandler()); + return commandLine; + } + + /** + * Reports whether the full stack trace of an error was asked for on the command line. + * + * @return true if {@code --stacktrace} was given + */ + boolean isStackTraceRequested() { + return stackTrace; } @Override public Integer call() throws Exception { + if (sqlExpressions == null && sql == null) { + throw new ParameterException( + spec.commandLine(), + "Missing SQL to convert: pass a SQL query as the first argument, " + + "or SQL expressions with -e / --expression"); + } ConverterProvider provider = ConverterProvider.builder().unquotedCasing(unquotedCasing).build(); // Isthmus image is parsing SQL Expression if that argument is defined if (sqlExpressions != null) { diff --git a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusExecutionExceptionHandler.java b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusExecutionExceptionHandler.java new file mode 100644 index 000000000..bd1a82c71 --- /dev/null +++ b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusExecutionExceptionHandler.java @@ -0,0 +1,165 @@ +package io.substrait.isthmus.cli; + +import java.io.PrintWriter; +import java.util.Optional; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import org.apache.calcite.runtime.CalciteContextException; +import org.apache.calcite.sql.parser.SqlParseException; +import picocli.CommandLine; + +/** + * Reports mistakes in the SQL handed to the CLI as a short message, with a hint about the option to + * reach for whenever the mistake can be identified. + * + *

Only failures that are recognizably caused by the input are reported this way. Anything else + * is rethrown so that its stack trace is still printed, as is the full stack trace of a recognized + * failure when {@code --stacktrace} is given. + */ +class IsthmusExecutionExceptionHandler implements CommandLine.IExecutionExceptionHandler { + + /** Matches Calcite's complaint about a table (or other object) that the catalog does not hold. */ + private static final Pattern OBJECT_NOT_FOUND = + Pattern.compile("(?:Object|Table) '([^']+)' not found"); + + /** Matches Calcite's complaint about a column that none of the known tables holds. */ + private static final Pattern COLUMN_NOT_FOUND = Pattern.compile("Column '([^']+)' not found"); + + /** Matches Calcite's complaint about an identifier that an expression cannot be resolved to. */ + private static final Pattern UNKNOWN_IDENTIFIER = Pattern.compile("Unknown identifier '([^']+)'"); + + /** The message the CREATE statement parser reports for anything that is not a CREATE TABLE. */ + private static final String NOT_A_CREATE_TABLE = "Not a valid CREATE TABLE statement."; + + /** The message the CREATE statement parser reports for a CREATE TABLE AS SELECT. */ + private static final String CTAS_NOT_SUPPORTED = "CTAS not supported."; + + /** The message the DDL converter reports for a CREATE TABLE without a query. */ + private static final String CTAS_ONLY = "Only create table as select statements are supported"; + + // The hints are hard-wrapped for a terminal rather than joined into single long lines. + + private static final String CREATE_HINT = + """ + 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 %1$s (col1 INT, col2 VARCHAR)" "SELECT * FROM %1$s" + + Unquoted identifiers are upper-cased unless --unquotedcasing says otherwise."""; + + private static final String COLUMN_HINT = + """ + Hint: '%s' is not a column of any table defined with -c / --create. Check + the column names in the CREATE TABLE statement; unquoted identifiers are + upper-cased unless --unquotedcasing says otherwise."""; + + private static final String EXPRESSION_HINT = + """ + Hint: identifiers in a -e / --expression must be columns of a table defined + with -c / --create: + + isthmus -c "CREATE TABLE T (%1$s INT)" -e "%1$s + 1\""""; + + private static final String QUERY_ARGUMENT_HINT = + """ + Hint: -c / --create takes plain CREATE TABLE statements; the query itself is + the first argument: + + isthmus -c "CREATE TABLE FOO (col1 INT)" "SELECT * FROM FOO\""""; + + @Override + public int handleExecutionException( + Exception ex, CommandLine cmd, CommandLine.ParseResult parseResult) throws Exception { + if (!isInputError(ex)) { + // Not something the user can act on: keep the stack trace, it belongs in a bug report. + throw ex; + } + + PrintWriter err = cmd.getErr(); + err.println(cmd.getColorScheme().errorText("Error: " + ex.getMessage())); + Optional hint = hint(ex); + if (hint.isPresent()) { + err.println(); + err.println(hint.get()); + } + err.flush(); + + if (stackTraceRequested(cmd)) { + throw ex; + } + return cmd.getCommandSpec().exitCodeOnExecutionException(); + } + + /** + * Reports whether the given exception was caused by the SQL given to the CLI rather than by a + * defect. + * + * @param ex the exception thrown while converting + * @return true if the exception describes a problem with the input + */ + private static boolean isInputError(final Exception ex) { + // CalciteContextException is, by construction, a complaint about the SQL at a line and column. + return ex instanceof SqlParseException + || ex instanceof CalciteContextException + || isPlainCreateTableQuery(ex); + } + + /** + * Reports whether the given exception is the DDL converter's rejection of a CREATE TABLE + * statement passed as the query. + * + * @param ex the exception thrown while converting + * @return true if a plain CREATE TABLE statement was given as the query + */ + private static boolean isPlainCreateTableQuery(final Exception ex) { + return ex instanceof IllegalArgumentException && CTAS_ONLY.equals(ex.getMessage()); + } + + /** + * Returns the hint to print for the given exception, if its message identifies a mistake we can + * advise on. + * + * @param ex the exception thrown while converting + * @return the hint to print below the error message, or empty if the mistake is not recognized + */ + private static Optional hint(final Exception ex) { + if (isPlainCreateTableQuery(ex)) { + return Optional.of(QUERY_ARGUMENT_HINT); + } + + String message = ex.getMessage(); + if (message == null) { + return Optional.empty(); + } + if (message.contains(NOT_A_CREATE_TABLE) || message.contains(CTAS_NOT_SUPPORTED)) { + return Optional.of(QUERY_ARGUMENT_HINT); + } + + Matcher objectNotFound = OBJECT_NOT_FOUND.matcher(message); + if (objectNotFound.find()) { + return Optional.of(CREATE_HINT.formatted(objectNotFound.group(1))); + } + Matcher columnNotFound = COLUMN_NOT_FOUND.matcher(message); + if (columnNotFound.find()) { + return Optional.of(COLUMN_HINT.formatted(columnNotFound.group(1))); + } + Matcher unknownIdentifier = UNKNOWN_IDENTIFIER.matcher(message); + if (unknownIdentifier.find()) { + return Optional.of(EXPRESSION_HINT.formatted(unknownIdentifier.group(1))); + } + return Optional.empty(); + } + + /** + * Reports whether the full stack trace was asked for on the command line. + * + * @param cmd the command line that failed + * @return true if {@code --stacktrace} was given + */ + private static boolean stackTraceRequested(final CommandLine cmd) { + Object command = cmd.getCommand(); + return command instanceof IsthmusEntryPoint + && ((IsthmusEntryPoint) command).isStackTraceRequested(); + } +} diff --git a/isthmus-cli/src/test/java/io/substrait/isthmus/cli/IsthmusEntryPointTest.java b/isthmus-cli/src/test/java/io/substrait/isthmus/cli/IsthmusEntryPointTest.java index 1a4fe42c8..b4dc5be5e 100644 --- a/isthmus-cli/src/test/java/io/substrait/isthmus/cli/IsthmusEntryPointTest.java +++ b/isthmus-cli/src/test/java/io/substrait/isthmus/cli/IsthmusEntryPointTest.java @@ -1,25 +1,148 @@ package io.substrait.isthmus.cli; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import java.io.PrintWriter; +import java.io.StringWriter; import org.junit.jupiter.api.Test; import picocli.CommandLine; class IsthmusEntryPointTest { + /** The output the CLI wrote to stderr, and the status code it exited with. */ + private static final class Run { + final int statusCode; + final String err; + + Run(int statusCode, String err) { + this.statusCode = statusCode; + this.err = err; + } + + /** Asserts that the error was reported as a message rather than as a stack trace. */ + void assertNoStackTrace() { + assertFalse(err.contains("\tat "), () -> "unexpected stack trace in:\n" + err); + } + + /** + * Asserts that stderr contains the given snippets. + * + * @param snippets the snippets stderr must contain + */ + void assertErrContains(String... snippets) { + for (String snippet : snippets) { + assertTrue(err.contains(snippet), () -> "expected '" + snippet + "' in:\n" + err); + } + } + } + + /** + * Runs the CLI with the given arguments, capturing what it writes to stderr. + * + * @param args the command line arguments + * @return the captured stderr and the status code + */ + private static Run run(String... args) { + CommandLine cli = IsthmusEntryPoint.createCommandLine(); + StringWriter err = new StringWriter(); + cli.setErr(new PrintWriter(err)); + int statusCode = cli.execute(args); + cli.getErr().flush(); + return new Run(statusCode, err.toString()); + } + @Test void canProcessQuery() { - IsthmusEntryPoint isthmusEntryPoint = new IsthmusEntryPoint(); - CommandLine cli = new CommandLine(isthmusEntryPoint); - int statusCode = cli.execute("SELECT 1;"); - assertEquals(0, statusCode); + assertEquals(0, run("SELECT 1;").statusCode); } @Test void canProcessQueryWithCreates() { - IsthmusEntryPoint isthmusEntryPoint = new IsthmusEntryPoint(); - CommandLine cli = new CommandLine(isthmusEntryPoint); - int statusCode = cli.execute("SELECT * FROM foo", "--create", "CREATE TABLE foo(id INT)"); - assertEquals(0, statusCode); + assertEquals(0, run("SELECT * FROM foo", "--create", "CREATE TABLE foo(id INT)").statusCode); + } + + @Test + void undefinedTableSuggestsCreateOption() { + Run run = run("SELECT * FROM foo"); + + assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); + run.assertErrContains("Object 'FOO' not found", "-c / --create", "CREATE TABLE FOO"); + run.assertNoStackTrace(); + } + + @Test + void undefinedColumnIsExplained() { + Run run = run("SELECT bar FROM foo", "-c", "CREATE TABLE foo(id INT)"); + + assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); + run.assertErrContains("Column 'BAR' not found", "'BAR' is not a column", "--unquotedcasing"); + run.assertNoStackTrace(); + } + + @Test + void unknownIdentifierInExpressionSuggestsCreateOption() { + Run run = run("-e", "col + 1"); + + assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); + run.assertErrContains("Unknown identifier 'COL'", "-e / --expression", "-c / --create"); + run.assertNoStackTrace(); + } + + @Test + void malformedQueryReportsTheParseError() { + Run run = run("SELECT 1 FROM"); + + assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); + run.assertErrContains("Encountered \"\" at line 1, column 13."); + run.assertNoStackTrace(); + } + + @Test + void createTableAsQuerySuggestsCreateOption() { + Run run = run("CREATE TABLE foo(a INT)"); + + assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); + run.assertErrContains("-c / --create takes plain CREATE TABLE statements"); + run.assertNoStackTrace(); + } + + @Test + void queryPassedToCreateOptionSuggestsQueryArgument() { + Run run = run("SELECT * FROM foo", "-c", "SELECT 1"); + + assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); + run.assertErrContains( + "Not a valid CREATE TABLE statement.", "-c / --create takes plain CREATE TABLE statements"); + run.assertNoStackTrace(); + } + + @Test + void ctasPassedToCreateOptionSuggestsQueryArgument() { + Run run = run("SELECT * FROM foo", "-c", "CREATE TABLE foo AS SELECT 1"); + + assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); + run.assertErrContains( + "CTAS not supported.", "-c / --create takes plain CREATE TABLE statements"); + run.assertNoStackTrace(); + } + + @Test + void missingSqlReportsUsageError() { + Run run = run("-c", "CREATE TABLE foo(id INT)"); + + assertEquals(CommandLine.ExitCode.USAGE, run.statusCode); + run.assertErrContains("Missing SQL to convert", "Usage: isthmus"); + run.assertNoStackTrace(); + } + + @Test + void stackTraceOptionKeepsTheStackTrace() { + Run run = run("SELECT * FROM foo", "--stacktrace"); + + assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); + run.assertErrContains( + "Object 'FOO' not found", "-c / --create", "org.apache.calcite.runtime", "\tat "); } } diff --git a/isthmus-cli/src/test/script/smoke.sh b/isthmus-cli/src/test/script/smoke.sh index 9b7fd84cb..bc0c647cc 100755 --- a/isthmus-cli/src/test/script/smoke.sh +++ b/isthmus-cli/src/test/script/smoke.sh @@ -39,3 +39,19 @@ echo "${LINEITEM}" # SQL Expression - 03 Projection expression (column-1, column-2, column-3) "${CMD}" --expression 'l_orderkey + 9888486986' 'l_orderkey * 2' 'l_orderkey > 10' 'l_orderkey in (10, 20)' --create "${LINEITEM}" + +# Error path - a query over an undefined table must be reported as a message hinting at --create, +# not as a Java stack trace +if error=$("${CMD}" 'select * from lineitem' 2>&1); then + echo "expected a query without a table definition to fail" + exit 1 +fi +echo "${error}" +if ! grep -q -- '--create' <<<"${error}"; then + echo "expected a hint about --create" + exit 1 +fi +if grep -q 'CalciteContextException' <<<"${error}"; then + echo "expected a message instead of a stack trace" + exit 1 +fi diff --git a/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitCreateStatementParser.java b/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitCreateStatementParser.java index bf0c2c6cb..0fe44388c 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitCreateStatementParser.java +++ b/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitCreateStatementParser.java @@ -215,6 +215,11 @@ private static CalciteSchema processCreateStatementsToSchema( } final SqlCreateTable create = (SqlCreateTable) parsed; + + if (create.query != null) { + throw fail("CTAS not supported.", create.name.getParserPosition()); + } + final List names = create.name.names; final CalciteSchema schema = diff --git a/isthmus/src/test/java/io/substrait/isthmus/sql/SubstraitCreateStatementParserTest.java b/isthmus/src/test/java/io/substrait/isthmus/sql/SubstraitCreateStatementParserTest.java index 004a3290d..37ebe3c59 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/sql/SubstraitCreateStatementParserTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/sql/SubstraitCreateStatementParserTest.java @@ -119,4 +119,13 @@ void testToCatalogWithMultipleCreateTableForSameTableThrowsException() throws Sq "create table src1 (intcol int, charcol varchar(10))", "create table src1 (intcol int, charcol varchar(20))")); } + + @Test + void testToCatalogWithCreateTableAsSelectThrowsException() { + assertThrows( + SqlParseException.class, + () -> + SubstraitCreateStatementParser.processCreateStatementsToCatalog( + "create table src1 as select 1")); + } } From 696ae64169ef468ca12517ff2838278e63ea7305 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Mon, 10 Aug 2026 17:59:25 +0200 Subject: [PATCH 2/3] fix(isthmus-cli): address review of the friendly error handling Close the remaining raw-trace and NPE paths the first pass left behind: - CREATE TABLE without a column list NPEd on both parser paths, not just the CTAS one; both now go through a shared guard. - A SqlParseException without a position did not come from the grammar, so it is a defect and keeps its stack trace instead of being reported as a mistake in the SQL. - -e / --expression is greedy, so a query written after it was silently parsed as an expression; giving both is now a usage error, and a query swallowed by -e is explained. - Hints are gated on the options actually given: the column hint no longer points at -c when no table was defined, the identifier hint no longer blames -e when the failure came from a -c statement, the table hint names the schema Calcite looked in, and the casing note is dropped once --unquotedcasing was chosen. - Calcite builds a syntax error's expected-token list by reflecting on the parser, which the native image could not do, so every syntax error surfaced as that reflection failure rather than as a position in the SQL. Register the three productions it calls. Bare `isthmus` now reports the missing SQL and exits 2 rather than printing usage and exiting 0, which is what every other missing-argument case does. --- isthmus-cli/README.md | 14 ++- isthmus-cli/build.gradle.kts | 8 ++ .../isthmus/cli/IsthmusEntryPoint.java | 25 ++-- .../cli/IsthmusExecutionExceptionHandler.java | 115 ++++++++++++++---- .../isthmus/cli/RegisterAtRuntime.java | 25 ++++ .../isthmus/cli/IsthmusEntryPointTest.java | 113 ++++++++++++++++- isthmus-cli/src/test/script/smoke.sh | 36 +++++- .../sql/SubstraitCreateStatementParser.java | 28 ++++- .../SubstraitCreateStatementParserTest.java | 47 ++++++- 9 files changed, 351 insertions(+), 60 deletions(-) diff --git a/isthmus-cli/README.md b/isthmus-cli/README.md index 973131e39..6a6cc27ad 100644 --- a/isthmus-cli/README.md +++ b/isthmus-cli/README.md @@ -41,8 +41,8 @@ Convert SQL Queries and SQL Expressions to Substrait --outputformat= Set the output format for the generated plan: PROTOJSON, PROTOTEXT, BINARY - --stacktrace Print the full stack trace of any error, not just its - message + --stacktrace Print the full stack trace of a conversion failure, not + just its message --unquotedcasing= Calcite's casing policy for unquoted identifiers: UNCHANGED, TO_UPPER, TO_LOWER @@ -66,7 +66,15 @@ statement for each table it references using -c / --create: Unquoted identifiers are upper-cased unless --unquotedcasing says otherwise. ``` -Add `--stacktrace` to get the full stack trace as well. Anything that is not a recognizable problem with the input is always reported with its stack trace. +Add `--stacktrace` to get the full stack trace of the failure as well. A conversion that fails for a reason that is not a recognizable problem with the input is always reported with its stack trace. + +The exit code distinguishes the two kinds of mistake: + +| Code | Meaning | +| --- | --- | +| 0 | The plan or extended expression was written to stdout | +| 1 | The SQL could not be converted, or the conversion hit a defect | +| 2 | The command line itself was wrong, e.g. no SQL was given | ## Example diff --git a/isthmus-cli/build.gradle.kts b/isthmus-cli/build.gradle.kts index ec1af73a7..93a99276d 100644 --- a/isthmus-cli/build.gradle.kts +++ b/isthmus-cli/build.gradle.kts @@ -26,6 +26,14 @@ dependencies { implementation(libs.classgraph) implementation(libs.guava) compileOnly(libs.graal.sdk) + // Only to name the parser class the native image must keep reflectively callable; :isthmus + // brings it along at runtime. + compileOnly(libs.calcite.server) { + exclude(group = "commons-lang", module = "commons-lang") + .because( + "calcite-core brings in commons-lang:commons-lang:2.4 which has a security vulnerability" + ) + } implementation(libs.picocli) annotationProcessor(libs.picocli.codegen) implementation(libs.protobuf.java.util) { diff --git a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java index cf007f981..a7172c0a9 100644 --- a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java +++ b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java @@ -64,7 +64,7 @@ enum OutputFormat { @Option( names = {"--stacktrace"}, - description = "Print the full stack trace of any error, not just its message") + description = "Print the full stack trace of a conversion failure, not just its message") private boolean stackTrace; @Spec private CommandSpec spec; @@ -75,12 +75,7 @@ enum OutputFormat { * @param args Isthmus CLI arguments. */ public static void main(String... args) { - CommandLine commandLine = createCommandLine(); - if (args.length == 0) { // If no arguments print usage help - commandLine.usage(commandLine.getOut()); - System.exit(CommandLine.ExitCode.OK); - } - System.exit(commandLine.execute(args)); + System.exit(createCommandLine().execute(args)); } /** @@ -96,15 +91,6 @@ static CommandLine createCommandLine() { return commandLine; } - /** - * Reports whether the full stack trace of an error was asked for on the command line. - * - * @return true if {@code --stacktrace} was given - */ - boolean isStackTraceRequested() { - return stackTrace; - } - @Override public Integer call() throws Exception { if (sqlExpressions == null && sql == null) { @@ -113,6 +99,13 @@ public Integer call() throws Exception { "Missing SQL to convert: pass a SQL query as the first argument, " + "or SQL expressions with -e / --expression"); } + if (sqlExpressions != null && sql != null) { + throw new ParameterException( + spec.commandLine(), + "Give either a SQL query or -e / --expression, not both: the query '" + + sql + + "' would be ignored"); + } ConverterProvider provider = ConverterProvider.builder().unquotedCasing(unquotedCasing).build(); // Isthmus image is parsing SQL Expression if that argument is defined if (sqlExpressions != null) { diff --git a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusExecutionExceptionHandler.java b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusExecutionExceptionHandler.java index bd1a82c71..db77db5ec 100644 --- a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusExecutionExceptionHandler.java +++ b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusExecutionExceptionHandler.java @@ -18,9 +18,12 @@ */ class IsthmusExecutionExceptionHandler implements CommandLine.IExecutionExceptionHandler { - /** Matches Calcite's complaint about a table (or other object) that the catalog does not hold. */ + /** + * Matches Calcite's complaint about a table (or other object) that the catalog does not hold. The + * second group holds the schema it looked in, for the qualified form of the message. + */ private static final Pattern OBJECT_NOT_FOUND = - Pattern.compile("(?:Object|Table) '([^']+)' not found"); + Pattern.compile("(?:Object|Table) '([^']+)' not found(?: within '([^']+)')?"); /** Matches Calcite's complaint about a column that none of the known tables holds. */ private static final Pattern COLUMN_NOT_FOUND = Pattern.compile("Column '([^']+)' not found"); @@ -28,12 +31,19 @@ class IsthmusExecutionExceptionHandler implements CommandLine.IExecutionExceptio /** Matches Calcite's complaint about an identifier that an expression cannot be resolved to. */ private static final Pattern UNKNOWN_IDENTIFIER = Pattern.compile("Unknown identifier '([^']+)'"); + /** Matches Calcite's complaint about a query given where an expression was expected. */ + private static final Pattern STATEMENT_AS_EXPRESSION = + Pattern.compile("Incorrect syntax near the keyword '(?:SELECT|WITH|VALUES|TABLE)'"); + /** The message the CREATE statement parser reports for anything that is not a CREATE TABLE. */ private static final String NOT_A_CREATE_TABLE = "Not a valid CREATE TABLE statement."; /** The message the CREATE statement parser reports for a CREATE TABLE AS SELECT. */ private static final String CTAS_NOT_SUPPORTED = "CTAS not supported."; + /** The message the CREATE statement parser reports for a CREATE TABLE without a column list. */ + private static final String COLUMNS_REQUIRED = "Column definitions are required."; + /** The message the DDL converter reports for a CREATE TABLE without a query. */ private static final String CTAS_ONLY = "Only create table as select statements are supported"; @@ -44,15 +54,19 @@ class IsthmusExecutionExceptionHandler implements CommandLine.IExecutionExceptio 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 %1$s (col1 INT, col2 VARCHAR)" "SELECT * FROM %1$s" - - Unquoted identifiers are upper-cased unless --unquotedcasing says otherwise."""; + isthmus -c "CREATE TABLE %1$s (col1 INT, col2 VARCHAR)" "SELECT * FROM %1$s\""""; private static final String COLUMN_HINT = """ - Hint: '%s' is not a column of any table defined with -c / --create. Check - the column names in the CREATE TABLE statement; unquoted identifiers are - upper-cased unless --unquotedcasing says otherwise."""; + Hint: '%1$s' is not a column of any table defined with -c / --create. Check + the column names in the CREATE TABLE statement."""; + + private static final String COLUMN_WITHOUT_CREATE_HINT = + """ + Hint: '%1$s' is not a column of any table, and no table was defined. Pass a + CREATE TABLE statement for each table the query reads using -c / --create: + + isthmus -c "CREATE TABLE T (%1$s INT)" "SELECT %1$s FROM T\""""; private static final String EXPRESSION_HINT = """ @@ -61,6 +75,13 @@ class IsthmusExecutionExceptionHandler implements CommandLine.IExecutionExceptio isthmus -c "CREATE TABLE T (%1$s INT)" -e "%1$s + 1\""""; + private static final String EXPRESSION_ARGUMENT_HINT = + """ + Hint: -e / --expression consumes every following argument, so a query + written after it is parsed as an expression. Pass the query on its own: + + isthmus "SELECT * FROM FOO\""""; + private static final String QUERY_ARGUMENT_HINT = """ Hint: -c / --create takes plain CREATE TABLE statements; the query itself is @@ -68,6 +89,16 @@ class IsthmusExecutionExceptionHandler implements CommandLine.IExecutionExceptio isthmus -c "CREATE TABLE FOO (col1 INT)" "SELECT * FROM FOO\""""; + private static final String COLUMN_LIST_HINT = + """ + Hint: -c / --create needs the columns of the table, with their types: + + isthmus -c "CREATE TABLE FOO (col1 INT, col2 VARCHAR)" "SELECT * FROM FOO\""""; + + /** Appended to the hints whose advice depends on how unquoted identifiers are cased. */ + private static final String CASING_NOTE = + "\n\nUnquoted identifiers are upper-cased unless --unquotedcasing says otherwise."; + @Override public int handleExecutionException( Exception ex, CommandLine cmd, CommandLine.ParseResult parseResult) throws Exception { @@ -78,14 +109,14 @@ public int handleExecutionException( PrintWriter err = cmd.getErr(); err.println(cmd.getColorScheme().errorText("Error: " + ex.getMessage())); - Optional hint = hint(ex); + Optional hint = hint(ex, parseResult); if (hint.isPresent()) { err.println(); err.println(hint.get()); } err.flush(); - if (stackTraceRequested(cmd)) { + if (parseResult.hasMatchedOption("--stacktrace")) { throw ex; } return cmd.getCommandSpec().exitCodeOnExecutionException(); @@ -99,10 +130,15 @@ public int handleExecutionException( * @return true if the exception describes a problem with the input */ private static boolean isInputError(final Exception ex) { + if (ex instanceof SqlParseException) { + // Calcite funnels whatever the parser threw into a SqlParseException, leaving the position + // null unless the cause was a grammar or lexer error + // (SqlAbstractParserImpl.convertException). A missing position therefore means a defect + // rather than a problem with the SQL. + return ((SqlParseException) ex).getPos() != null; + } // CalciteContextException is, by construction, a complaint about the SQL at a line and column. - return ex instanceof SqlParseException - || ex instanceof CalciteContextException - || isPlainCreateTableQuery(ex); + return ex instanceof CalciteContextException || isPlainCreateTableQuery(ex); } /** @@ -117,13 +153,15 @@ private static boolean isPlainCreateTableQuery(final Exception ex) { } /** - * Returns the hint to print for the given exception, if its message identifies a mistake we can - * advise on. + * Returns the hint to print for the given exception, if the message and the options it was given + * identify a mistake we can advise on. * * @param ex the exception thrown while converting + * @param parseResult the parsed command line the failure came from * @return the hint to print below the error message, or empty if the mistake is not recognized */ - private static Optional hint(final Exception ex) { + private static Optional hint( + final Exception ex, final CommandLine.ParseResult parseResult) { if (isPlainCreateTableQuery(ex)) { return Optional.of(QUERY_ARGUMENT_HINT); } @@ -135,31 +173,56 @@ private static Optional hint(final Exception ex) { if (message.contains(NOT_A_CREATE_TABLE) || message.contains(CTAS_NOT_SUPPORTED)) { return Optional.of(QUERY_ARGUMENT_HINT); } + if (message.contains(COLUMNS_REQUIRED)) { + return Optional.of(COLUMN_LIST_HINT); + } + + boolean expressions = parseResult.hasMatchedOption("-e"); + if (expressions && STATEMENT_AS_EXPRESSION.matcher(message).find()) { + return Optional.of(EXPRESSION_ARGUMENT_HINT); + } Matcher objectNotFound = OBJECT_NOT_FOUND.matcher(message); if (objectNotFound.find()) { - return Optional.of(CREATE_HINT.formatted(objectNotFound.group(1))); + return Optional.of( + withCasingNote(CREATE_HINT.formatted(objectName(objectNotFound)), parseResult)); } Matcher columnNotFound = COLUMN_NOT_FOUND.matcher(message); if (columnNotFound.find()) { - return Optional.of(COLUMN_HINT.formatted(columnNotFound.group(1))); + String hint = parseResult.hasMatchedOption("-c") ? COLUMN_HINT : COLUMN_WITHOUT_CREATE_HINT; + return Optional.of(withCasingNote(hint.formatted(columnNotFound.group(1)), parseResult)); } Matcher unknownIdentifier = UNKNOWN_IDENTIFIER.matcher(message); - if (unknownIdentifier.find()) { + if (expressions && unknownIdentifier.find()) { + // Without -e this is a complaint about a -c statement, where naming the identifier as a + // column of a new table would be the wrong advice. return Optional.of(EXPRESSION_HINT.formatted(unknownIdentifier.group(1))); } return Optional.empty(); } /** - * Reports whether the full stack trace was asked for on the command line. + * Returns the name to suggest defining, qualified with the schema Calcite looked in when the + * message names one. + * + * @param objectNotFound the match of {@link #OBJECT_NOT_FOUND} against the error message + * @return the object name to name in the hint + */ + private static String objectName(final Matcher objectNotFound) { + String schema = objectNotFound.group(2); + return schema == null ? objectNotFound.group(1) : schema + "." + objectNotFound.group(1); + } + + /** + * Appends the note about identifier casing to the given hint, unless the casing was chosen on the + * command line and the note would be both wrong and beside the point. * - * @param cmd the command line that failed - * @return true if {@code --stacktrace} was given + * @param hint the hint to print below the error message + * @param parseResult the parsed command line the failure came from + * @return the hint, with the casing note where it applies */ - private static boolean stackTraceRequested(final CommandLine cmd) { - Object command = cmd.getCommand(); - return command instanceof IsthmusEntryPoint - && ((IsthmusEntryPoint) command).isStackTraceRequested(); + private static String withCasingNote( + final String hint, final CommandLine.ParseResult parseResult) { + return parseResult.hasMatchedOption("--unquotedcasing") ? hint : hint + CASING_NOTE; } } diff --git a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/RegisterAtRuntime.java b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/RegisterAtRuntime.java index 0b3ca1adb..2f1609548 100644 --- a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/RegisterAtRuntime.java +++ b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/RegisterAtRuntime.java @@ -36,6 +36,7 @@ import org.apache.calcite.runtime.CalciteContextException; import org.apache.calcite.runtime.Resources; import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.apache.calcite.sql.parser.ddl.SqlDdlParserImpl; import org.apache.calcite.sql.validate.SqlValidatorException; import org.apache.calcite.sql2rel.StandardConvertletTable; import org.apache.calcite.util.BuiltInMethod; @@ -112,6 +113,14 @@ public void beforeAnalysis(BeforeAnalysisAccess access) { register(Resources.class, SqlValidatorException.class); + // Calcite reports a syntax error by collecting the tokens the grammar expected at that + // point, which it does by reflectively calling these productions on the parser + // (SqlAbstractParserImpl.MetadataImpl.initList). Without them the parser cannot build a + // SqlParseException at all, and every syntax error surfaces as the reflection failure + // instead of as a message about the SQL. + registerMethods( + SqlDdlParserImpl.class, "ReservedFunctionName", "ContextVariable", "NonReservedKeyWord"); + for (BuiltInMethod method : BuiltInMethod.values()) { if (method.field != null) { RuntimeReflection.register(method.field); @@ -128,6 +137,22 @@ public void beforeAnalysis(BeforeAnalysisAccess access) { } } + /** + * Registers the named no-argument methods of the given class for reflective lookup. + * + * @param c the class declaring the methods + * @param methodNames the names of the public no-argument methods to register + * @throws NoSuchMethodException if the class does not declare one of the methods, so that the + * native image fails to build rather than silently losing the behavior that needs them + */ + private static void registerMethods(Class c, String... methodNames) + throws NoSuchMethodException { + RuntimeReflection.register(c); + for (String methodName : methodNames) { + RuntimeReflection.register(c.getMethod(methodName)); + } + } + private static void register(Class... classes) { for (Class c : classes) { RuntimeReflection.register(c); diff --git a/isthmus-cli/src/test/java/io/substrait/isthmus/cli/IsthmusEntryPointTest.java b/isthmus-cli/src/test/java/io/substrait/isthmus/cli/IsthmusEntryPointTest.java index b4dc5be5e..31fab1f41 100644 --- a/isthmus-cli/src/test/java/io/substrait/isthmus/cli/IsthmusEntryPointTest.java +++ b/isthmus-cli/src/test/java/io/substrait/isthmus/cli/IsthmusEntryPointTest.java @@ -26,6 +26,11 @@ void assertNoStackTrace() { assertFalse(err.contains("\tat "), () -> "unexpected stack trace in:\n" + err); } + /** Asserts that the error was reported with its stack trace. */ + void assertStackTrace() { + assertTrue(err.contains("\tat "), () -> "expected a stack trace in:\n" + err); + } + /** * Asserts that stderr contains the given snippets. * @@ -36,6 +41,17 @@ void assertErrContains(String... snippets) { assertTrue(err.contains(snippet), () -> "expected '" + snippet + "' in:\n" + err); } } + + /** + * Asserts that stderr contains none of the given snippets. + * + * @param snippets the snippets stderr must not contain + */ + void assertErrDoesNotContain(String... snippets) { + for (String snippet : snippets) { + assertFalse(err.contains(snippet), () -> "unexpected '" + snippet + "' in:\n" + err); + } + } } /** @@ -72,6 +88,25 @@ void undefinedTableSuggestsCreateOption() { run.assertNoStackTrace(); } + @Test + void undefinedTableInSchemaIsSuggestedQualified() { + Run run = run("SELECT * FROM s.u", "-c", "CREATE TABLE s.t(a INT)"); + + assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); + run.assertErrContains("Object 'U' not found within 'S'", "CREATE TABLE S.U"); + run.assertNoStackTrace(); + } + + @Test + void chosenCasingIsNotSecondGuessed() { + Run run = run("SELECT * FROM foo", "--unquotedcasing", "UNCHANGED"); + + assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); + run.assertErrContains("Object 'foo' not found", "CREATE TABLE foo"); + run.assertErrDoesNotContain("upper-cased"); + run.assertNoStackTrace(); + } + @Test void undefinedColumnIsExplained() { Run run = run("SELECT bar FROM foo", "-c", "CREATE TABLE foo(id INT)"); @@ -81,6 +116,15 @@ void undefinedColumnIsExplained() { run.assertNoStackTrace(); } + @Test + void undefinedColumnWithoutCreatesSuggestsCreateOption() { + Run run = run("SELECT bar"); + + assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); + run.assertErrContains("Column 'BAR' not found", "no table was defined", "CREATE TABLE T (BAR"); + run.assertNoStackTrace(); + } + @Test void unknownIdentifierInExpressionSuggestsCreateOption() { Run run = run("-e", "col + 1"); @@ -90,6 +134,16 @@ void unknownIdentifierInExpressionSuggestsCreateOption() { run.assertNoStackTrace(); } + @Test + void unknownIdentifierInCreateIsNotBlamedOnExpressions() { + Run run = run("SELECT 1", "-c", "CREATE TABLE foo(a NOSUCHTYPE)"); + + assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); + run.assertErrContains("Unknown identifier 'NOSUCHTYPE'"); + run.assertErrDoesNotContain("-e / --expression"); + run.assertNoStackTrace(); + } + @Test void malformedQueryReportsTheParseError() { Run run = run("SELECT 1 FROM"); @@ -128,6 +182,42 @@ void ctasPassedToCreateOptionSuggestsQueryArgument() { run.assertNoStackTrace(); } + @Test + void createTableWithoutColumnsSuggestsAColumnList() { + Run run = run("SELECT * FROM foo", "-c", "CREATE TABLE foo"); + + assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); + run.assertErrContains("Column definitions are required.", "needs the columns of the table"); + run.assertNoStackTrace(); + } + + @Test + void createTableWithoutColumnsSuggestsAColumnListForExpressions() { + Run run = run("-e", "1", "-c", "CREATE TABLE foo"); + + assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); + run.assertErrContains("Column definitions are required.", "needs the columns of the table"); + run.assertNoStackTrace(); + } + + @Test + void queryAfterExpressionOptionIsExplained() { + Run run = run("-e", "1 + 1", "SELECT 1"); + + assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); + run.assertErrContains("-e / --expression consumes every following argument"); + run.assertNoStackTrace(); + } + + @Test + void queryAndExpressionsTogetherReportsUsageError() { + Run run = run("SELECT 1", "-e", "1 + 1"); + + assertEquals(CommandLine.ExitCode.USAGE, run.statusCode); + run.assertErrContains("not both", "the query 'SELECT 1' would be ignored", "Usage: isthmus"); + run.assertNoStackTrace(); + } + @Test void missingSqlReportsUsageError() { Run run = run("-c", "CREATE TABLE foo(id INT)"); @@ -137,12 +227,31 @@ void missingSqlReportsUsageError() { run.assertNoStackTrace(); } + @Test + void noArgumentsReportsUsageError() { + Run run = run(); + + assertEquals(CommandLine.ExitCode.USAGE, run.statusCode); + run.assertErrContains("Missing SQL to convert", "Usage: isthmus"); + run.assertNoStackTrace(); + } + + @Test + void failureWithoutAPositionKeepsTheStackTrace() { + // A parse failure that Calcite reports without a position did not come from the grammar, so it + // is a defect rather than a problem with the SQL and must stay reported as one. + Run run = run(""); + + assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); + run.assertStackTrace(); + } + @Test void stackTraceOptionKeepsTheStackTrace() { Run run = run("SELECT * FROM foo", "--stacktrace"); assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); - run.assertErrContains( - "Object 'FOO' not found", "-c / --create", "org.apache.calcite.runtime", "\tat "); + run.assertErrContains("Object 'FOO' not found", "-c / --create", "org.apache.calcite.runtime"); + run.assertStackTrace(); } } diff --git a/isthmus-cli/src/test/script/smoke.sh b/isthmus-cli/src/test/script/smoke.sh index bc0c647cc..592d760c2 100755 --- a/isthmus-cli/src/test/script/smoke.sh +++ b/isthmus-cli/src/test/script/smoke.sh @@ -47,11 +47,43 @@ if error=$("${CMD}" 'select * from lineitem' 2>&1); then exit 1 fi echo "${error}" -if ! grep -q -- '--create' <<<"${error}"; then +# Match the hint itself rather than the option name, which also appears in the usage block +if ! grep -q 'table definitions are not part of the query' <<<"${error}"; then echo "expected a hint about --create" exit 1 fi -if grep -q 'CalciteContextException' <<<"${error}"; then +if grep -q $'\tat ' <<<"${error}"; then echo "expected a message instead of a stack trace" exit 1 fi + +# Error path - a syntax error must be reported at its position. Calcite collects the tokens the +# grammar expected there by reflecting on the parser, which only works in the native image if those +# productions were registered, so this is not covered by the JVM tests. +if syntax=$("${CMD}" 'select 1 from' 2>&1); then + echo "expected a malformed query to fail" + exit 1 +fi +echo "${syntax}" +if ! grep -q 'Encountered "" at line 1, column 13' <<<"${syntax}"; then + echo "expected the parse error to name the position" + exit 1 +fi +if grep -q $'\tat ' <<<"${syntax}"; then + echo "expected a message instead of a stack trace" + exit 1 +fi + +# Error path - --stacktrace keeps the trace as well as the message +if trace=$("${CMD}" --stacktrace 'select * from lineitem' 2>&1); then + echo "expected a query without a table definition to fail" + exit 1 +fi +if ! grep -q 'table definitions are not part of the query' <<<"${trace}"; then + echo "expected --stacktrace to print the hint as well" + exit 1 +fi +if ! grep -q $'\tat ' <<<"${trace}"; then + echo "expected --stacktrace to print the stack trace" + exit 1 +fi diff --git a/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitCreateStatementParser.java b/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitCreateStatementParser.java index 0fe44388c..9206cc6a7 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitCreateStatementParser.java +++ b/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitCreateStatementParser.java @@ -84,9 +84,7 @@ public static List processCreateStatements( throw fail("Only simple table names are allowed.", create.name.getParserPosition()); } - if (create.query != null) { - throw fail("CTAS not supported.", create.name.getParserPosition()); - } + validateCreateTable(create); tableList.add( createSubstraitTable( @@ -196,6 +194,26 @@ private static SqlParseException fail(@Nullable final String message) { return fail(message, SqlParserPos.ZERO); } + /** + * Rejects the CREATE TABLE statements that carry no table schema to build a {@link + * SubstraitTable} from. Calcite's DDL grammar makes both the column list and the {@code AS query} + * optional and independent of each other, so a statement can arrive with either part missing. + * + * @param create the parsed CREATE TABLE statement; must not be null + * @throws SqlParseException if the statement defines its columns by a query, or does not define + * them at all + */ + private static void validateCreateTable(@NonNull final SqlCreateTable create) + throws SqlParseException { + if (create.query != null) { + throw fail("CTAS not supported.", create.name.getParserPosition()); + } + + if (create.columnList == null) { + throw fail("Column definitions are required.", create.name.getParserPosition()); + } + } + /** * Parses one or more SQL strings containing only CREATE statements into a {@link CalciteSchema} * using the given provider's parser config. @@ -216,9 +234,7 @@ private static CalciteSchema processCreateStatementsToSchema( final SqlCreateTable create = (SqlCreateTable) parsed; - if (create.query != null) { - throw fail("CTAS not supported.", create.name.getParserPosition()); - } + validateCreateTable(create); final List names = create.name.names; diff --git a/isthmus/src/test/java/io/substrait/isthmus/sql/SubstraitCreateStatementParserTest.java b/isthmus/src/test/java/io/substrait/isthmus/sql/SubstraitCreateStatementParserTest.java index 37ebe3c59..c6caf6977 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/sql/SubstraitCreateStatementParserTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/sql/SubstraitCreateStatementParserTest.java @@ -122,10 +122,47 @@ void testToCatalogWithMultipleCreateTableForSameTableThrowsException() throws Sq @Test void testToCatalogWithCreateTableAsSelectThrowsException() { - assertThrows( - SqlParseException.class, - () -> - SubstraitCreateStatementParser.processCreateStatementsToCatalog( - "create table src1 as select 1")); + final SqlParseException e = + assertThrows( + SqlParseException.class, + () -> + SubstraitCreateStatementParser.processCreateStatementsToCatalog( + "create table src1 as select 1")); + + assertEquals("CTAS not supported.", e.getMessage()); + } + + @Test + void testToCatalogWithCreateTableAsSelectWithColumnListThrowsException() { + final SqlParseException e = + assertThrows( + SqlParseException.class, + () -> + SubstraitCreateStatementParser.processCreateStatementsToCatalog( + "create table src1 (intcol int) as select 1")); + + assertEquals("CTAS not supported.", e.getMessage()); + } + + @Test + void testToCatalogWithCreateTableWithoutColumnsThrowsException() { + final SqlParseException e = + assertThrows( + SqlParseException.class, + () -> + SubstraitCreateStatementParser.processCreateStatementsToCatalog( + "create table src1")); + + assertEquals("Column definitions are required.", e.getMessage()); + } + + @Test + void testCreateStatementsWithCreateTableWithoutColumnsThrowsException() { + final SqlParseException e = + assertThrows( + SqlParseException.class, + () -> SubstraitCreateStatementParser.processCreateStatements("create table src1")); + + assertEquals("Column definitions are required.", e.getMessage()); } } From c35c7ede66c58c1e31f25a787bad2b783177883e Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Tue, 11 Aug 2026 10:42:04 +0200 Subject: [PATCH 3/3] fix(isthmus-cli): note the casing on the expression hint too The -e / --expression hint was the one identifier hint left without the note, and it is where the upper-casing is least obvious: the hint quotes the identifier back with a casing the user never typed. --- .../isthmus/cli/IsthmusExecutionExceptionHandler.java | 3 ++- .../java/io/substrait/isthmus/cli/IsthmusEntryPointTest.java | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusExecutionExceptionHandler.java b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusExecutionExceptionHandler.java index db77db5ec..0f717a1ef 100644 --- a/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusExecutionExceptionHandler.java +++ b/isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusExecutionExceptionHandler.java @@ -196,7 +196,8 @@ private static Optional hint( if (expressions && unknownIdentifier.find()) { // Without -e this is a complaint about a -c statement, where naming the identifier as a // column of a new table would be the wrong advice. - return Optional.of(EXPRESSION_HINT.formatted(unknownIdentifier.group(1))); + return Optional.of( + withCasingNote(EXPRESSION_HINT.formatted(unknownIdentifier.group(1)), parseResult)); } return Optional.empty(); } diff --git a/isthmus-cli/src/test/java/io/substrait/isthmus/cli/IsthmusEntryPointTest.java b/isthmus-cli/src/test/java/io/substrait/isthmus/cli/IsthmusEntryPointTest.java index 31fab1f41..d27849b14 100644 --- a/isthmus-cli/src/test/java/io/substrait/isthmus/cli/IsthmusEntryPointTest.java +++ b/isthmus-cli/src/test/java/io/substrait/isthmus/cli/IsthmusEntryPointTest.java @@ -130,7 +130,8 @@ void unknownIdentifierInExpressionSuggestsCreateOption() { Run run = run("-e", "col + 1"); assertEquals(CommandLine.ExitCode.SOFTWARE, run.statusCode); - run.assertErrContains("Unknown identifier 'COL'", "-e / --expression", "-c / --create"); + run.assertErrContains( + "Unknown identifier 'COL'", "-e / --expression", "-c / --create", "--unquotedcasing"); run.assertNoStackTrace(); }