feat(dqp): support partial ordered topn set_var and external hints#116
feat(dqp): support partial ordered topn set_var and external hints#116hawkingrei merged 2 commits intomainfrom
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
shiro | 860b037 | Commit Preview URL Branch Preview URL |
Feb 15 2026, 04:52 PM |
Summary of ChangesHello @hawkingrei, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the DQP's flexibility and testability by integrating direct control over the tidb_opt_partial_ordered_index_for_topn optimizer variable and introducing a robust system for external hint injection. These changes empower users to fine-tune query optimization behavior and extend DQP's capabilities through configuration, facilitating more comprehensive testing and custom optimization strategies without requiring code modifications. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for tidb_opt_partial_ordered_index_for_topn as a SET_VAR hint in DQP and a mechanism for injecting external hints via configuration. However, the DQPExternalHints configuration field introduces a critical SQL injection vulnerability, as arbitrary strings are injected into SQL queries without sanitization for the comment closer */. Additionally, there's a logic issue in the external hint parsing that could generate invalid SQL.
There was a problem hiding this comment.
Pull request overview
This pull request adds support for TiDB's tidb_opt_partial_ordered_index_for_topn optimizer variable to the DQP (Differential Query Plan) oracle, along with a flexible external hint injection mechanism. The feature allows testing of the partial ordered index optimization for TopN operators and enables operator-specific hint extensions without code changes.
Changes:
- Added built-in SET_VAR toggles for partial ordered TopN optimization (
COSTandDISABLEmodes) - Introduced config-driven external DQP hint injection via
oracles.dqp_external_hintsfield - Implemented dual-format hint support: full hint strings and
var=valueshorthand auto-wrapped to SET_VAR
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/oracle/hints.go | Added constants for partial ordered TopN SET_VAR hints |
| internal/oracle/dqp.go | Implemented external hint parsing, splitting, and integration into DQP variant generation |
| internal/config/config.go | Added DQPExternalHints field to OracleConfig |
| internal/oracle/dqp_test.go | Added test coverage for partial ordered TopN hints and external hint processing |
| internal/config/config_test.go | Added test for loading DQP external hints from config |
| README.md | Documented DQP external hint injection feature with usage examples |
| AGENTS.md | Updated recent changes log with implementation details |
| docs/todo.md | Updated latest sync entry with implementation summary |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for external DQP hints via a new configuration option, oracles.dqp_external_hints, and adds built-in SET_VAR toggles for tidb_opt_partial_ordered_index_for_topn. The changes are well-structured, with new logic for parsing and classifying hints, along with comprehensive tests. My review includes a high-severity fix for a potential bug in hint normalization logic that could misclassify certain hints, and a medium-severity suggestion to improve test code robustness. All comments align with the repository's code review guidelines.
| if strings.Contains(trimmed, "=") { | ||
| return "SET_VAR(" + trimmed + ")", true, true | ||
| } |
There was a problem hiding this comment.
The current logic for detecting SET_VAR shorthand hints is too broad. It uses strings.Contains(trimmed, "="), which could incorrectly classify a regular optimizer hint containing an equals sign (e.g., in a string literal argument) as a SET_VAR hint. The shorthand form var=value for session variables typically does not contain parentheses. To avoid this misclassification, the check should be more specific by also ensuring no parentheses are present in the shorthand form.
I also recommend adding a test case to TestNormalizeSetVarHintClassification for a base hint containing an equals sign to prevent future regressions, for example:
{
name: "base_hint_with_equals",
raw: "HYPOTHETICAL_HINT(key='value')",
hint: "",
isSetVar: false,
valid: false,
},| if strings.Contains(trimmed, "=") { | |
| return "SET_VAR(" + trimmed + ")", true, true | |
| } | |
| if strings.Contains(trimmed, "=") && !strings.Contains(trimmed, "(") { | |
| return "SET_VAR(" + trimmed + ")", true, true | |
| } |
| if len(cfg.Oracles.DQPExternalHints) != 2 { | ||
| t.Fatalf("unexpected dqp external hints count: %d", len(cfg.Oracles.DQPExternalHints)) | ||
| } | ||
| if cfg.Oracles.DQPExternalHints[0] != "SET_VAR(tidb_opt_partial_ordered_index_for_topn='COST')" { | ||
| t.Fatalf("unexpected first dqp external hint: %s", cfg.Oracles.DQPExternalHints[0]) | ||
| } | ||
| if cfg.Oracles.DQPExternalHints[1] != "HASH_JOIN" { | ||
| t.Fatalf("unexpected second dqp external hint: %s", cfg.Oracles.DQPExternalHints[1]) | ||
| } |
There was a problem hiding this comment.
The assertions for the DQPExternalHints slice can be made more robust and maintainable by comparing the entire slice at once using reflect.DeepEqual, rather than checking the length and each element individually. This makes the test cleaner and provides a more comprehensive diff if the test fails. This change requires importing the reflect package.
expectedHints := []string{
"SET_VAR(tidb_opt_partial_ordered_index_for_topn='COST')",
"HASH_JOIN",
}
if !reflect.DeepEqual(cfg.Oracles.DQPExternalHints, expectedHints) {
t.Fatalf("unexpected dqp external hints\ngot: %v\nwant: %v", cfg.Oracles.DQPExternalHints, expectedHints)
}There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| candidates = append(candidates, toggleHints(SetVarEnableHashJoinOn, SetVarEnableHashJoinOff)...) | ||
| candidates = append(candidates, toggleHints(SetVarEnableOuterJoinReorderOn, SetVarEnableOuterJoinReorderOff)...) | ||
| candidates = append(candidates, toggleHints(SetVarEnableInlJoinInnerMultiOn, SetVarEnableInlJoinInnerMultiOff)...) | ||
| } |
There was a problem hiding this comment.
The partial ordered TopN SET_VAR hints are added unconditionally to all query candidates (line 252), regardless of whether the query has ORDER BY or LIMIT clauses that would make TOP-N optimization relevant. Other hints in this function are conditionally added based on query features (e.g., hasJoin for join-related hints, hasSubquery for subquery hints, hasPartition for partition hints). Consider adding a condition like if hasOrderBy || hasLimit to only include these hints when they would be applicable, or document why these hints should apply to all queries.
| } | |
| } | |
| // Apply partial ordered TopN SET_VAR hints to all queries intentionally. Even when | |
| // there is no explicit ORDER BY or LIMIT, we want to exercise optimizer behavior | |
| // controlled by these knobs across a wide range of plans. |
Summary
SET_VARtoggles fortidb_opt_partial_ordered_index_for_topn:SET_VAR(tidb_opt_partial_ordered_index_for_topn='COST')SET_VAR(tidb_opt_partial_ordered_index_for_topn='DISABLE')oracles.dqp_external_hintsSET_VAR(...),HASH_JOIN(...))var=valueshorthand auto-wrapped toSET_VAR(var=value)Motivation
tidb_opt_partial_ordered_index_for_topnis a new TiDB optimizer variable (SET_VAR-controllable). DQP should be able to stress this branch directly and also allow external, operator-specific hint extensions without changing code.Scope of change
internal/oracle/hints.gointernal/oracle/dqp.gointernal/config/config.gooracles.dqp_external_hintsconfig fieldinternal/oracle/dqp_test.gointernal/config/config_test.goREADME.mdAGENTS.mddocs/todo.mdRelated references
Test
GOCACHE=/tmp/shiro-go-cache /Users/weizhenwang/.gvm/gos/go1.25.7/bin/go test ./internal/oracleGOCACHE=/tmp/shiro-go-cache /Users/weizhenwang/.gvm/gos/go1.25.7/bin/go test ./internal/configGOCACHE=/tmp/shiro-go-cache /Users/weizhenwang/.gvm/gos/go1.25.7/bin/go test ./cmd/shiro