Show warnings - implementation #464
Conversation
sql/plan/showwarnings.go
Outdated
| // ShowWarnings is a node that shows the session warnings | ||
| type ShowWarnings struct { | ||
| warnings []*sql.Warning | ||
| offset uint |
There was a problem hiding this comment.
instead of implementing offset and limit here again, what do you think about making ShowWarnings return all warnings and then wrap it in Limit and Offset nodes instead, which are already implemented?
sql/parse/warnings.go
Outdated
| } | ||
| } | ||
|
|
||
| return plan.NewShowWarnings(ctx.Session.Warnings(), uint(offset), uint(count)), nil |
There was a problem hiding this comment.
You can just wrap ShowWarnings nodes in Limit and Offset nodes
sql/session.go
Outdated
| Value interface{} | ||
| // AppendWarning append a warning on to the slice | ||
| // The function implements sql.Session interface | ||
| func (s *BaseSession) AppendWarning(warn *Warning) { |
There was a problem hiding this comment.
AppendWarning is perhaps too coupled with how it's implemented (docs shouldn't specify whether it's in a slice or not. The internal implementation should not be disclosed
How about Warn or AddWarning?
|
@erizocosmico - I wrapped it by Offset/Limit (what's cool - I didn't notice that we've already got it). |
|
@kuba-- can you rebase? |
|
@erizocosmico - done. |
| @@ -32,15 +32,20 @@ type Session interface { | |||
| GetAll() map[string]TypedValue | |||
| // ID returns the unique ID of the connection. | |||
| ID() uint32 | |||
| // Warn stores the warning in the session. | |||
| Warn(warn *Warning) | |||
There was a problem hiding this comment.
should we separate this into another session interface to do not force people to implement warnings?
There was a problem hiding this comment.
type LogSession + func WithWarnings?
There was a problem hiding this comment.
If warnings is supported, it should be implemented in the session imho
| user string | ||
| mu sync.RWMutex | ||
| config map[string]TypedValue | ||
| warnings []*Warning |
There was a problem hiding this comment.
when is this slice deleted?
There was a problem hiding this comment.
It can be deleted if we have some kind Close session.
There was a problem hiding this comment.
Isn't this supposed to be a per-query thing?
There was a problem hiding this comment.
Yep, should be deleted before new query: https://mariadb.com/kb/en/library/show-warnings/
There was a problem hiding this comment.
So I believe it will be auto-cleared because in handler.go (ComQuery) we create a new context. Am I right?
There was a problem hiding this comment.
Sessions are not equal to contexts. Sessions outlive contexts. Sessions are per-connection.
There was a problem hiding this comment.
Any better idea than extending Session interface by ClearWarnings()?
There was a problem hiding this comment.
we should add ClearWarnings() and call it before any query that is not a show errors or show warnings.
sql/parse/parse.go
Outdated
| @@ -63,6 +64,11 @@ func Parse(ctx *sql.Context, query string) (sql.Node, error) { | |||
| } | |||
|
|
|||
| lowerQuery := strings.ToLower(s) | |||
| if showWarningsRegex.MatchString(lowerQuery) { | |||
There was a problem hiding this comment.
should we do this on analyzer phase using a rule?
There was a problem hiding this comment.
We can do it (hopefully no warnings will be missed), by adding a new rule to
analyzer.OnceBeforeDefault. Is it a right place?
There was a problem hiding this comment.
I think it should be on OnceAfterAll.
There was a problem hiding this comment.
ok, it also will work:+1
Signed-off-by: kuba-- <kuba@sourced.tech>
|
@ajnavarro - added |
|
@kuba-- could you rebase again please? |
Signed-off-by: kuba-- kuba@sourced.tech
This PR implements just:
SHOW WARNINGS [LIMIT [offset,] count]Note: We need to implement collect session warnings (#465)
Closes #442