Add SQL comment support to the Relational lexer by hatyo · Pull Request #4392 · FoundationDB/fdb-record-layer

hatyo · 2026-07-23T17:41:57Z

This adds proper SQL comment support to the Relational layer and settles the dialect on ANSI/PostgreSQL semantics. Line comments now start at -- and run to the end of the line without requiring trailing whitespace (so SELECT 1--comment reads as SELECT 1), and block comments use /* ... */ and may span multiple lines.

The MySQL-specific styles are dropped: the # line comment and the /*! ... */ executable comment, along with its dedicated lexer channel, are removed (they were not supported anyway). Outside of a string literal # is now a syntax error, and /*! ... */ is simply treated as an ordinary, ignored block comment.

Comments are handled purely at the lexical level, they're routed to hidden channels and stripped before anything reaches the parser, so they can't carry meaning, and hint-style comments are intentionally unsupported. Because the canonical query string is rebuilt from the parse tree by the normalizer, comments are automatically excluded from the plan cache key: two statements that differ only in their comments produce the same key and hash. Comment markers that appear inside string literals are left untouched, so a literal like 'a -- b' keeps its value verbatim.

Added testing on multiple levels, and comprehensive documentation to SQL comments (with documentation tests).

Breaking change: `--` without surrounding whitespace

Because -- now begins a comment even when it is not followed by whitespace, an unspaced subtraction of a negative literal changes meaning. SELECT 1--1 previously parsed as SELECT 1 - -1 and evaluated to 2; it now parses as SELECT 1, with the --1 and the rest of the line commented out, and evaluates to 1.

This fixes #4391.

Route ANSI/PostgreSQL line comments (--) and C-style block comments (/* */) to the HIDDEN channel so they are stripped before reaching the parse tree, and are therefore excluded from the canonical query string and the plan cache key. Adopt ANSI/PostgreSQL semantics where '--' starts a comment without requiring a following whitespace. Drop the MySQL-specific comment styles: the '#' line comment and the '/*!' executable-comment rule (with its dedicated MYSQLCOMMENT channel). A '/*!' sequence now falls through to the ordinary block-comment rule. Comments are handled purely at the lexical level, so hint-style comments are intentionally unsupported (that would require parser involvement). Add parser-level JUnit tests and end-to-end yamsql coverage; the yamsql file is gated to the current version since it exercises new behaviour.

robert-brunel · 2026-07-29T13:12:20Z

+Comments are only recognized outside of string literals. The sequences `--` and `/*` inside a quoted string are ordinary data, so the literal `'a -- b'` retains its value verbatim.
+
+```{note}
+Unlike MySQL, the Relational Layer does **not** support `#` line comments or `/*! ... */` executable comments. Outside of a string literal, `#` is a syntax error, and a `/*! ... */` sequence is treated as an ordinary block comment (its contents are ignored rather than executed).


The Unlike MySQL, comparison could be dropped or phrased more generically, like Unlike some SQL dialects. As far as I know, we don’t mention anywhere else in the documentation that the dialect is derived from MySQL; it’s just an internal detail in the grammar and lexer so far.

