-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
executor: add several sql hint related to session variables #11809
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11809 +/- ##
===========================================
Coverage 81.6107% 81.6107%
===========================================
Files 455 455
Lines 101380 101380
===========================================
Hits 82737 82737
Misses 12852 12852
Partials 5791 5791 |
PTAL. @qw4990 @lzmhhh123 |
390a337
to
3431942
Compare
PTAL. @qw4990 |
Almost LGTM, and please address comments and resolve CI problems @foreyes . |
/run-all-tests |
Address comments and add some tests. PTAL again. @qw4990 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PTAL. @XuHuaiyu |
/run-unit-test |
c.Assert(tk.Se.GetSessionVars().StmtCtx.MemTracker.CheckBytesLimit(val), IsTrue) | ||
tk.MustExec("select /*+ MEMORY_QUOTA(1 GB), MEMORY_QUOTA(1 MB) */ 1;") | ||
val = int64(1) * 1024 * 1024 | ||
c.Assert(tk.Se.GetSessionVars().StmtCtx.GetWarnings(), HasLen, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May we check the warning msg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning messages need to be dealt with specially later, and there is no need to care too much about them at this stage.
c.Assert(tk.Se.GetSessionVars().GetReplicaRead(), Equals, kv.ReplicaReadLeader) | ||
} | ||
|
||
func (s *testSessionSuite) TestStmtHints(c *C) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add some test case with hints in subqueries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unlikely to support use these hints in subqueries.
These hints need to handle before sql execute and set statement variables, handle them in subquries is too expensive.
Maybe discuss with PM team later.
address comment, add test and change a few codes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
What problem does this PR solve?
Add SQL hints:
MEMORY_QUOTA
,USE_TOJA
,NO_INDEX_MERGE
,READ_CONSISTENT_REPLICA
What is changed and how it works?
Extract sql hints before executing each query, and store hints in
StmtCtx
.When we need access or set related session variables, we have to use interfaces instead.
For example, if there is ReplicaRead hint in StmtCtx,
GetReplicaRead()
will return the value in StmtCtx, otherwise returnSessionVars.replicaRead
.Check List
Tests
Code Changes
Release Notes
Support several SQL hints related to session variables: MEMORY_QUOTA, USE_TOJA, NO_INDEX_MERGE, READ_CONSISTENT_REPLICA.