-
Notifications
You must be signed in to change notification settings - Fork 289
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
Avro supports handle key(s) as Kafka key, and other miscellaneous improvements #862
Conversation
8269b05
to
c6af3da
Compare
/run-all-tests |
/run-all-tests |
/run-all-tests |
/run-all-tests |
Codecov Report
@@ Coverage Diff @@
## master #862 +/- ##
===========================================
Coverage 34.9028% 34.9028%
===========================================
Files 97 97
Lines 11681 11681
===========================================
Hits 4077 4077
Misses 7195 7195
Partials 409 409 |
cdc/sink/codec/avro.go
Outdated
if v, ok := col.Value.(uint64); ok { | ||
col.Value = int64(v) | ||
} |
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.
This conversion will lose precision, does Avro have no support for uint64?
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.
The implementation of Avro only supports sign integers, and passing unsigned types will cause panics. Avro should support unsigned integers in theory.
/run-all-tests |
1 similar comment
/run-all-tests |
/run-all-tests |
1 similar comment
/run-all-tests |
/run-all-tests |
/run-all-tests |
/run-integration-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
@@ -55,113 +55,130 @@ const ( | |||
MultipleKeyFlag | |||
// NullableFlag means the column is nullable | |||
NullableFlag | |||
// UnsignedFlag means the column stores an unsigned integer | |||
UnsignedFlag |
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.
Don't forget to add this flag to document
/merge |
/run-all-tests |
What problem does this PR solve?
Check List
Tests
Release note