robert-brunel · 2026-07-29T13:18:57Z

 ```{toctree}
 :maxdepth: 1

+Comments


Perhaps generalize the section name to Syntax or Lexical structure or Language structure and syntax or Basic elements or something like that, because Comments feels oddly specific for a “Concepts” subsection. Then, for now, just make Comments the only subsection therein.

Though we could always revisit that section later as well.

Made a dedicated Lexical Structure section, currently it has only a comment section, but we can flesh it out with more content later.

robert-brunel · 2026-07-29T13:21:03Z


+    @Test
+    void commentsAreExcludedFromCanonicalQueryString() throws Exception {
+        //


Nit: No need for the extra empty // lines really :-)

The comments are maybe a bit on the verbose and repetitive side, across the change. That could be dialed back a bit. But I don’t feel that strongly.

I like the comment spacing actually as it helps increasing the readability of dense parts of the code. But yeah in this file perhaps that's not necessary. I will that up.

robert-brunel · 2026-07-29T13:23:58Z

+                .isEqualTo(bareResult.getQueryCacheKey());
+        Assertions.assertThat(commentedResult.getQueryCacheKey().hashCode())
+                .as("commented and bare queries must share the same cache key hash")
+                .isEqualTo(bareResult.getQueryCacheKey().hashCode());


hasSameHashCodeAs()

robert-brunel · 2026-07-29T13:25:18Z

+    }
+
+    @Test
+    void lineCommentMarkersInsideStringLiteralAreNotStripped() throws Exception {


What about comment markers inside double quotes, are there tests for those as well?

It could also be interesting to test how multi-line quoted identifiers (containing \n) interact with the ---style comment which normally consumes the rest of the line. Although admittedly that’s a really strange case.

There are tests for comments being ignored correctly when within single quoted string literals lineCommentMarkersInsideStringLiteralAreNotStripped and blockCommentMarkersInsideStringLiteralAreNotStripped.

New added tests for ignoring comments within double-quoted identifiers: commentMarkersInsideQuotedIdentifierAreNotStripped and multiLineQuotedIdentifierIsNotTruncatedByLineComment

robert-brunel · 2026-07-29T13:42:27Z

-                                       | '--' ('\r'? '\n' | EOF)
-                                     ) -> channel(HIDDEN);
-
+LINE_COMMENT:                        '--' ~[\r\n]* ('\r'? '\n' | EOF) -> channel(HIDDEN);


As an alternative to channel(HIDDEN) there is also skip. Did you consider that? skip could be preferable, because channel(HIDDEN) does technically emit a token. Which means that adding such a comment will impact the subsequent token indexes, and this in turn can affect the IDs in literals, specifically. I’m not sure whether that actually flows into the plan cache keys or hashes, but the skip could make that even more stable when there are constant literals present.

Good observation, indeed that's a bug, switched to skip and added regression tests. Thanks.

robert-brunel · 2026-07-29T13:44:36Z

+        //
+        // 'SELECT 1--1' -> the '--1' (and the rest of the line) is a comment, so this equals 'SELECT 1 FROM T'.
+        //
+        assertSameTree("SELECT 1--1\nFROM T",


So this is a case where a user might get silent changes of query results, right? SELECT 1--1 is now SELECT 1 instead of SELECT 1 - -1 aka. SELECT 2. Perhaps something to call out in the commit message or even in the release notes, although it an obscure case.

Right, I will adjust the commit message to reflect this, though it is, as you noted, a highly obscure case that likely can be changed without much concern for customer impact.

robert-brunel · 2026-07-29T13:46:18Z

         [('101 Branch St', 'New York', 10001), ('202 Office Rd', 'Boston', 02101)]
        ),
        (2, 'Global Inc',
-         null,  # Headquarters can be null


If there are any persisted SQL strings such as schema templates out there that use such a comment for some reason, they would fail to re-parse after upgrading. Could be another thing worth pointing out prominently in the commit message!

schema-templates are not serialized as strings, they get translated to record metadata pb message, so comments will be dropped way before serialization.

robert-brunel · 2026-07-29T13:48:37Z

-                                       | '--' ('\r'? '\n' | EOF)
-                                     ) -> channel(HIDDEN);
-
+LINE_COMMENT:                        '--' ~[\r\n]* ('\r'? '\n' | EOF) -> channel(HIDDEN);


What happens when the comment ends with \r alone? It’s probably a syntax error—which I guess is fine. But it could be worth a test. What does PostgreSQL/ANSI semantics say about that case?

verified in pgsql, added support for \r alone, see the new set of tests.

robert-brunel · 2026-07-29T13:51:03Z

+        //
+        final var arithmetic = QueryParser.parse("SELECT 3 - -2 FROM T").getRootContext().getText();
+        final var commented = QueryParser.parse("SELECT 3 FROM T").getRootContext().getText();
+        assertThat(arithmetic).isNotEqualTo(commented);


Instead of just isNotEqualTo(), can we maybe make stronger assertSameTree-type asserts to verify how exactly they differ?

hatyo added enhancement New feature or request breaking change Changes that are not backwards compatible documentation Documentation change labels Jul 23, 2026

arnaud-lacurie requested a review from robert-brunel July 28, 2026 12:00

hatyo added 3 commits July 28, 2026 18:30

Fix SQL comment.

8886253

fix test.

42cbca9

hatyo force-pushed the sql-comments branch from 71c40e9 to 42cbca9 Compare July 28, 2026 16:33

robert-brunel requested changes Jul 29, 2026

View reviewed changes

hatyo added 2 commits August 24, 2026 11:05

address comments.

cf98f29

Merge remote-tracking branch 'upstream/main' into sql-comments

7283184

hatyo force-pushed the sql-comments branch from 6375c81 to 7283184 Compare August 24, 2026 11:36

hatyo added 2 commits August 25, 2026 10:21

Merge branch 'main' into sql-comments

c62f36c

Merge branch 'main' into sql-comments

69ea4a2

Uh oh!

Conversation

hatyo commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change: -- without surrounding whitespace

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hatyo commented Jul 23, 2026 •

edited

Loading

Breaking change: `--` without surrounding whitespace