Repository navigation
MDEV-41606 Support line comments in optimizer hints - #5935
Chaitanya-970 wants to merge 1 commit into
Conversation
Optimizer hints reject SQL-style -- comments, causing the whole hint block to be ignored. Skip line comments between tokens using the SQL lexer's whitespace and control-character rule. Add regression coverage for comments between hints and arguments, quoted identifiers, trailing comments, CRLF input and diagnostic line numbers. Assisted-by: Codex:gpt-6-astra
|
|
gkodinov
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
It's a good first iteration, but it needs more work IMHO. I have drafted some suggestions to improve the diff below.
Please also add a more detailed description either in the jira and a summary of it into the commit message. This should contribute to better documentation.
This should cover:
- what was the state before (a.k.a. what was wrong)
- what new functionality is added
- what is supposed to work
- what is supposed to not work
- what is missing (if anything)
- how was the feature tested
| for ( ; ; ) | ||
| { | ||
| get_spaces(); | ||
| if (m_end - m_ptr < 2 || m_ptr[0] != '-' || m_ptr[1] != '-') |
There was a problem hiding this comment.
that would be 3, wouldn't it? Please add a comment explaining what it means and why it's 3.
| @@ -224,7 +224,18 @@ Optimizer_hint_tokenizer::find_keyword(const LEX_CSTRING &str) | |||
| Optimizer_hint_tokenizer::Token | |||
| Optimizer_hint_tokenizer::get_token(CHARSET_INFO *cs) | |||
There was a problem hiding this comment.
I believe the proper way to do this is to add a new state to the parser for the allowed comments. And deal with it as it comes up during parsing. The below is just adding "--comments" before identifiers. So something like
/*+
QB_NAME(child_select -- this is a comment
,derived1 .derived2)
*/
would still fail.
This said, I do not fully understand why a separate lexer and parser are needed and why this couldn't have been done in the main one. But, frankly, I'm a bit out of my depth here and lacking the historical context. So I'll let that slide.
| DEALLOCATE PREPARE stmt; | ||
|
|
||
| --echo # Whitespace is required after the two dashes | ||
| --disable_ps_protocol |
There was a problem hiding this comment.
why do you need to disable these?
| --enable_ps_protocol | ||
| --enable_view_protocol | ||
|
|
||
| DROP TABLE t1, t2; |
There was a problem hiding this comment.
We customarily end .test files with a comment like:
--echo End of x.y tests
Also, please add tests that cover comments in all possible locations (basically, every call to get_spaces()).
3 participants
Implements MDEV-41606.
Optimizer hints currently reject SQL-style
--comments, causing the whole hint block to be ignored. Teach the tokenizer to skip line comments between tokens, using the SQL lexer's whitespace and control-character rule.The regression covers comments between hints and arguments, quoted identifiers, trailing comments, CRLF input and syntax-error line numbers.
Tested on a Linux Debug build:
--ps-protocoland--cursor-protocol.I submit this contribution under the BSD-new license.