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

feat!(c++): Refactor code structure to consolidate include and src into a single src directory #533

Merged
merged 9 commits into from
Jul 10, 2024

Conversation

acezen
Copy link
Contributor

@acezen acezen commented Jul 5, 2024

Reason for this PR

Our current project structure separates header files and source files into include and src directories, respectively. This approach, while beneficial in some contexts, adds complexity and overhead for smaller projects, and not fits to protobuf generated code. To simplify the structure and enhance maintainability, we need to consolidate all code into a single src directory.

What changes are included in this PR?

Here are some notable changes:

  • Consolidate the header and source code into single src directory.
  • Add subdirectory to classify the arrow and high-level related code.
  • Add api headers to let users to easily to include the function of c++ library.

Are these changes tested?

Yes

Are there any user-facing changes?

BREAKING CHANGE:

  • the header path and header name may change, like:
    • graphar/arrow_chunk_reader.h to graphar/arrow/chunk_reader.h
    • graphar/arrow_chunk_writer.h to graphar/arrow/chunk_writer.h
  • The copy way writer function has been moved from arrow_chunk_writer.h to chunk_info_writer.h

Signed-off-by: acezen <qiaozi.zwb@alibaba-inc.com>
Signed-off-by: acezen <qiaozi.zwb@alibaba-inc.com>
Signed-off-by: acezen <qiaozi.zwb@alibaba-inc.com>
Signed-off-by: acezen <qiaozi.zwb@alibaba-inc.com>
@acezen acezen changed the title feat(c++): Refactor code structure to consolidate include and src into a single src directory !feat(c++): Refactor code structure to consolidate include and src into a single src directory Jul 5, 2024
@acezen acezen changed the title !feat(c++): Refactor code structure to consolidate include and src into a single src directory feat!(c++): Refactor code structure to consolidate include and src into a single src directory Jul 8, 2024
@acezen
Copy link
Contributor Author

acezen commented Jul 8, 2024

Hi, @lixueclaire, I create this PR to consolidate all code into a single src directory and brings some BREAKING CHANGE, may need your opinion about the changes, especially the naming of the directory and file:

  • I use high-level for our high level reader and writer directory name
  • Since we use arrow folder to separate the arrow reader and writer, so I change the arrow_chunk_reader and arrow_chunk_writer to chunk_reader and chunk_writer.
  • And also the api 's header names.

@acezen acezen marked this pull request as ready for review July 8, 2024 08:34
@acezen acezen requested a review from lixueclaire July 8, 2024 08:36
@lixueclaire
Copy link
Contributor

Hi, @lixueclaire, I create this PR to consolidate all code into a single src directory and brings some BREAKING CHANGE, may need your opinion about the changes, especially the naming of the directory and file:

  • I use high-level for our high level reader and writer directory name
  • Since we use arrow folder to separate the arrow reader and writer, so I change the arrow_chunk_reader and arrow_chunk_writer to chunk_reader and chunk_writer.
  • And also the api 's header names.

Hi, I think the APIs maybe a little confusing now. Originally, we have chunk_info_reader and arrow_chunk_reader and high-level graph collections. If we use basic_reader, it is not clear what is read. I suggest naming info_reader/meta_reader,arrow_reader/chunk_reader and high_level_reader/graph_reader.

@acezen
Copy link
Contributor Author

acezen commented Jul 9, 2024

Hi, @lixueclaire, I create this PR to consolidate all code into a single src directory and brings some BREAKING CHANGE, may need your opinion about the changes, especially the naming of the directory and file:

  • I use high-level for our high level reader and writer directory name
  • Since we use arrow folder to separate the arrow reader and writer, so I change the arrow_chunk_reader and arrow_chunk_writer to chunk_reader and chunk_writer.
  • And also the api 's header names.

Hi, I think the APIs maybe a little confusing now. Originally, we have chunk_info_reader and arrow_chunk_reader and high-level graph collections. If we use basic_reader, it is not clear what is read. I suggest naming info_reader/meta_reader,arrow_reader/chunk_reader and high_level_reader/graph_reader.

You mean we should split reader and writer to info, arrow and high_leveland also split the same level reader/writer to different directory?

@lixueclaire
Copy link
Contributor

Hi, @lixueclaire, I create this PR to consolidate all code into a single src directory and brings some BREAKING CHANGE, may need your opinion about the changes, especially the naming of the directory and file:

  • I use high-level for our high level reader and writer directory name
  • Since we use arrow folder to separate the arrow reader and writer, so I change the arrow_chunk_reader and arrow_chunk_writer to chunk_reader and chunk_writer.
  • And also the api 's header names.

Hi, I think the APIs maybe a little confusing now. Originally, we have chunk_info_reader and arrow_chunk_reader and high-level graph collections. If we use basic_reader, it is not clear what is read. I suggest naming info_reader/meta_reader,arrow_reader/chunk_reader and high_level_reader/graph_reader.

You mean we should split reader and writer to info, arrow and high_leveland also split the same level reader/writer to different directory?

I'm talking about the file names under the api directory.

… meta_writer

Signed-off-by: acezen <qiaozi.zwb@alibaba-inc.com>
@acezen
Copy link
Contributor Author

acezen commented Jul 9, 2024

Hi, @lixueclaire, I create this PR to consolidate all code into a single src directory and brings some BREAKING CHANGE, may need your opinion about the changes, especially the naming of the directory and file:

  • I use high-level for our high level reader and writer directory name
  • Since we use arrow folder to separate the arrow reader and writer, so I change the arrow_chunk_reader and arrow_chunk_writer to chunk_reader and chunk_writer.
  • And also the api 's header names.

Hi, I think the APIs maybe a little confusing now. Originally, we have chunk_info_reader and arrow_chunk_reader and high-level graph collections. If we use basic_reader, it is not clear what is read. I suggest naming info_reader/meta_reader,arrow_reader/chunk_reader and high_level_reader/graph_reader.

You mean we should split reader and writer to info, arrow and high_leveland also split the same level reader/writer to different directory?

I'm talking about the file names under the api directory.

I have changed the naming under api, could you help reviewing again?

Signed-off-by: acezen <qiaozi.zwb@alibaba-inc.com>
Copy link
Contributor

@lixueclaire lixueclaire left a comment

Choose a reason for hiding this comment

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

LGTM now. And it's essential to update our documentation accordingly.

@acezen acezen merged commit 712b204 into apache:main Jul 10, 2024
3 checks passed
This pull request was closed.
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.

feat(c++): Refactor code structure to consolidate include and src into a single src directory
2 participants