-
Notifications
You must be signed in to change notification settings - Fork 22
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
DBZ-8043 Fix FLOAT32 value conversions. #95
Conversation
This mapping is needed to read the FLOAT32 value from spanner's response. This was missed in debezium#92.
4514002
to
6554367
Compare
This is a continuation of debezium#92. This IT does not support verifying the float32 values as it is dependent on a change in Cloud Spanner Emulator, and it should be available in the next few emulator releases. This is a test-only issue; running this against production should pass the assertion.
82fdc91
to
7c521aa
Compare
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 from my PoV, deferring to @nancyxu123 for a final review.
LGTM as well. @ShuranZhang probably it's good to track the work to add support for numeric, json, float32 |
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.
@arawind LGTM, thanks! I left few minor comments, other than that this is ready to be merged.
PG dialect doesn't seem to work with numeric and jsonb types. float4 support in the emulator is WIP.
@arawind Applied, thanks! Let's solve the pg issue in separate Jira/PR. |
FieldJsonNodeValueMapper changes are needed to read the FLOAT32 value from spanner's response.
This PR also adds the integration tests missed in #92.
Unfortunately the IT does not support verifying the float32 values, as it is dependent on a change in Cloud Spanner Emulator. The fix is already submitted there, and it should be available in the next few emulator releases.
Note that this is a test-only issue; running this against production should pass the assertion.