Skip to content

Conversation

@shivendra-dev54
Copy link
Contributor

@shivendra-dev54 shivendra-dev54 commented Dec 16, 2025

Fixes #809

Reason for this PR

The graphar::Type enum is part of public APIs and is also used in
serialization / layout-sensitive code paths. Previously, its underlying
type was implicit, which makes the width platform-dependent.

This PR makes the underlying type explicit so that it is stable and
well-defined across platforms.

What changes are included in this PR?

  • Update graphar::Type to use an explicit int32_t underlying type
  • Align the forward declaration in fwd.h with the enum definition

No behavior changes are intended.

Testing

  • Code review and static inspection
  • Local CMake configure succeeded
  • Full build was not completed locally due to Arrow Acero/ORC header
    availability issues in the conda-forge Arrow version
  • Relying on CI for full build and integration testing

Are there any user-facing changes?

No. This change is internal and does not affect file formats or API behavior.

@yangxk1
Copy link
Contributor

yangxk1 commented Dec 17, 2025

https://github.com/apache/incubator-graphar/actions/runs/20261430580/job/58288535539
Please remember to execute

cmake ..
make graphar-clformat
make graphar-cpplint

format code

@codecov-commenter
Copy link

codecov-commenter commented Dec 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.82%. Comparing base (bb4a1d3) to head (b8de338).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #811      +/-   ##
============================================
+ Coverage     76.74%   76.82%   +0.08%     
  Complexity      593      593              
============================================
  Files            84       85       +1     
  Lines          8883     8825      -58     
  Branches       1042     1040       -2     
============================================
- Hits           6817     6780      -37     
+ Misses         1836     1815      -21     
  Partials        230      230              
Flag Coverage Δ
cpp 71.52% <ø> (+0.06%) ⬆️

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.

@shivendra-dev54
Copy link
Contributor Author

I’ve run clang-format and cpplint locally and pushed the formatting fixes. Thanks for the guidance!

@shivendra-dev54
Copy link
Contributor Author

I’ve run clang-format and cpplint locally and pushed the formatting fixes.
I’m unable to run clang-format-8 locally on Fedora due to tool availability, so I’m relying on CI for the exact formatting check.

@shivendra-dev54
Copy link
Contributor Author

I’ve updated the code and reformatted it using clang-format-8 (via Docker) to match the CI environment. Thanks for your patience.

@Sober7135
Copy link
Contributor

any updates on this PR?

My upcoming Rust bindings will rely on the enums in fwd.h, so I’m tracking this closely. I also have a few suggestions to share:

  1. Could we also specify an explicit underlying type for the other enums in fwd.h (e.g., Cardinality, FileType) for consistency?
  2. Would you consider renaming the PR to something more specific, e.g. feat(c++): specify underlying types for enums in fwd.h? The current title feels a bit general.
  3. I think we should align on a convention for underlying types (int32_t vs uint32_t vs something else). My preference is uint32_t since these enums are never negative.
  4. For AdjListType, should we switch the underlying type to uint32_t (or int32_t) to match the agreed convention? I tend to use 32-bit width for consistency. Using uint8_t does not seem to yield significant benefits.

Thoughts?

CC @yangxk1 @shivendra-dev54

@shivendra-dev54
Copy link
Contributor Author

Thanks for the suggestions! I’ll take a closer look and follow up in the comments shortly.

@shivendra-dev54 shivendra-dev54 changed the title feat(c++): Use fixed-width int32_t instead of platform-dependent int for binary layout consistency feat(c++): specify underlying types for enums in fwd.h Dec 20, 2025
@shivendra-dev54
Copy link
Contributor Author

I have added int32_t types for all the enums in fwd.h which are Cardinality, FileType, SelectType and GetChunkVersion.
also updated type of AdjListType to int32_t for consistency.
Before pushing those changes i want to know weather we are going with the int32_t or uint32_t ?
My pick is also uint32_t so what is the final call ? @yangxk1 @Sober7135

@shivendra-dev54
Copy link
Contributor Author

I have pushed changes with uint32_t as the type

@yangxk1
Copy link
Contributor

yangxk1 commented Dec 25, 2025

I think these are enough. If we find there is anything else that needs to be changed, please sync them in the issue #809.

Copy link
Contributor

@yangxk1 yangxk1 left a comment

Choose a reason for hiding this comment

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

Thank You~ @shivendra-dev54

@yangxk1
Copy link
Contributor

yangxk1 commented Dec 25, 2025

cc @Sober7135

@yangxk1 yangxk1 merged commit 2381640 into apache:main Dec 25, 2025
6 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.

feat(c++): Use fixed-width int32_t instead of platform-dependent int for binary layout consistency

4 participants