-
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
expression: do not set ParseToJSONFlag to a JSON column #8564
Conversation
/run-all-tests |
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-common-test tidb-test=pr/676 |
/run-common-test |
expression/builtin_compare.go
Outdated
// Moreover, Column.RetType refers to the infoschema, if we modify | ||
// it, data race may happen if another goroutine read from the | ||
// infoschema at the same time. | ||
if _, isColumn := args[i].(*Column); isColumn { |
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.
Can we extract a common function to do it? There are a lot of duplicated code and comments.
/run-common-test tidb-test=pr/676 |
/run-all-tests |
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
What problem does this PR solve?
Do not set ParseToJSONFlag to a JSON column which can avoid the data race when another goroutine tries to read the column's flag.
What is changed and how it works?
ParseToJSONFlag is used to effect the behavior of castStringAsJSON.
If ParseToJSONFlag is set, we invoke
json.ParseBinaryFromString(str)
to parse this string to a JSON, wherestr
must be a valid JSON-string (e.g. '"abc"').Or, we use json.CreateBinary(str) to wrap this string as a JSON directly without check.
ParseToJSONFlag is introduced to resolve the compatibility of compare function and JSON-related function with MySQL.
e.g.:
For a JSON column, we do not need to set this flag for it.
Check List
Tests
Code changes
Side effects
N/A
Related changes
release-2.1 release-2.0
This change is