diff --git a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/ParserUtils.java b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/ParserUtils.java index 4390939d2148..23b8a70237d0 100644 --- a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/ParserUtils.java +++ b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/ParserUtils.java @@ -47,17 +47,90 @@ public static void validateFunction(String canonicalName, List opera */ public static String sanitizeSql(String sql) { - // 1. Remove trailing whitespaces + // 1. Strip single-line SQL comments (-- ... to end of line). + // The legacy OPTIONS regex anchors at end-of-string, so without this step a query + // like "SELECT col1 FROM foo -- option(skipUpsert=true)" would be mistakenly matched + // as if skipUpsert were a real query option. + sql = stripSingleLineComments(sql); + // 2. Remove trailing whitespace int endIndex = sql.length() - 1; while (endIndex >= 0 && Character.isWhitespace(sql.charAt(endIndex))) { endIndex--; } - sql = sql.substring(0, endIndex + 1); - - // Likewise extend for other improvements + return sql.substring(0, endIndex + 1); + } - return sql; + /** + * Returns the sql string with all single-line SQL comments (-- ... to end of line) removed, + * respecting single-quoted string literals, double-quoted identifiers, and block comments. + * A "--" found inside a block comment or a quoted context is not treated as a comment marker. + */ + static String stripSingleLineComments(String sql) { + StringBuilder result = new StringBuilder(sql.length()); + int len = sql.length(); + boolean inSingleQuote = false; + boolean inDoubleQuote = false; + boolean inBlockComment = false; + int i = 0; + while (i < len) { + char c = sql.charAt(i); + if (inBlockComment) { + result.append(c); + if (c == '*' && i + 1 < len && sql.charAt(i + 1) == '/') { + result.append('/'); + inBlockComment = false; + i += 2; + } else { + i++; + } + } else if (inSingleQuote) { + result.append(c); + if (c == '\'' && i + 1 < len && sql.charAt(i + 1) == '\'') { + result.append('\''); + i += 2; // '' escape inside a single-quoted literal + } else { + if (c == '\'') { + inSingleQuote = false; + } + i++; + } + } else if (inDoubleQuote) { + result.append(c); + if (c == '"' && i + 1 < len && sql.charAt(i + 1) == '"') { + result.append('"'); + i += 2; // "" escape inside a double-quoted identifier + } else { + if (c == '"') { + inDoubleQuote = false; + } + i++; + } + } else { + if (c == '\'') { + inSingleQuote = true; + result.append(c); + i++; + } else if (c == '"') { + inDoubleQuote = true; + result.append(c); + i++; + } else if (c == '/' && i + 1 < len && sql.charAt(i + 1) == '*') { + inBlockComment = true; + result.append(c); + i++; + } else if (c == '-' && i + 1 < len && sql.charAt(i + 1) == '-') { + // Skip from here to end of line; the newline itself is kept. + while (i < len && sql.charAt(i) != '\n') { + i++; + } + } else { + result.append(c); + i++; + } + } + } + return result.toString(); } private static void validateJsonExtractScalarFunction(List operands) { diff --git a/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java b/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java index 46bb7b094086..20890a10ab12 100644 --- a/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java +++ b/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java @@ -879,6 +879,27 @@ public void testQueryOptions() { } catch (SqlCompilationException e) { Assert.assertTrue(e.getCause() instanceof ParseException); } + + // Options inside SQL comments must NOT be honored. + // A trailing -- comment should not be mistaken for a real query option. + pinotQuery = compileToPinotQuery( + "SELECT col1, count(*) FROM foo GROUP BY col1 -- option(skipUpsert=true)"); + Assert.assertTrue(pinotQuery.getQueryOptions() == null || pinotQuery.getQueryOptions().isEmpty(), + "option(...) inside a -- comment must not be parsed as a query option"); + + // Same check when the -- comment appears inline after a WHERE predicate (multi-line query). + // The regex finds OPTION() starting from the 'O', ignoring the preceding '--', so without + // the fix this would incorrectly honour skipUpsert. + pinotQuery = compileToPinotQuery( + "select *\nfrom foo\nwhere uuid = 1 --OPTION(skipUpsert = true)"); + Assert.assertTrue(pinotQuery.getQueryOptions() == null || pinotQuery.getQueryOptions().isEmpty(), + "OPTION(...) after -- in a WHERE clause must not be parsed as a query option"); + + // A /* */ block comment should also not trigger the OPTIONS parser. + pinotQuery = compileToPinotQuery( + "SELECT col1, count(*) FROM foo GROUP BY col1 /* option(skipUpsert=true) */"); + Assert.assertTrue(pinotQuery.getQueryOptions() == null || pinotQuery.getQueryOptions().isEmpty(), + "option(...) inside a /* */ comment must not be parsed as a query option"); } @Test