Repository: tobymao/sqlglot
Pull request: fix!: Tokenize hints as comments
Fixes #4425

Currently, hints are not tokenized as a single entity like we do for the comments (i.e through `_scan_comments()`), but instead the following token stream is produced:

```
>>> tokens = tokenize("SELECT /*+ hint */ * FROM t")
>>> for token in tokens:
   ... print(token)

<Token token_type: TokenType.SELECT, text: SELECT, line: 1, col: 6, start: 0, end: 5, comments: []>
<Token token_type: TokenType.HINT, text: /*+, line: 1, col: 10, start: 7, end: 9, comments: []>
<Token token_type: TokenType.VAR, text: hint, line: 1, col: 15, start: 11, end: 14, comments: []>
<Token token_type: TokenType.STAR, text: *, line: 1, col: 17, start: 16, end: 16, comments: []>
<Token token_type: TokenType.SLASH, text: /, line: 1, col: 18, start: 17, end: 17, comments: []>
<Token token_type: TokenType.STAR, text: *, line: 1, col: 20, start: 19, end: 19, comments: []>
<Token token_type: TokenType.FROM, text: FROM, line: 1, col: 25, start: 21, end: 24, comments: []>
<Token token_type: TokenType.VAR, text: t, line: 1, col: 27, start: 26, end: 26, comments: []>
```

For this reason, the hint is actually processed at parse time instead of tokenization time. This can lead to subtle bugs such as the one on the issue, where a slightly different pattern such as `SELECT /*+ hint */* FROM t` can throw off the tokenizer; In this case, we'd currently treat that as `SELECT /*+ * /*` i.e `SELECT <hint_start> * <comment_start> FROM t`.

This PR fixes this issue by:
- Treating hints as comments, so the entire hint text is processed by `scan_comments`
- Upon tokenizing a hint, we produce a `TokenType.HINT` with the payload/text attached to it's `comments`
- During parse time, we transform `TokenType.HINT` into an `exp.Hint` by parsing it's payload/text 
  

Problem statement:
TokenError: Error tokenizing 'select /*+ ORDERED */* from dual'
The tokenizer fails to parse a hint followed by a `*` if it is not separated by a space:

    /*+ ... */* from dual

```
import sqlglot

sqlglot.parse_one(
    '''
       select /*+ ORDERED */* from dual   
    '''
)
```

    sqlglot.errors.TokenError: Error tokenizing 'lect /*+ ORDERED */* from dual

This is valid syntax.

If I add a space between inside of `*/*` such that it is `*/ *`, it works fine.

```
sqlglot.parse_one(
    '''
       select /*+ ORDERED */ * from dual   
    '''
)
```

It also works when changing `*/*` to `/*1`.

```
sqlglot.parse_one(
    '''
       select /*+ ORDERED */1 from dual   
    '''
)

```
Note that this only works for hints, not comments, so this is fine:

```
sqlglot.parse_one(
    '''
       select /*+ ORDERED */1 from dual   
    '''
)
```

My guess is that the `*/*` is confusing the tokenizer for the start of a comment/hint like `/*`.

Thank you.

Hints from issue discussion:

Commit message: fix: Tokenize & parse hints as comments

Patch to review:
diff --git a/sqlglot/dialects/mysql.py b/sqlglot/dialects/mysql.py
index 1404f0fc32..434a212917 100644
--- a/sqlglot/dialects/mysql.py
+++ b/sqlglot/dialects/mysql.py
@@ -178,7 +178,12 @@ class MySQL(Dialect):
 
     class Tokenizer(tokens.Tokenizer):
         QUOTES = ["'", '"']
-        COMMENTS = ["--", "#", ("/*", "*/")]
+        COMMENTS = [
+            "--",
+            "#",
+            ("/*", "*/"),
+            ("/*+", "*/"),
+        ]
         IDENTIFIERS = ["`"]
         STRING_ESCAPES = ["'", '"', "\\"]
         BIT_STRINGS = [("b'", "'"), ("B'", "'"), ("0b", "")]
diff --git a/sqlglot/dialects/oracle.py b/sqlglot/dialects/oracle.py
index 1025a075a6..cda13e7897 100644
--- a/sqlglot/dialects/oracle.py
+++ b/sqlglot/dialects/oracle.py
@@ -15,7 +15,6 @@
 from sqlglot.helper import seq_get
 from sqlglot.parser import OPTIONS_TYPE, build_coalesce
 from sqlglot.tokens import TokenType
-from sqlglot.errors import ParseError
 
 if t.TYPE_CHECKING:
     from sqlglot._typing import E
@@ -207,35 +206,6 @@ def _parse_json_array(self, expr_type: t.Type[E], **kwargs) -> E:
                 **kwargs,
             )
 
