Skip to content

Conversation

@czy006
Copy link
Contributor

@czy006 czy006 commented May 19, 2025

Include ResourceSerde interface and impl with SimpleSerialize/KryoSerialize/JavaSerialize
Their serialization/deserialization behavior was originally consistent with SequenceUtil

Before close #3361
Close #3356.

  • Split the SequenceUtil code to make it more concise
  • Add ResourceSerde interface for serialize and deserialize
  • SimpleSerialize/KryoSerialize/JavaSerializer impl for ResourceSerde

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before making a pull request

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not documented)

@czy006 czy006 requested a review from klion26 May 19, 2025 03:51
@czy006 czy006 changed the title [AMORO-3356] Include ResourceSerde interface and impl with SimpleSerialize and KryoSerialize [AMORO-3356] Include ResourceSerde interface and impl with Simple/Kryo/Java Serialize May 19, 2025
@czy006 czy006 force-pushed the issues/amoro-3356 branch from 066f412 to a2f774c Compare May 19, 2025 09:19
@czy006 czy006 requested a review from xxubai May 19, 2025 09:22
@codecov-commenter
Copy link

codecov-commenter commented May 19, 2025

Codecov Report

Attention: Patch coverage is 8.33333% with 66 lines in your changes missing coverage. Please review.

Project coverage is 28.27%. Comparing base (58c6013) to head (3bf4687).
Report is 32 commits behind head on master.

Files with missing lines Patch % Lines
...org/apache/amoro/serialization/KryoSerializer.java 0.00% 34 Missing ⚠️
...g/apache/amoro/serialization/SimpleSerializer.java 0.00% 15 Missing ⚠️
...org/apache/amoro/serialization/JavaSerializer.java 0.00% 8 Missing ⚠️
...java/org/apache/amoro/utils/SerializationUtil.java 0.00% 8 Missing ⚠️
...org/apache/amoro/utils/map/SimpleSpillableMap.java 85.71% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3571      +/-   ##
============================================
+ Coverage     21.76%   28.27%   +6.50%     
- Complexity     2391     3703    +1312     
============================================
  Files           431      617     +186     
  Lines         40501    49779    +9278     
  Branches       5745     6432     +687     
============================================
+ Hits           8816    14073    +5257     
- Misses        30938    34698    +3760     
- Partials        747     1008     +261     
Flag Coverage Δ
core 28.27% <8.33%> (?)
trino ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@czy006 czy006 requested a review from xxubai May 21, 2025 01:50
Copy link
Contributor

@xxubai xxubai left a comment

Choose a reason for hiding this comment

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

Have a good shape. Some small notes for you

@czy006 czy006 requested a review from xxubai May 28, 2025 08:48
@xxubai xxubai merged commit 7ff304c into apache:master May 29, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Subtask]: Include ResourceSerde interface and impl with SimpleSerialize and KryoSerialize

3 participants