Skip to content

feat(c++): Add WriterOption to allow user to configure writer option #700

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

Merged
merged 19 commits into from
Jun 27, 2025

Conversation

yangxk1
Copy link
Contributor

@yangxk1 yangxk1 commented Jun 5, 2025

Reason for this PR

This PR resolves issue #108 by providing a WriterOption API to support different file types.

What changes are included in this PR?

I've added a WriterOption class that provides different options for each file type.
To improve clarity and usability, I've separated the options for the three supported file types into three distinct classes, so users can clearly see what configurations are available for each format.
Additionally, I've created a Builder class to help users construct these options in a more intuitive and structured way.

Are these changes tested?

yes

Are there any user-facing changes?

yes, users can use WriterOptions to customize the writer option for each file type.

@yangxk1 yangxk1 changed the title Add WriterOption to allow user to configure writer option feat(c++): Add WriterOption to allow user to configure writer option Jun 6, 2025
@Thespica Thespica requested a review from Copilot June 20, 2025 16:44
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new WriterOption API to configure writer options per file type and updates multiple components (tests, file system, and chunk writers) to support these options. Key changes include:

  • Introduction of the WriterOptions class and its associated Builders for CSV, Parquet, and ORC.
  • Updates to test cases and examples to utilize the new WriterOptions.
  • Modifications in filesystem and chunk writer modules to pass and apply writer options.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
cpp/test/test_arrow_chunk_writer.cc Updated tests to use new WriterOptions API; some naming inconsistencies observed.
cpp/src/graphar/writer_util.h, .cc Added WriterOptions class and Builders to encapsulate file type options.
cpp/src/graphar/filesystem.cc Modified WriteTableToFile to accept WriterOptions and adjusted parquet write configuration.
cpp/src/graphar/arrow/chunk_writer.h, .cc Updated constructors and Make methods to support WriterOptions.
cpp/src/graphar/high-level/* Updated instantiation of writer classes to include the new WriterOptions API.
cpp/examples/mid_level_writer_example.cc Enhanced example to demonstrate usage of WriterOptions.
cli/CMakeLists.txt Added conditional definitions for ORC support.
Comments suppressed due to low confidence (2)

cpp/test/test_arrow_chunk_writer.cc:381

  • The variable name 'optionsBilder' appears to be a typo. Please rename it to 'optionsBuilder' for clarity and consistency.
    WriterOptions::Builder optionsBilder;

cpp/src/graphar/filesystem.cc:242

  • The variable name 'writer_porpertices_builder' is misspelled. Consider renaming it to 'writer_properties_builder' to accurately reflect its purpose.
        std::make_shared<parquet::WriterProperties::Builder>();

@yangxk1 yangxk1 requested a review from lixueclaire June 27, 2025 11:04
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~Thanks for your contribution!

@lixueclaire lixueclaire merged commit 961c7b8 into apache:main Jun 27, 2025
7 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.

2 participants