Skip to content
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

bugfix: serialize the type of timestamp lost nano value #1443

Merged
merged 29 commits into from
Aug 14, 2019

Conversation

l81893521
Copy link
Contributor

@l81893521 l81893521 commented Aug 11, 2019

Ⅰ. Describe what this PR did

Jackson would use DateSerializer to handle java.sql.Timestamp(Timestamp extend from Date). So when java.sql.Timestamp has nano second, the digits would lost(java.util.Date dost not support nano second)

I provide customize serializer and deserializer. When serialize java.sql.Timestamp I convert it to a long array include fastTime and nanoSecond, look like this

{"@class":"io.seata.rm.datasource.sql.struct.Field","name":"timestamp_type","keyType":"NULL","type":93,"value":["java.sql.Timestamp",[1565405366926,926554000]]}

finally use it rebuild java.sql.Timestamp when deserialize.

Ⅱ. Does this pull request fix one issue?

#1434

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

l81893521 and others added 24 commits April 29, 2019 17:23
@codecov-io
Copy link

codecov-io commented Aug 11, 2019

Codecov Report

Merging #1443 into develop will increase coverage by 0.02%.
The diff coverage is 72%.

Impacted file tree graph

@@             Coverage Diff              @@
##             develop   #1443      +/-   ##
============================================
+ Coverage      45.68%   45.7%   +0.02%     
  Complexity      1664    1664              
============================================
  Files            345     345              
  Lines          12641   12666      +25     
  Branches        1588    1589       +1     
============================================
+ Hits            5775    5789      +14     
- Misses          6220    6228       +8     
- Partials         646     649       +3
Impacted Files Coverage Δ Complexity Δ
...m/datasource/undo/parser/JacksonUndoLogParser.java 65.11% <72%> (+9.56%) 4 <0> (ø) ⬇️
...server/store/file/FileTransactionStoreManager.java 45.99% <0%> (-1.4%) 19% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c9261d...a6823c2. Read the comment docs.


@Override
public void serializeWithType(Timestamp timestamp, JsonGenerator gen, SerializerProvider serializers, TypeSerializer typeSerializer) throws IOException {
gen.setCurrentValue(timestamp);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to check the api to think is it necessary.

} catch (IOException e) {
LOGGER.error("deserialize java.sql.Timestamp error : {}", e.getMessage(), e);
}
Timestamp timestamp = new Timestamp(arrayNode.get(0).asLong());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be in the catch block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, thanks for correct my mistake

@slievrly slievrly changed the title Fix serialize the type of Timestamp lost digits bugfix: serialize the type of timestamp lost nano value Aug 12, 2019
Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@sharajava sharajava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okey to me

@slievrly slievrly merged commit d80ae6f into apache:develop Aug 14, 2019
@slievrly slievrly mentioned this pull request Aug 15, 2019
1 task
@l81893521 l81893521 deleted the fix_special_type_serialize branch August 15, 2019 13:48
@wangliang181230 wangliang181230 added this to the 0.8.1 milestone Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants