-
Notifications
You must be signed in to change notification settings - Fork 490
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
add hexLit
and binaryList
support for LOAD DATA
#365
Conversation
Codecov Report
@@ Coverage Diff @@
## master #365 +/- ##
=======================================
Coverage 50.44% 50.44%
=======================================
Files 31 31
Lines 6918 6918
=======================================
Hits 3490 3490
Misses 3069 3069
Partials 359 359
Continue to review full report at Codecov.
|
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
parser.y
Outdated
} | ||
| hexLit | ||
{ | ||
$$ = ast.NewValueExpr($1).GetString() |
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.
Could we use NewHexLiteral directly here?
Lines 2035 to 2039 in 3b36f86
// NewHexLiteral creates a types.HexLiteral value, it's provided by parser driver. | |
var NewHexLiteral func(string) (interface{}, error) | |
// NewBitLiteral creates a types.BitLiteral value, it's provided by parser driver. | |
var NewBitLiteral func(string) (interface{}, error) |
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.
ok
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.
But NewHexLiteral
will return a n interface. type assert may be very complex
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.
🤔 Could we change the return type from interface{}
to some real interface?
type BinaryLiteral interface {
ToString() string
}
var NewHexLiteral func(string) (BinaryLiteral, error)
var NewBitLiteral func(string) (BinaryLiteral, error)
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 a good idea.
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.
but it works will in TiDB😂
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.
Maybe try updating go.mod1
and go.sum1
? 😂
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.
@kennytm how to upgrade go.mod
in parser😂
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.
@imtbkcat Try to rename go.mod1
and go.sum1
to go.mod
and go.sum
, then do the regular update, then rename them back to go.mod1
and go.sum1
😂
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.
@kennytm fixed, PTAL
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
* support hex and bin for terminator * add more tests * add test for binary * adddress comment * add ast.BinaryLiteral * go fmt project * address comment * fix parser * update gomod
* support hex and bin for terminator * add more tests * add test for binary * adddress comment * add ast.BinaryLiteral * go fmt project * address comment * fix parser * update gomod
What problem does this PR solve?
TiDB cannot support
LOAD DATA
sql which fields terminated by hex expression.What is changed and how it works?
Add
hexLit
andbinaryLit
forFieldTerminator
, and convert hex expression to string during parsing.After:
Check List
Tests