This is an automated email from the ASF dual-hosted git repository. MartijnVisser pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/flink.git
commit e64938456a590f47ad0e09aaefd1addbbbfd8755 Author: Martijn Visser <[email protected]> AuthorDate: Wed Sep 30 20:48:27 2026 +0200 [FLINK-24908][table-planner] Show the position and the failing line in parse errors Errors that the grammar raises through SqlUtil.newContextException, such as "Non-query expression encountered in illegal context" for "ELECT 1", carried no line or column because CalciteParser never read SqlParseException#getPos(). Every parse error now echoes the failing line with carets under the span, after the Calcite headline and before JavaCC's token list; errors whose text does not name the position get it appended to the reason. Long lines are cut and sensitive option values are masked. Generated-by: Claude Code (Claude Fable 5.1) --- .../service/application/ScriptExecutorITCase.java | 4 +- .../table/planner/delegation/PlannerContext.java | 11 +- .../flink/table/planner/parse/CalciteParser.java | 81 +++- .../table/planner/parse/ParseErrorSnippet.java | 307 +++++++++++++++ .../table/planner/calcite/FlinkPlannerImpl.scala | 2 +- .../table/planner/delegation/ParserImplTest.java | 31 +- .../table/planner/parse/CalciteParserTest.java | 423 +++++++++++++++++++++ .../table/planner/parse/ParseErrorSnippetTest.java | 177 +++++++++ 8 files changed, 1022 insertions(+), 14 deletions(-) diff --git a/flink-table/flink-sql-gateway/src/test/java/org/apache/flink/table/gateway/service/application/ScriptExecutorITCase.java b/flink-table/flink-sql-gateway/src/test/java/org/apache/flink/table/gateway/service/application/ScriptExecutorITCase.java index 906c9799e1d..920729c25dd 100644 --- a/flink-table/flink-sql-gateway/src/test/java/org/apache/flink/table/gateway/service/application/ScriptExecutorITCase.java +++ b/flink-table/flink-sql-gateway/src/test/java/org/apache/flink/table/gateway/service/application/ScriptExecutorITCase.java @@ -151,7 +151,9 @@ public class ScriptExecutorITCase extends AbstractSqlGatewayStatementITCaseBase void testParseErrorPositionIsCorrect() throws Exception { assertThat(runScript("error.q")) .contains( - "org.apache.flink.table.api.SqlParserException: SQL parse failed. Encountered \")\" at line 26, column 1."); + "org.apache.flink.table.api.SqlParserException: SQL parse failed. Encountered \")\" at line 26, column 1:\n" + + " ) WITH (\n" + + " ^\n"); } @Override diff --git a/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/delegation/PlannerContext.java b/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/delegation/PlannerContext.java index e0f7fcade90..68f774acc9c 100644 --- a/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/delegation/PlannerContext.java +++ b/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/delegation/PlannerContext.java @@ -19,6 +19,7 @@ package org.apache.flink.table.planner.delegation; import org.apache.flink.annotation.Internal; +import org.apache.flink.configuration.SecurityOptions; import org.apache.flink.sql.parser.validate.FlinkSqlConformance; import org.apache.flink.table.api.SqlDialect; import org.apache.flink.table.api.TableConfig; @@ -172,11 +173,17 @@ public class PlannerContext { public FlinkPlannerImpl createFlinkPlanner() { return new FlinkPlannerImpl( - createFrameworkConfig(), this::createCatalogReader, typeFactory, cluster); + createFrameworkConfig(), + createCalciteParser(), + this::createCatalogReader, + typeFactory, + cluster); } public CalciteParser createCalciteParser() { - return new CalciteParser(getSqlParserConfig()); + return new CalciteParser( + getSqlParserConfig(), + context.getTableConfig().get(SecurityOptions.ADDITIONAL_SENSITIVE_KEYS)); } public FlinkCalciteCatalogReader createCatalogReader(boolean lenientCaseSensitivity) { diff --git a/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/parse/CalciteParser.java b/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/parse/CalciteParser.java index 67b88a00a6d..01955b932f3 100644 --- a/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/parse/CalciteParser.java +++ b/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/parse/CalciteParser.java @@ -28,18 +28,26 @@ import org.apache.calcite.sql.SqlNodeList; import org.apache.calcite.sql.parser.SqlAbstractParserImpl; import org.apache.calcite.sql.parser.SqlParseException; import org.apache.calcite.sql.parser.SqlParser; +import org.apache.calcite.sql.parser.SqlParserPos; import org.apache.calcite.util.SourceStringReader; import java.io.Reader; +import java.util.List; +import java.util.Optional; +import java.util.regex.Pattern; + +import static org.apache.calcite.util.Static.RESOURCE; /** * Thin wrapper around {@link SqlParser} that does exception conversion and {@link SqlNode} casting. */ public class CalciteParser { private final SqlParser.Config config; + private final List<String> additionalSensitiveKeys; - public CalciteParser(SqlParser.Config config) { + public CalciteParser(SqlParser.Config config, List<String> additionalSensitiveKeys) { this.config = config; + this.additionalSensitiveKeys = additionalSensitiveKeys; } /** @@ -55,7 +63,7 @@ public class CalciteParser { SqlParser parser = SqlParser.create(sql, config); return parser.parseStmt(); } catch (SqlParseException e) { - throw toStatementException(e); + throw toStatementException(sql, e); } } @@ -72,7 +80,7 @@ public class CalciteParser { SqlParser parser = SqlParser.create(sql, config); return parser.parseStmtList(); } catch (SqlParseException e) { - throw toStatementException(e); + throw toStatementException(sql, e); } } @@ -88,7 +96,7 @@ public class CalciteParser { final SqlParser parser = SqlParser.create(sqlExpression, config); return parser.parseExpression(); } catch (SqlParseException e) { - throw toParserException(e); + throw toParserException(sqlExpression, e); } } @@ -118,18 +126,73 @@ public class CalciteParser { * Converts a parse error of a statement. An error at the end of the input means the statement * is incomplete, which callers such as the SQL client use to keep reading. */ - private static SqlParserException toStatementException(SqlParseException e) { + private SqlParserException toStatementException(String sql, SqlParseException e) { final String message = e.getMessage(); if (message != null && message.contains("Encountered \"<EOF>\"")) { return new SqlParserEOFException(message, e); } - return toParserException(e); + return toParserException(sql, e); } - private static SqlParserException toParserException(SqlParseException e) { - final String message = e.getMessage(); + private SqlParserException toParserException(String sql, SqlParseException e) { + final String message = describe(sql, e); return new SqlParserException( - message == null ? "SQL parse failed." : "SQL parse failed. " + message, e); + message.isEmpty() ? "SQL parse failed." : "SQL parse failed. " + message, e); + } + + /** + * Returns the Calcite message with the line the parser stopped at echoed after its headline, so + * the reason stays at the front and JavaCC's token list at the end. Errors raised through + * {@code SqlUtil.newContextException} keep their position only in {@link + * SqlParseException#getPos()}, and {@link SqlParserException} exposes none, so the position is + * appended to the reason in the wording validation errors use. + */ + private String describe(String sql, SqlParseException e) { + final String message = e.getMessage() == null ? "" : e.getMessage(); + final SqlParserPos pos = e.getPos(); + if (pos == null || pos.getLineNum() <= 0 || message.isEmpty()) { + return message; + } + final int lineBreak = message.indexOf('\n'); + final String headline = lineBreak < 0 ? message : message.substring(0, lineBreak); + final String rest = lineBreak < 0 ? "" : message.substring(lineBreak); + final boolean positioned = + Pattern.compile( + "\\bline " + + pos.getLineNum() + + ", column " + + pos.getColumnNum() + + "\\b") + .matcher(headline) + .find(); + final Optional<String> snippet = + ParseErrorSnippet.render(sql, pos, additionalSensitiveKeys); + if (positioned) { + return snippet.map( + s -> + (headline.endsWith(".") + ? headline.substring(0, headline.length() - 1) + : headline) + + ":\n" + + s + + rest) + .orElse(message); + } + final boolean point = + pos.getLineNum() == pos.getEndLineNum() + && pos.getColumnNum() == pos.getEndColumnNum(); + final String context = + point + ? RESOURCE.validatorContextPoint(pos.getLineNum(), pos.getColumnNum()).str() + : RESOURCE.validatorContext( + pos.getLineNum(), + pos.getColumnNum(), + pos.getEndLineNum(), + pos.getEndColumnNum()) + .str(); + final String reason = message.endsWith(".") ? message : message + "."; + return snippet.map(s -> reason + " " + context + ":\n" + s) + .orElse(reason + " " + context + "."); } /** diff --git a/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/parse/ParseErrorSnippet.java b/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/parse/ParseErrorSnippet.java new file mode 100644 index 00000000000..16385b4a1b1 --- /dev/null +++ b/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/parse/ParseErrorSnippet.java @@ -0,0 +1,307 @@ +/* + * 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.planner.parse; + +import org.apache.flink.configuration.GlobalConfiguration; + +import org.apache.calcite.sql.parser.SqlParserPos; + +import java.util.List; +import java.util.Optional; + +/** + * Renders the line of a SQL text that a parser position points at, with carets under the span, the + * way psql shows a syntax error. + * + * <p>A long line is cut to a window around the span that fits an 80-column terminal. Sensitive + * option values on the line are masked and keep their length, so the columns stay valid; a value on + * the next line, or a credential inside another value such as a URL, is not. A tab becomes one + * space because the parser counts it as one column. Widths are display cells, so East Asian + * characters count two. + */ +final class ParseErrorSnippet { + + private static final int WIDTH = 76; + private static final int RIGHT_CONTEXT = 10; + private static final String INDENT = " "; + private static final String CUT = "..."; + private static final String SEPARATOR_CHARS = "=:>"; + private static final String QUOTES = "'\"`"; + + private ParseErrorSnippet() {} + + /** + * Returns the indented line and the caret line under it, or nothing when the position is + * unknown or lies beyond the text. Values of options whose key is sensitive, built in or + * through {@code additionalSensitiveKeys}, are masked. + */ + static Optional<String> render( + String sql, SqlParserPos pos, List<String> additionalSensitiveKeys) { + if (pos.getLineNum() <= 0) { + return Optional.empty(); + } + final int lineStart = lineStart(sql, pos.getLineNum()); + if (lineStart < 0) { + return Optional.empty(); + } + String line = + mask(sql.substring(lineStart, lineEnd(sql, lineStart)), additionalSensitiveKeys) + .replace('\t', ' '); + final int caret = Math.min(Math.max(pos.getColumnNum() - 1, 0), line.length()); + // Trailing whitespace goes, except where the position points into it. + line = line.substring(0, Math.max(line.stripTrailing().length(), caret)); + final int spanEnd = + pos.getEndLineNum() == pos.getLineNum() + ? Math.min( + line.length(), + caret + pos.getEndColumnNum() - pos.getColumnNum() + 1) + : line.length(); + + // The window is measured in cells; the caret may sit one cell past the end of the line. + final int[] cells = cellOffsets(line); + final int extent = Math.max(cells[line.length()], cells[caret] + 1); + int fromCell = 0; + int toCell = extent; + String head = ""; + String tail = ""; + if (extent > WIDTH) { + final int needed = Math.min(extent, cells[caret] + RIGHT_CONTEXT + 1); + if (needed <= WIDTH - CUT.length()) { + toCell = WIDTH - CUT.length(); + tail = CUT; + } else if (needed == extent) { + fromCell = extent - (WIDTH - CUT.length()); + head = CUT; + } else { + toCell = needed; + fromCell = toCell - (WIDTH - 2 * CUT.length()); + head = CUT; + tail = CUT; + } + } + final int from = firstBoundaryAtOrAfter(line, cells, fromCell); + final int to = lastBoundaryAtOrBefore(line, cells, toCell); + final int caretEnd = Math.min(Math.max(spanEnd, caret + 1), Math.max(to, caret)); + return Optional.of( + INDENT + + head + + line.substring(from, to) + + tail + + "\n" + + INDENT + + " ".repeat(head.length() + cells[caret] - cells[from]) + + "^".repeat(Math.max(cells[caretEnd] - cells[caret], 1))); + } + + /** + * The index where the given line starts, or -1 when the text has fewer lines. A lone CR is a + * line break for the lexer as well, which Calcite's {@code SqlParserUtil.nextLine} gets wrong. + */ + private static int lineStart(String sql, int line) { + int start = 0; + for (int i = 1; i < line; i++) { + final int end = lineEnd(sql, start); + if (end >= sql.length()) { + return -1; + } + start = end + (sql.startsWith("\r\n", end) ? 2 : 1); + } + return start; + } + + private static int lineEnd(String sql, int start) { + int end = start; + while (end < sql.length() && sql.charAt(end) != '\n' && sql.charAt(end) != '\r') { + end++; + } + return end; + } + + /** Display cells before each character index, so {@code cells[line.length()]} is the width. */ + private static int[] cellOffsets(String line) { + final int[] cells = new int[line.length() + 1]; + int width = 0; + for (int i = 0; i < line.length(); ) { + final int cp = line.codePointAt(i); + final int count = Character.charCount(cp); + for (int j = 0; j < count; j++) { + cells[i + j] = width; + } + width += cellsOf(cp); + i += count; + } + cells[line.length()] = width; + return cells; + } + + private static int firstBoundaryAtOrAfter(String line, int[] cells, int cell) { + int i = 0; + while (i < line.length() && (cells[i] < cell || !isBoundary(line, i))) { + i++; + } + return i; + } + + private static int lastBoundaryAtOrBefore(String line, int[] cells, int cell) { + int i = line.length(); + while (i > 0 && (cells[i] > cell || !isBoundary(line, i))) { + i--; + } + return i; + } + + /** A cut may not split a surrogate pair or separate a mark from its base. */ + private static boolean isBoundary(String line, int i) { + if (i == 0 || i == line.length()) { + return true; + } + return !Character.isLowSurrogate(line.charAt(i)) && cellsOf(line.codePointAt(i)) > 0; + } + + /** + * Masks the value of every {@code key = value} pair whose key is sensitive. A key is a string + * in single, double or back quotes, or a bare token that starts with a letter or digit and + * contains a dot or a dash, which an unquoted SQL identifier cannot; the separator is any run + * of {@code =}, {@code :} or {@code >}, or nothing at all; the value is a quoted string, + * unterminated included, or after a separator a bare token. + */ + private static String mask(String line, List<String> additionalSensitiveKeys) { + final StringBuilder masked = new StringBuilder(line); + final int length = line.length(); + int i = 0; + while (i < length) { + final String key; + final int keyEnd; + if (QUOTES.indexOf(line.charAt(i)) >= 0) { + final int end = quotedEnd(line, i); + key = line.substring(i + 1, end < 0 ? length : end - 1); + keyEnd = end < 0 ? length : end; + } else if (Character.isLetterOrDigit(line.charAt(i))) { + int end = i; + while (end < length && isBareKeyChar(line.charAt(end))) { + end++; + } + key = line.substring(i, end); + keyEnd = end; + if (key.indexOf('.') < 0 && key.indexOf('-') < 0) { + i = end; + continue; + } + } else { + i++; + continue; + } + + int j = skipSpaces(line, keyEnd); + final int separatorStart = j; + while (j < length && SEPARATOR_CHARS.indexOf(line.charAt(j)) >= 0) { + j++; + } + final boolean separated = j > separatorStart; + j = skipSpaces(line, j); + + final int valueStart; + final int valueEnd; + final int next; + if (j < length && QUOTES.indexOf(line.charAt(j)) >= 0) { + final int end = quotedEnd(line, j); + valueStart = j + 1; + valueEnd = end < 0 ? length : end - 1; + next = end < 0 ? length : end; + } else if (separated && j < length && isBareValueChar(line.charAt(j))) { + valueStart = j; + int end = j; + while (end < length && isBareValueChar(line.charAt(end))) { + end++; + } + valueEnd = end; + next = end; + } else { + i = keyEnd; + continue; + } + if (GlobalConfiguration.isSensitive(key, additionalSensitiveKeys)) { + for (int k = valueStart; k < valueEnd; k++) { + masked.setCharAt(k, '*'); + } + } + i = next; + } + return masked.toString(); + } + + /** The index after the closing quote of the string starting at {@code start}, or -1. */ + private static int quotedEnd(String line, int start) { + final char quote = line.charAt(start); + int i = start + 1; + while (i < line.length()) { + if (line.charAt(i) == quote) { + if (i + 1 < line.length() && line.charAt(i + 1) == quote) { + i += 2; + continue; + } + return i + 1; + } + i++; + } + return -1; + } + + private static int skipSpaces(String line, int i) { + while (i < line.length() && Character.isWhitespace(line.charAt(i))) { + i++; + } + return i; + } + + private static boolean isBareKeyChar(char c) { + return Character.isLetterOrDigit(c) || c == '_' || c == '.' || c == '-'; + } + + private static boolean isBareValueChar(char c) { + return !Character.isWhitespace(c) && ",;()".indexOf(c) < 0 && QUOTES.indexOf(c) < 0; + } + + /** Display cells of a code point: marks and format characters none, East Asian wide two. */ + private static int cellsOf(int cp) { + final int type = Character.getType(cp); + if (type == Character.NON_SPACING_MARK + || type == Character.ENCLOSING_MARK + || type == Character.FORMAT) { + return 0; + } + return isWide(cp) ? 2 : 1; + } + + private static boolean isWide(int cp) { + return (cp >= 0x1100 && cp <= 0x115F) + || (cp >= 0x2E80 && cp <= 0x303E) + || (cp >= 0x3041 && cp <= 0x33FF) + || (cp >= 0x3400 && cp <= 0x4DBF) + || (cp >= 0x4E00 && cp <= 0x9FFF) + || (cp >= 0xA000 && cp <= 0xA4CF) + || (cp >= 0xAC00 && cp <= 0xD7A3) + || (cp >= 0xF900 && cp <= 0xFAFF) + || (cp >= 0xFE30 && cp <= 0xFE4F) + || (cp >= 0xFF00 && cp <= 0xFF60) + || (cp >= 0xFFE0 && cp <= 0xFFE6) + || (cp >= 0x1F300 && cp <= 0x1FAFF) + || (cp >= 0x20000 && cp <= 0x3FFFD); + } +} diff --git a/flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/calcite/FlinkPlannerImpl.scala b/flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/calcite/FlinkPlannerImpl.scala index 846402a940e..52ec52abbca 100644 --- a/flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/calcite/FlinkPlannerImpl.scala +++ b/flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/calcite/FlinkPlannerImpl.scala @@ -59,12 +59,12 @@ import scala.collection.JavaConverters._ */ class FlinkPlannerImpl( val config: FrameworkConfig, + val parser: CalciteParser, catalogReaderSupplier: JFunction[JBoolean, CalciteCatalogReader], typeFactory: FlinkTypeFactory, val cluster: RelOptCluster) { val operatorTable: SqlOperatorTable = config.getOperatorTable - val parser: CalciteParser = new CalciteParser(config.getParserConfig) val convertletTable: SqlRexConvertletTable = config.getConvertletTable val sqlToRelConverterConfig: SqlToRelConverter.Config = config.getSqlToRelConverterConfig.withAddJsonTypeOperatorEnabled(false) diff --git a/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/delegation/ParserImplTest.java b/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/delegation/ParserImplTest.java index 94ca5f441f0..6827646b2b3 100644 --- a/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/delegation/ParserImplTest.java +++ b/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/delegation/ParserImplTest.java @@ -18,8 +18,10 @@ package org.apache.flink.table.planner.delegation; +import org.apache.flink.configuration.SecurityOptions; import org.apache.flink.table.api.SqlParserEOFException; import org.apache.flink.table.api.SqlParserException; +import org.apache.flink.table.api.TableConfig; import org.apache.flink.table.delegation.Parser; import org.apache.flink.table.operations.Operation; import org.apache.flink.table.planner.calcite.FlinkPlannerImpl; @@ -73,7 +75,15 @@ class ParserImplTest { forStatement("SET execution.runtime-type=") // TODO: the exception message should be "no value defined" .error( - "SQL parse failed. Encountered \"execution\" at line 1, column 5")); + "SQL parse failed. Encountered \"execution\" at line 1, column 5:\n" + + " SET execution.runtime-type=\n" + + " ^^^^^^^^^\n"), + forStatement("ELECT 1") + .error( + "SQL parse failed. Non-query expression encountered in illegal context." + + " From line 1, column 1 to line 1, column 5:\n" + + " ELECT 1\n" + + " ^^^^^")); @Test void testParseLegalStatements() { @@ -106,6 +116,25 @@ class ParserImplTest { .isInstanceOf(SqlParserEOFException.class); } + /** The statement parser comes from {@code PlannerContext}, which passes the configured keys. */ + @Test + void testStatementParserHonorsAdditionalSensitiveKeys() { + final TableConfig config = TableConfig.getDefault(); + config.set(SecurityOptions.ADDITIONAL_SENSITIVE_KEYS, List.of("hunter")); + final PlannerMocks mocks = PlannerMocks.newBuilder().withTableConfig(config).build(); + final Parser parser = + new DefaultParserFactory() + .create( + new DefaultCalciteContext( + mocks.getCatalogManager(), mocks.getPlannerContext())); + assertThatThrownBy( + () -> parser.parse("CREATE TABLE t (a INT) WITH ('my.hunter.opt' = 'x') 1")) + .isInstanceOf(SqlParserException.class) + .message() + .contains(" CREATE TABLE t (a INT) WITH ('my.hunter.opt' = '*') 1\n") + .doesNotContain("'x'"); + } + @Test void testCompletionTest() { verifySqlCompletion("QU", 1, new String[] {"QUIT"}); diff --git a/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/parse/CalciteParserTest.java b/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/parse/CalciteParserTest.java new file mode 100644 index 00000000000..8bc8f08105f --- /dev/null +++ b/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/parse/CalciteParserTest.java @@ -0,0 +1,423 @@ +/* + * 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.planner.parse; + +import org.apache.flink.configuration.SecurityOptions; +import org.apache.flink.sql.parser.impl.FlinkSqlParserImpl; +import org.apache.flink.sql.parser.validate.FlinkSqlConformance; +import org.apache.flink.table.api.SqlParserEOFException; +import org.apache.flink.table.api.SqlParserException; +import org.apache.flink.table.api.TableConfig; +import org.apache.flink.table.planner.calcite.FlinkPlannerImpl; +import org.apache.flink.table.planner.utils.PlannerMocks; + +import org.apache.calcite.config.Lex; +import org.apache.calcite.sql.parser.SqlParser; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.IntStream; +import java.util.stream.Stream; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** Tests for the exception conversion in {@link CalciteParser}. */ +class CalciteParserTest { + + private static final String NON_QUERY = "Non-query expression encountered in illegal context"; + + private static final SqlParser.Config CONFIG = + SqlParser.config() + .withParserFactory(FlinkSqlParserImpl.FACTORY) + .withConformance(FlinkSqlConformance.DEFAULT) + .withLex(Lex.JAVA) + .withIdentifierMaxLength(256); + + private final CalciteParser parser = new CalciteParser(CONFIG, List.of()); + + /** + * Errors that the grammar raises through {@code SqlUtil.newContextException}. Calcite keeps + * their position only in {@code SqlParseException#getPos()}, so the message has to name it and + * show the line. + */ + private static Stream<Arguments> statementsWithContextErrors() { + return Stream.of( + Arguments.of( + "misspelled keyword", + "ELECT 1", + NON_QUERY + + ". From line 1, column 1 to line 1, column 5:\n" + + " ELECT 1\n" + + " ^^^^^"), + Arguments.of( + "non-query INSERT body", + "INSERT INTO t 1", + NON_QUERY + + ". At line 1, column 15:\n" + + " INSERT INTO t 1\n" + + " ^"), + Arguments.of( + "non-query UNION operand", + "SELECT a,\n b\nFROM t\nUNION\n foo_bar_baz", + NON_QUERY + + ". From line 5, column 3 to line 5, column 13:\n" + + " foo_bar_baz\n" + + " ^^^^^^^^^^^"), + Arguments.of( + "leading block comment", + "/*\ncomment\n*/ ELECT 1", + NON_QUERY + + ". From line 3, column 5 to line 3, column 9:\n" + + " */ ELECT 1\n" + + " ^^^^^"), + Arguments.of( + "CRLF line breaks", + "SELECT 1\r\nFROM t\r\nUNION\r\nfoo_bar", + NON_QUERY + + ". From line 4, column 1 to line 4, column 7:\n" + + " foo_bar\n" + + " ^^^^^^^"), + Arguments.of( + "CRLF before the next line", + "SELECT 1\r\nUNION\r\nfoo_bar\r\nUNION\r\nSELECT 2", + NON_QUERY + + ". From line 3, column 1 to line 3, column 7:\n" + + " foo_bar\n" + + " ^^^^^^^"), + Arguments.of( + "CR line breaks", + "SELECT 1\rUNION\rfoo_bar", + NON_QUERY + + ". From line 3, column 1 to line 3, column 7:\n" + + " foo_bar\n" + + " ^^^^^^^"), + Arguments.of( + "tab counts as one column", + "SELECT\t1\tUNION\tfoo", + NON_QUERY + + ". From line 1, column 16 to line 1, column 18:\n" + + " SELECT 1 UNION foo\n" + + " ^^^"), + Arguments.of( + "expected query or join", + "SELECT * FROM (t)", + "Expected query or join. At line 1, column 16:\n" + + " SELECT * FROM (t)\n" + + " ^"), + Arguments.of( + "Flink grammar extension", + "CREATE SYSTEM CONNECTION c WITH ('a'='b')", + "CREATE SYSTEM CONNECTION is not" + + " supported, system connections can only be registered as" + + " temporary connections, you can use CREATE TEMPORARY SYSTEM" + + " CONNECTION instead." + + " From line 1, column 8 to line 1, column 13:\n" + + " CREATE SYSTEM CONNECTION c WITH ('a'='b')\n" + + " ^^^^^^")); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("statementsWithContextErrors") + void testStatementErrorShowsTheLine(String name, String sql, String expectedMessage) { + assertThatThrownBy(() -> parser.parse(sql)) + .isInstanceOf(SqlParserException.class) + .hasMessage("SQL parse failed. " + expectedMessage); + assertThatThrownBy(() -> parser.parseSqlList(sql)) + .isInstanceOf(SqlParserException.class) + .hasMessage("SQL parse failed. " + expectedMessage); + } + + @Test + void testStatementListErrorShowsTheLineOfTheScript() { + final String script = + IntStream.rangeClosed(1, 18) + .mapToObj(i -> "-- line " + i + "\n") + .collect(Collectors.joining()) + + "SELECT 1;\n SELCT 2;"; + assertThatThrownBy(() -> parser.parseSqlList(script)) + .isInstanceOf(SqlParserException.class) + .hasMessage( + "SQL parse failed. " + + NON_QUERY + + ". From line 20, column 3 to line 20, column 7:\n" + + " SELCT 2;\n" + + " ^^^^^"); + } + + @Test + void testExpressionErrorShowsTheLine() { + assertThatThrownBy(() -> parser.parseExpression("x IN (SELECT 1 UNION 2)")) + .isInstanceOf(SqlParserException.class) + .hasMessage( + "SQL parse failed. " + + NON_QUERY + + ". At line 1, column 22:\n" + + " x IN (SELECT 1 UNION 2)\n" + + " ^"); + } + + /** JavaCC names the position itself; the line goes before its token list. */ + @Test + void testSyntaxErrorShowsTheLineBeforeTheExpectedTokens() { + assertThatThrownBy(() -> parser.parse("SELECT 1; -- x")) + .isInstanceOf(SqlParserException.class) + .hasMessageStartingWith( + "SQL parse failed. Encountered \";\" at line 1, column 9:\n" + + " SELECT 1; -- x\n" + + " ^\n" + + "Was expecting one of:\n"); + } + + @Test + void testKeywordErrorShowsTheLine() { + assertThatThrownBy(() -> parser.parse("SELECT FROM t")) + .isInstanceOf(SqlParserException.class) + .hasMessageStartingWith( + "SQL parse failed. Incorrect syntax near the keyword 'FROM' at line 1, column 8:\n" + + " SELECT FROM t\n" + + " ^^^^\n" + + "Was expecting one of:\n"); + } + + /** The lexer stops after the last character here, so the caret sits behind the line. */ + @Test + void testLexicalErrorShowsTheLine() { + assertThatThrownBy(() -> parser.parse("SELECT 1 #")) + .isInstanceOf(SqlParserException.class) + .hasMessageStartingWith("SQL parse failed. Lexical error at line 1, column 11.") + .hasMessageEndingWith(":\n" + " SELECT 1 #\n" + " ^"); + } + + @Test + void testLongLineIsCutAfterTheError() { + final String sql = "ELECT 1 FROM " + "a".repeat(100); + assertThatThrownBy(() -> parser.parse(sql)) + .isInstanceOf(SqlParserException.class) + .hasMessage( + "SQL parse failed. " + + NON_QUERY + + ". From line 1, column 1 to line 1, column 5:\n" + + " ELECT 1 FROM " + + "a".repeat(60) + + "...\n" + + " ^^^^^"); + } + + @Test + void testLongLineIsCutBeforeTheError() { + final String sql = "SELECT " + "a".repeat(100) + " FROM t UNION foo_bar"; + assertThatThrownBy(() -> parser.parse(sql)) + .isInstanceOf(SqlParserException.class) + .hasMessage( + "SQL parse failed. " + + NON_QUERY + + ". From line 1, column 122 to line 1, column 128:\n" + + " ..." + + "a".repeat(52) + + " FROM t UNION foo_bar\n" + + " " + + " ".repeat(69) + + "^^^^^^^"); + } + + @Test + void testLongLineIsCutOnBothSidesOfTheError() { + final String sql = "SELECT " + "a".repeat(100) + " UNION foo_bar UNION " + "b".repeat(100); + assertThatThrownBy(() -> parser.parse(sql)) + .isInstanceOf(SqlParserException.class) + .hasMessage( + "SQL parse failed. " + + NON_QUERY + + ". From line 1, column 115 to line 1, column 121:\n" + + " ..." + + "a".repeat(52) + + " UNION foo_bar UNI...\n" + + " " + + " ".repeat(62) + + "^^^^^^^"); + } + + @Test + void testSensitiveOptionValuesAreMaskedInTheLine() { + assertThatThrownBy( + () -> + parser.parse( + "CREATE TABLE t (a INT) WITH ('password' = 'hunter2') 1")) + .isInstanceOf(SqlParserException.class) + .hasMessageStartingWith( + "SQL parse failed. Encountered \"1\" at line 1, column 54:\n" + + " CREATE TABLE t (a INT) WITH ('password' = '*******') 1\n" + + " ^\n") + .message() + .doesNotContain("hunter2"); + } + + @Test + void testSensitiveValueOfAnUnquotedOptionIsMasked() { + assertThatThrownBy(() -> parser.parse("SET s3.secret-key=abc")) + .isInstanceOf(SqlParserException.class) + .hasMessageStartingWith( + "SQL parse failed. Encountered \"s3\" at line 1, column 5:\n" + + " SET s3.secret-key=***\n" + + " ^^\n") + .message() + .doesNotContain("abc"); + } + + @Test + void testAdditionalSensitiveKeysAreMasked() { + final CalciteParser masking = new CalciteParser(CONFIG, List.of("hunter")); + assertThatThrownBy( + () -> + masking.parse( + "CREATE TABLE t (a INT) WITH ('my.hunter.opt' = 'x') 1")) + .isInstanceOf(SqlParserException.class) + .message() + .contains(" CREATE TABLE t (a INT) WITH ('my.hunter.opt' = '*') 1\n") + .doesNotContain("'x'"); + } + + /** The Calcite text stays in the message, only its headline gains the echoed line. */ + @Test + void testMessageKeepsTheParserText() { + for (String sql : new String[] {"ELECT 1", "SELECT 1; -- x", "SELECT 1 #"}) { + assertThatThrownBy(() -> parser.parse(sql)) + .isInstanceOf(SqlParserException.class) + .satisfies( + e -> { + final String[] cause = e.getCause().getMessage().split("\n", 2); + assertThat(e.getMessage()) + .contains(cause[0].replaceAll("\\.$", "")); + if (cause.length > 1) { + assertThat(e.getMessage()).endsWith(cause[1]); + } + }); + } + } + + @ParameterizedTest(name = "{0}") + @MethodSource("malformedOptionPairs") + void testSensitiveValueIsMaskedWhateverBrokeThePair(String name, String sql, String line) { + assertThatThrownBy(() -> parser.parse(sql)) + .isInstanceOf(SqlParserException.class) + .message() + .contains("\n " + line + "\n") + .doesNotContain("hunter2"); + } + + private static Stream<Arguments> malformedOptionPairs() { + return Stream.of( + Arguments.of( + "colon", + "CREATE TABLE t (a INT) WITH ('password': 'hunter2')", + "CREATE TABLE t (a INT) WITH ('password': '*******')"), + Arguments.of( + "arrow", + "CREATE TABLE t (a INT) WITH ('password' => 'hunter2')", + "CREATE TABLE t (a INT) WITH ('password' => '*******')"), + Arguments.of( + "missing equals", + "CREATE TABLE t (a INT) WITH ('password' 'hunter2')", + "CREATE TABLE t (a INT) WITH ('password' '*******')"), + Arguments.of( + "unterminated value", + "CREATE TABLE t (a INT) WITH ('password' = 'hunter2", + "CREATE TABLE t (a INT) WITH ('password' = '*******"), + Arguments.of( + "double-quoted key", + "CREATE TABLE t (a INT) WITH (\"password\" = 'hunter2') 1", + "CREATE TABLE t (a INT) WITH (\"password\" = '*******') 1"), + Arguments.of( + "pair in a trailing comment", + "SELECT 1 UNION foo_bar -- 'password' = 'hunter2'", + "SELECT 1 UNION foo_bar -- 'password' = '*******'")); + } + + @Test + void testPlainIdentifiersAreNotMasked() { + assertThatThrownBy(() -> parser.parse("SELECT * FROM t WHERE token_count=5 UNION foo_bar")) + .isInstanceOf(SqlParserException.class) + .message() + .contains(" SELECT * FROM t WHERE token_count=5 UNION foo_bar\n"); + } + + @Test + void testExpressionParserOfThePlannerHonorsAdditionalSensitiveKeys() { + final TableConfig config = TableConfig.getDefault(); + config.set(SecurityOptions.ADDITIONAL_SENSITIVE_KEYS, List.of("hunter")); + final FlinkPlannerImpl planner = + PlannerMocks.newBuilder() + .withTableConfig(config) + .build() + .getPlannerContext() + .createFlinkPlanner(); + assertThatThrownBy(() -> planner.parser().parseExpression("f('my.hunter.opt' = 'x' 1)")) + .isInstanceOf(SqlParserException.class) + .message() + .contains(" f('my.hunter.opt' = '*' 1)\n") + .doesNotContain("'x'"); + } + + @Test + void testLongInputsStillProduceAParserException() { + assertThatThrownBy(() -> parser.parse("ELECT " + "a".repeat(100_000))) + .isInstanceOf(SqlParserException.class); + assertThatThrownBy( + () -> + parser.parse( + "CREATE TABLE t (a INT) WITH ('format' = '" + + "a".repeat(20_000) + + "') 1")) + .isInstanceOf(SqlParserException.class); + } + + @Test + void testErrorWithoutPositionIsUnchanged() { + assertThatThrownBy(() -> parser.parse("show databases in db.t")) + .isInstanceOf(SqlParserException.class) + .hasMessage( + "SQL parse failed. Show databases from/in identifier [ db.t ] format" + + " error, catalog must be a single part identifier."); + } + + @Test + void testIncompleteStatementIsReportedAsEof() { + assertThatThrownBy(() -> parser.parse("SELECT a FROM t WHERE")) + .isInstanceOf(SqlParserEOFException.class) + .hasMessageStartingWith("Encountered \"<EOF>\" at line 1, column 21."); + assertThatThrownBy(() -> parser.parseSqlList("SELECT a FROM t WHERE")) + .isInstanceOf(SqlParserEOFException.class) + .hasMessageStartingWith("Encountered \"<EOF>\" at line 1, column 21."); + } + + @Test + void testIncompleteExpressionIsNotReportedAsEof() { + assertThatThrownBy(() -> parser.parseExpression("a +")) + .isExactlyInstanceOf(SqlParserException.class) + .hasMessageStartingWith( + "SQL parse failed. Encountered \"+ <EOF>\" at line 1, column 3:\n" + + " a +\n" + + " ^\n"); + } +} diff --git a/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/parse/ParseErrorSnippetTest.java b/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/parse/ParseErrorSnippetTest.java new file mode 100644 index 00000000000..a68224c51b7 --- /dev/null +++ b/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/parse/ParseErrorSnippetTest.java @@ -0,0 +1,177 @@ +/* + * 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.planner.parse; + +import org.apache.calcite.sql.parser.SqlParserPos; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +import java.util.List; +import java.util.Optional; + +import static org.assertj.core.api.Assertions.assertThat; + +/** Tests for {@link ParseErrorSnippet}. */ +class ParseErrorSnippetTest { + + /** Longer than the 76-character window. */ + private static final String LONG_LINE = "SELECT " + "a".repeat(100) + " FROM t"; + + @Test + void testPointAndSpan() { + assertThat(render("SELECT a FROM t", new SqlParserPos(1, 8))) + .contains(" SELECT a FROM t\n ^"); + assertThat(render("SELECT a FROM t", new SqlParserPos(1, 10, 1, 13))) + .contains(" SELECT a FROM t\n ^^^^"); + } + + @Test + void testLineOfAMultiLineText() { + assertThat(render("SELECT a\r\nFROM t\r\nWHERE x", new SqlParserPos(2, 6))) + .contains(" FROM t\n ^"); + assertThat(render("SELECT a\nFROM t\nWHERE x", new SqlParserPos(3, 1, 3, 5))) + .contains(" WHERE x\n ^^^^^"); + assertThat(render("SELECT a\rFROM t\rWHERE x", new SqlParserPos(3, 1, 3, 5))) + .contains(" WHERE x\n ^^^^^"); + } + + @Test + void testSpanAcrossLinesEndsAtTheLineEnd() { + assertThat(render("SELECT a +\n b FROM t", new SqlParserPos(1, 8, 2, 3))) + .contains(" SELECT a +\n ^^^"); + } + + @Test + void testTrailingWhitespaceIsKeptUpToTheColumn() { + assertThat(render("SELECT a ", new SqlParserPos(1, 11))) + .contains(" SELECT a \n ^"); + assertThat(render("SELECT a ", new SqlParserPos(1, 8))) + .contains(" SELECT a\n ^"); + } + + @Test + void testUnknownOrMissingLineRendersNothing() { + assertThat(render("SELECT a", SqlParserPos.ZERO)).isEmpty(); + assertThat(render("SELECT a", new SqlParserPos(2, 1))).isEmpty(); + assertThat(render("SELECT a\n", new SqlParserPos(3, 1))).isEmpty(); + } + + @Test + void testTabIsOneColumn() { + assertThat(render("\tSELECT\ta", new SqlParserPos(1, 9))) + .contains(" SELECT a\n ^"); + } + + @Test + void testWideCharactersTakeTwoCellsBeforeTheCaret() { + assertThat(render("SELECT '日本語' UNION foo_bar", new SqlParserPos(1, 20, 1, 26))) + .contains(" SELECT '日本語' UNION foo_bar\n" + " ".repeat(26) + "^^^^^^^"); + assertThat(render("SELECT 'é' UNION foo_bar", new SqlParserPos(1, 19, 1, 25))) + .contains(" SELECT 'é' UNION foo_bar\n" + " ".repeat(21) + "^^^^^^^"); + assertThat(render("SELECT '日本' UNION foo", new SqlParserPos(1, 9, 1, 10))) + .contains(" SELECT '日本' UNION foo\n ^^^^"); + } + + @Test + void testLongLineKeepsItsStartWhenTheSpanIsThere() { + assertThat(render(LONG_LINE, new SqlParserPos(1, 1, 1, 6))) + .contains(" SELECT " + "a".repeat(66) + "...\n ^^^^^^"); + } + + @Test + void testLongLineKeepsItsEndWhenTheSpanIsThere() { + assertThat(render(LONG_LINE, new SqlParserPos(1, 109, 1, 112))) + .contains(" ..." + "a".repeat(66) + " FROM t\n " + " ".repeat(70) + "^^^^"); + } + + @Test + void testLongLineKeepsTenCharactersAfterTheSpanStart() { + assertThat(render(LONG_LINE, new SqlParserPos(1, 81, 1, 83))) + .contains(" ..." + "a".repeat(70) + "...\n " + " ".repeat(62) + "^^^"); + } + + @Test + void testCaretPastTheEndOfALongLineStaysWithinTheWindow() { + assertThat(render("x".repeat(76), new SqlParserPos(1, 77))) + .contains(" ..." + "x".repeat(72) + "\n " + " ".repeat(75) + "^"); + } + + @Test + void testWindowIsMeasuredInCells() { + assertThat(render("日".repeat(100), new SqlParserPos(1, 90))) + .contains(" ..." + "日".repeat(34) + "...\n " + " ".repeat(61) + "^^"); + assertThat(render("😀".repeat(100), new SqlParserPos(1, 99, 1, 100))) + .contains(" ..." + "😀".repeat(34) + "...\n " + " ".repeat(61) + "^^"); + } + + @Test + void testSpanIsCutWithTheLine() { + assertThat(render(LONG_LINE, new SqlParserPos(1, 1, 1, 100))) + .contains(" SELECT " + "a".repeat(66) + "...\n " + "^".repeat(73)); + } + + @ParameterizedTest(name = "{0}") + @CsvSource( + delimiter = '|', + quoteCharacter = '"', + value = { + "quoted pairs|WITH ('connector' = 'kafka', 'PASSWORD'='hunter2')|WITH ('connector' = 'kafka', 'PASSWORD'='*******')", + "doubled quote in the value|WITH ('a.secret.b' = 'it''s')|WITH ('a.secret.b' = '*****')", + "bare option key|SET s3.secret-key=abc, a=b|SET s3.secret-key=***, a=b", + "colon instead of equals|WITH ('password': 'hunter2')|WITH ('password': '*******')", + "arrow instead of equals|WITH ('password' => 'hunter2')|WITH ('password' => '*******')", + "no separator|WITH ('password' 'hunter2')|WITH ('password' '*******')", + "unterminated value|WITH ('password' = 'hun|WITH ('password' = '***", + "value quoted after a bare key|SET security.token='abc'|SET security.token='***'", + "double-quoted key|WITH (\"password\" = 'hunter2')|WITH (\"password\" = '*******')", + "back-quoted key and double-quoted value|WITH (`password` = \"hunter2\")|WITH (`password` = \"*******\")", + "long separator|WITH ('password' ==== 'hunter2')|WITH ('password' ==== '*******')", + "pair after a comment marker|SELECT 1 -- 'password' = 'hunter2'|SELECT 1 -- 'password' = '*******'", + "plain identifier is not a key|SELECT * FROM t WHERE token_count=5|SELECT * FROM t WHERE token_count=5", + "quoted literal without a value|SELECT 'password' FROM t|SELECT 'password' FROM t", + "non-sensitive pairs|WITH ('url' = 'jdbc:x', 'a-b' = 'c')|WITH ('url' = 'jdbc:x', 'a-b' = 'c')", + }) + void testSensitiveOptionValuesAreMasked(String name, String line, String expected) { + assertThat(render(line, new SqlParserPos(1, 1))).contains(" " + expected + "\n ^"); + } + + @Test + void testAdditionalSensitiveKeysAreMasked() { + final String line = "WITH ('my.custom' = 'x', 'other' = 'y')"; + assertThat(ParseErrorSnippet.render(line, new SqlParserPos(1, 1), List.of("custom"))) + .contains(" WITH ('my.custom' = '*', 'other' = 'y')\n ^"); + } + + @Test + void testLongLinesRenderInLinearTime() { + assertThat(render("ELECT " + "a".repeat(1_000_000), new SqlParserPos(1, 1, 1, 5))) + .isPresent(); + assertThat(render("x '" + "a".repeat(1_000_000) + "'", new SqlParserPos(1, 1))).isPresent(); + assertThat( + render( + "WITH ('password' = '" + "a".repeat(1_000_000) + "') 1", + new SqlParserPos(1, 1))) + .isPresent(); + } + + private static Optional<String> render(String sql, SqlParserPos pos) { + return ParseErrorSnippet.render(sql, pos, List.of()); + } +}