-        def _parse_hint(self) -> t.Optional[exp.Hint]:
-            start_index = self._index
-            should_fallback_to_string = False
-
-            if not self._match(TokenType.HINT):
-                return None
-
-            hints = []
-
-            try:
-                for hint in iter(
-                    lambda: self._parse_csv(
-                        lambda: self._parse_hint_function_call() or self._parse_var(upper=True),
-                    ),
-                    [],
-                ):
-                    hints.extend(hint)
-            except ParseError:
-                should_fallback_to_string = True
-
-            if not self._match_pair(TokenType.STAR, TokenType.SLASH):
-                should_fallback_to_string = True
-
-            if should_fallback_to_string:
-                self._retreat(start_index)
-                return self._parse_hint_fallback_to_string()
-
-            return self.expression(exp.Hint, expressions=hints)
-
         def _parse_hint_function_call(self) -> t.Optional[exp.Expression]:
             if not self._curr or not self._next or self._next.token_type != TokenType.L_PAREN:
                 return None
@@ -258,20 +228,6 @@ def _parse_hint_args(self):
 
             return args
 
-        def _parse_hint_fallback_to_string(self) -> t.Optional[exp.Hint]:
-            if self._match(TokenType.HINT):
-                start = self._curr
-                while self._curr and not self._match_pair(TokenType.STAR, TokenType.SLASH):
-                    self._advance()
-
-                if not self._curr:
-                    self.raise_error("Expected */ after HINT")
-
-                end = self._tokens[self._index - 3]
-                return exp.Hint(expressions=[self._find_sql(start, end)])
-
-            return None
-
         def _parse_query_restrictions(self) -> t.Optional[exp.Expression]:
             kind = self._parse_var_from_options(self.QUERY_RESTRICTIONS, raise_unmatched=False)
 
diff --git a/sqlglot/parser.py b/sqlglot/parser.py
index cbbeffe5e1..a8513eb2a9 100644
--- a/sqlglot/parser.py
+++ b/sqlglot/parser.py
@@ -756,6 +756,7 @@ class Parser(metaclass=_Parser):
         exp.From: lambda self: self._parse_from(joins=True),
         exp.Group: lambda self: self._parse_group(),
         exp.Having: lambda self: self._parse_having(),
+        exp.Hint: lambda self: self._parse_hint_body(),
         exp.Identifier: lambda self: self._parse_id_var(),
         exp.Join: lambda self: self._parse_join(),
         exp.Lambda: lambda self: self._parse_lambda(),
