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

[C++] Add the basic row format serializer for C++ class types via reflection #1144

Merged
merged 7 commits into from
Nov 27, 2023

Conversation

PragmaTwice
Copy link
Member

@PragmaTwice PragmaTwice commented Nov 26, 2023

What do these changes do?

In this PR, we will include a simple row format serializer for class types.
Currently, it only supports basic field types (support for nested types will be added in future PRs).
Since the object graph protocol is not implemented in C++, we only support row format.
However, I believe the serialization framework can be easily extended to other format.

Related issue number

#1145

Check code requirements

  • tests added / passed (if needed)
  • Ensure all linting tests pass, see here for how to run them

@chaokunyang
Copy link
Collaborator

Great work! @PragmaTwice . We use encoder for row format encoding in Java and Python, and use serialization for object graph serialization. Do we need to keep consistent with Java and Python naming?

@PragmaTwice
Copy link
Member Author

Great work! @PragmaTwice . We use encoder for row format encoding in Java and Python, and use serialization for object graph serialization. Do we need to keep consistent with Java and Python naming?

Sure! Let me rename it to encoder.

@PragmaTwice PragmaTwice marked this pull request as ready for review November 26, 2023 15:08
@PragmaTwice
Copy link
Member Author

PragmaTwice commented Nov 26, 2023

Hi @chaokunyang , I think I've finished the implementation phase of this patch now.

Check row_encoder_test.cc for a simple use case.

Current only basic types are supported, and just a raw trait API is provided, I will gradually support other features according to #1145.

Feel free to review it when you are free : )

@PragmaTwice PragmaTwice changed the title [WIP] [C++] Add the basic row format serializer for C++ class types via reflection [C++] Add the basic row format serializer for C++ class types via reflection Nov 26, 2023
@chaokunyang
Copy link
Collaborator

chaokunyang commented Nov 26, 2023

@PragmaTwice, very impressive and professional work, thank you so much for submitting the CPP serialization implementation for Fury. This is a very big step for fury cpp support, it not only enhances the project but also helps the development of our community. Can't tell how excited I am.
I will do a preliminary review of the code today and provide any immediate feedback. And do a more detailed review tomorrow.
Thanks again for your contribution!

@chaokunyang
Copy link
Collaborator

Can we add a simple user doc to src/README.md and/or docs/guide/xlang_object_graph_guide.md?

chaokunyang
chaokunyang previously approved these changes Nov 27, 2023
Copy link
Collaborator

@chaokunyang chaokunyang left a comment

Choose a reason for hiding this comment

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

LGTM overall, I left some minor comments

@PragmaTwice
Copy link
Member Author

PragmaTwice commented Nov 27, 2023

Can we add a simple user doc to src/README.md and/or docs/guide/xlang_object_graph_guide.md?

I think the current interface (i.e. RowEncodeTrait) is not supposed to be used by users directly.
I am plan to add more user-friendly interface like RowEncoder in further PRs.

And maybe we can add these documentation after that.
I've added the documentation work into plan in #1145 : )

@chaokunyang
Copy link
Collaborator

chaokunyang commented Nov 27, 2023

Can we add a simple user doc to src/README.md and/or docs/guide/xlang_object_graph_guide.md?

I think the current interface (i.e. RowEncodeTrait) is not supposed to be used by users directly. I am plan to add more user-friendly interface like RowEncoder in further PRs.

And maybe we can add these documentation after that. I've added the documentation work into plan in #1145 : )

Got it

Copy link
Collaborator

@chaokunyang chaokunyang left a comment

Choose a reason for hiding this comment

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

LGTM

@chaokunyang chaokunyang merged commit cb60135 into apache:main Nov 27, 2023
15 checks passed
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.

None yet

2 participants