test(sql): pin optimizer hint blocks survive format() round-trip (#38189) - #42733
test(sql): pin optimizer hint blocks survive format() round-trip (#38189)#42733rusackas wants to merge 3 commits into
Conversation
) Reporter's repro (SELECT with a /*+ SET_VAR(...) */ StarRocks/MySQL-style optimizer hint plus a trailing -- comment) doesn't reproduce against the actual execution-time code path: SQLStatement.format(), the same method executor.py and celery_task.py call to build the SQL sent to the engine. sqlglot 30.12.0 repositions the trailing comment safely after the statement instead of injecting it inside the hint block. Passes on current master, closing #38189 as already fixed.
Code Review Agent Run #c4c877Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42733 +/- ##
==========================================
- Coverage 65.57% 65.56% -0.01%
==========================================
Files 2818 2821 +3
Lines 160038 159249 -789
Branches 36557 36386 -171
==========================================
- Hits 104942 104419 -523
+ Misses 53051 52804 -247
+ Partials 2045 2026 -19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
The current test case in Updating the test to include a semicolon would look like this: sql = """SELECT /*+ SET_VAR(query_timeout = 3000) */ col1, col2
FROM my_table
LIMIT 100;
-- increase timeout for large scans"""This will verify if the formatter correctly handles the semicolon without corrupting the optimizer hint block. tests/unit_tests/sql/parse_tests.py |
) Review flagged that the no-semicolon repro passing on master doesn't mean #38189 is fully closed: a `;`-terminated statement still hits the comment relocation branch and corrupts the /*+ SET_VAR(...) */ hint block. Add an xfail(strict=True) test pinning that still-broken case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Code Review Agent Run #b9c9a6Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Assert the trailing comment survives outside the hint block (not just that the specific corruption string is absent), per codeant-ai review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #f0c768Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
3 participants
SUMMARY
Regression test for #38189, which reported that Superset's SQL parser injects a repositioned
--comment inside a/*+ SET_VAR(...) */optimizer hint block, producing invalid SQL for StarRocks/MySQL-style engines that use the/*+ ... */hint convention.The original repro (no terminating
;) doesn't reproduce against the actual execution-time code path:SQLStatement.format(), the same methodsuperset/sql/execution/executor.pyandsuperset/sql/execution/celery_task.pycall to build the SQL that's actually sent to the engine. With the currently pinnedsqlglot30.12.0, the trailing--comment is repositioned safely after the statement, not injected inside the hint block, for that form.However, a
;-terminated statement (as flagged in review) still hits the broken relocation branch and reproduces the corruption. So #38189 is only partially fixed and should stay open for that case. This PR adds two tests: one pinning the now-fixed no-semicolon form, and onexfail(strict=True)pinning the still-broken semicolon form so a future fix will surface as an unexpected pass and can flip to a real regression test.TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
--comments inside optimizer hint blocks (/*+ SET_VAR */), breaking StarRocks syntax #38189