This is an automated email from the ASF dual-hosted git repository.
shauryachats pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 3d62d6fcd54 [bugfix] fix(sql): strip trailing -- comments before
OPTIONS regex match (#18425)
3d62d6fcd54 is described below
commit 3d62d6fcd54c0c79b063c08da54b7338a7e9b713
Author: tarun11Mavani <[email protected]>
AuthorDate: Tue May 26 11:55:19 2026 +0530
[bugfix] fix(sql): strip trailing -- comments before OPTIONS regex match
(#18425)
---
.../org/apache/pinot/sql/parsers/ParserUtils.java | 83 ++++++++++++++++++++--
.../pinot/sql/parsers/CalciteSqlCompilerTest.java | 27 +++++++
2 files changed, 105 insertions(+), 5 deletions(-)
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 4390939d214..23b8a70237d 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 class ParserUtils {
*/
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<Expression>
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 532b28ab0bd..9db25530264 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,33 @@ public class CalciteSqlCompilerTest {
} 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");
+
+ // Double dashes inside a double-quoted identifier must not be treated as
a comment.
+ pinotQuery = compileToPinotQuery(
+ "SELECT \"my--column--name\" FROM foo");
+ Assert.assertEquals(
+ pinotQuery.getSelectList().get(0).getIdentifier().getName(),
"my--column--name");
}
@Test
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