@@ -2973,10 +2974,11 @@ def _parse_select(
         # duckdb supports leading with FROM x
         from_ = self._parse_from() if self._match(TokenType.FROM, advance=False) else None
 
+        hint = self._parse_hint()
         if self._match(TokenType.SELECT):
             comments = self._prev_comments
 
-            hint = self._parse_hint()
+            hint = hint or self._parse_hint()
 
             if self._next and not self._next.token_type == TokenType.DOT:
                 all_ = self._match(TokenType.ALL)
@@ -3226,21 +3228,42 @@ def _parse_query_modifiers(
 
         return this
 
-    def _parse_hint(self) -> t.Optional[exp.Hint]:
-        if self._match(TokenType.HINT):
-            hints = []
+    def _parse_hint_fallback_to_string(self) -> t.Optional[exp.Hint]:
+        start = self._curr
+        while self._curr:
+            self._advance()
+
+        end = self._tokens[self._index - 1]
+        return exp.Hint(expressions=[self._find_sql(start, end)])
+
+    def _parse_hint_function_call(self) -> t.Optional[exp.Expression]:
+        return self._parse_function_call()
+
+    def _parse_hint_body(self) -> t.Optional[exp.Hint]:
+        start_index = self._index
+        should_fallback_to_string = False
+
+        hints = []
+        try:
             for hint in iter(
                 lambda: self._parse_csv(
-                    lambda: self._parse_function() or self._parse_var(upper=True)
+                    lambda: self._parse_hint_function_call() or self._parse_var(upper=True),
                 ),
                 [],
             ):
                 hints.extend(hint)
+        except ParseError:
+            should_fallback_to_string = True
+
+        if should_fallback_to_string or self._curr:
+            self._retreat(start_index)
+            return self._parse_hint_fallback_to_string()
 
-            if not self._match_pair(TokenType.STAR, TokenType.SLASH):
-                self.raise_error("Expected */ after HINT")
+        return self.expression(exp.Hint, expressions=hints)
 
-            return self.expression(exp.Hint, expressions=hints)
+    def _parse_hint(self) -> t.Optional[exp.Hint]:
+        if self._match(TokenType.HINT) and self._prev.comments:
+            return exp.maybe_parse(self._prev.comments[0], into=exp.Hint, dialect=self.dialect)
 
         return None
 
diff --git a/sqlglot/tokens.py b/sqlglot/tokens.py
index 5e0ef41e70..7aed1fef12 100644
--- a/sqlglot/tokens.py
+++ b/sqlglot/tokens.py
@@ -561,6 +561,7 @@ def _quotes_to_format(
                 string=_TOKEN_TYPE_TO_INDEX[TokenType.STRING],
                 var=_TOKEN_TYPE_TO_INDEX[TokenType.VAR],
                 heredoc_string_alternative=_TOKEN_TYPE_TO_INDEX[klass.HEREDOC_STRING_ALTERNATIVE],
+                hint=_TOKEN_TYPE_TO_INDEX[TokenType.HINT],
             )
             klass._RS_TOKENIZER = RsTokenizer(settings, token_types)
         else:
@@ -954,7 +955,11 @@ class Tokenizer(metaclass=_Tokenizer):
     # Handle numeric literals like in hive (3L = BIGINT)
     NUMERIC_LITERALS: t.Dict[str, str] = {}
 
-    COMMENTS = ["--", ("/*", "*/")]
+    COMMENTS = [
+        "--",
+        ("/*", "*/"),
+        ("/*+", "*/"),
+    ]
 
     __slots__ = (
         "sql",
@@ -1230,6 +1235,9 @@ def _scan_comment(self, comment_start: str) -> bool:
                 self._advance(alnum=True)
             self._comments.append(self._text[comment_start_size:])
 
+        if comment_start == "/*+":
+            self._add(TokenType.HINT)
+
         # Leading comment is attached to the succeeding token, whilst trailing comment to the preceding.
         # Multiple consecutive comments are preserved by appending them to the current comments list.
         if comment_start_line == self._prev_token_line:
diff --git a/sqlglotrs/src/settings.rs b/sqlglotrs/src/settings.rs
index 3068fd2813..bf66b5acd5 100644
--- a/sqlglotrs/src/settings.rs
+++ b/sqlglotrs/src/settings.rs
@@ -19,6 +19,7 @@ pub struct TokenTypeSettings {
     pub string: TokenType,
     pub var: TokenType,
     pub heredoc_string_alternative: TokenType,
+    pub hint: TokenType,
 }
 
 #[pymethods]
@@ -38,6 +39,7 @@ impl TokenTypeSettings {
         string: TokenType,
         var: TokenType,
         heredoc_string_alternative: TokenType,
+        hint: TokenType,
     ) -> Self {
         TokenTypeSettings {
             bit_string,
@@ -53,6 +55,7 @@ impl TokenTypeSettings {
             string,
             var,
             heredoc_string_alternative,
+            hint,
         }
     }
 }
diff --git a/sqlglotrs/src/tokenizer.rs b/sqlglotrs/src/tokenizer.rs
index beff96e2fc..53167f7982 100644
--- a/sqlglotrs/src/tokenizer.rs
+++ b/sqlglotrs/src/tokenizer.rs
@@ -395,6 +395,10 @@ impl<'a> TokenizerState<'a> {
                 .push(self.text()[comment_start_size..].to_string());
         }
 
+        if comment_start == "/*+" {
+            self.add(self.token_types.hint, None)?;
+        }
+
         // Leading comment is attached to the succeeding token, whilst trailing comment to the preceding.
         // Multiple consecutive comments are preserved by appending them to the current comments list.
         if Some(comment_start_line) == self.previous_token_line {
diff --git a/tests/dialects/test_postgres.py b/tests/dialects/test_postgres.py
index ffe08c67eb..00d89df477 100644
--- a/tests/dialects/test_postgres.py
+++ b/tests/dialects/test_postgres.py
@@ -127,10 +127,6 @@ def test_postgres(self):
             "pg_catalog.PG_TABLE_IS_VISIBLE(c.oid) "
             "ORDER BY 2, 3"
         )
-        self.validate_identity(
-            "/*+ some comment*/ SELECT b.foo, b.bar FROM baz AS b",
-            "/* + some comment */ SELECT b.foo, b.bar FROM baz AS b",
-        )
         self.validate_identity(
             "SELECT ARRAY[1, 2, 3] <@ ARRAY[1, 2]",
             "SELECT ARRAY[1, 2] @> ARRAY[1, 2, 3]",
@@ -819,6 +815,13 @@ def test_postgres(self):
             },
         )
 
+        self.validate_all(
+            "/*+ some comment*/ SELECT b.foo, b.bar FROM baz AS b",
+            write={
+                "postgres": UnsupportedError,
+            },
+        )
+
     def test_ddl(self):
         # Checks that user-defined types are parsed into DataType instead of Identifier
         self.parse_one("CREATE TABLE t (a udt)").this.expressions[0].args["kind"].assert_is(


Code review comments:
diff_hunk: @@ -1230,6 +1235,9 @@ def _scan_comment(self, comment_start: str) -> bool:
                 self._advance(alnum=True)
             self._comments.append(self._text[comment_start_size:])
 
+        if comment_start == "/*+":
path: sqlglot/tokens.py
text: @user1:
make this a constant?

@user1:
i guess it needs to be passed in and configurable?

@user2:
I agree about making `/*+` a constant, not sure if we need to make it configurable. Isn't this pretty much universal? Perhaps there's something like `/*!` in MySQL or somewhere else? I recall seeing something similar but need to search docs...

@author:
Looks like `/*!` does exist for MySQL: https://dev.mysql.com/doc/refman/8.4/en/comments.html



@author:
Doesn't seem like a hint though, more like a MySQL specific code pass-through so the `/*+` should be universal

diff_hunk: @@ -3226,21 +3228,42 @@ def _parse_query_modifiers(
 
         return this
 
-    def _parse_hint(self) -> t.Optional[exp.Hint]:
-        if self._match(TokenType.HINT):
-            hints = []
+    def _parse_hint_fallback_to_string(self) -> t.Optional[exp.Hint]:
+        start = self._curr
+        while self._curr:
+            self._advance()
+
+        end = self._tokens[self._index - 1]
+        return exp.Hint(expressions=[self._find_sql(start, end)])
+
+    def _parse_hint_function_call(self) -> t.Optional[exp.Expression]:
+        return self._parse_function_call()
+
+    def _parse_hint_body(self) -> t.Optional[exp.Hint]:
+        start_index = self._index
+        should_fallback_to_string = False
+
+        hints = []
+        try:
             for hint in iter(
                 lambda: self._parse_csv(
-                    lambda: self._parse_function() or self._parse_var(upper=True)
+                    lambda: self._parse_hint_function_call() or self._parse_var(upper=True),
                 ),
                 [],
             ):
                 hints.extend(hint)
+        except ParseError:
+            should_fallback_to_string = True
+
+        if should_fallback_to_string or self._curr:
+            self._retreat(start_index)
+            return self._parse_hint_fallback_to_string()
 
-            if not self._match_pair(TokenType.STAR, TokenType.SLASH):
-                self.raise_error("Expected */ after HINT")
+        return self.expression(exp.Hint, expressions=hints)
 
-            return self.expression(exp.Hint, expressions=hints)
+    def _parse_hint(self) -> t.Optional[exp.Hint]:
+        if self._match(TokenType.HINT) and self._prev.comments:
+            return exp.maybe_parse(self._prev.comments[0], into=exp.Hint, dialect=self.dialect)
path: sqlglot/parser.py
text: @user1:
```suggestion
    def _parse_hint(self) -> t.Optional[exp.Hint]:
        if self._match(TokenType.HINT) and self._prev_comments:
            return exp.maybe_parse(self._prev_comments[0], into=exp.Hint, dialect=self.dialect)
```

diff_hunk: @@ -2973,10 +2974,11 @@ def _parse_select(
         # duckdb supports leading with FROM x
         from_ = self._parse_from() if self._match(TokenType.FROM, advance=False) else None
 
+        hint = self._parse_hint()
path: sqlglot/parser.py
text: @user1:
Why was this added? It looks like this affects the Postgres test. Is there documentation that backs this up?

@author:
Ditto about the hint tokenization.

Let's assume we have the following query:
```SQL
/*+ comment */ SELECT ...
```

Before, this was treated as a comment so it'd be attached to the following `exp.Select` node, but now this'd be tokenized as a `TokenType.HINT`, so `exp.Select` needs to parse that before `TokenType.SELECT`



@user1:
I replied to the other thread. I don't think this is right? Hints should be after the `SELECT` token IIRC.

diff_hunk: @@ -954,7 +955,11 @@ class Tokenizer(metaclass=_Tokenizer):
     # Handle numeric literals like in hive (3L = BIGINT)
     NUMERIC_LITERALS: t.Dict[str, str] = {}
 
-    COMMENTS = ["--", ("/*", "*/")]
+    COMMENTS = [
+        "--",
+        ("/*", "*/"),
+        ("/*+", "*/"),
+    ]
path: sqlglot/tokens.py
text: @user1:
Since `/*+` was previously treated as a hint for all dialects, we don't have to add it in `COMMENTS`, but rather do it in the metaclass. That way you don't have to change MySQL, for example. It would look like this:

```python
        klass._COMMENTS = {
            **dict(
                (comment, None) if isinstance(comment, str) else (comment[0], comment[1])
                for comment in klass.COMMENTS
            ),
            "{#": "#}",  # Ensure Jinja comments are tokenized correctly in all dialects
            "/*+": "*/",  # Ensure hints are treated as comments and tokenized correctly in all dialects
        }
```