You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@graphar.apache.org by "shivendra-dev54 (via GitHub)" <gi...@apache.org> on 2025/12/16 08:30:19 UTC
[PR] feat(c++): Use fixed-width int32_t instead of platform-dependent int for binary layout consistency [incubator-graphar]
shivendra-dev54 opened a new pull request, #811:
URL: https://github.com/apache/incubator-graphar/pull/811
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] feat(c++): Use fixed-width int32_t instead of platform-dependent int for binary layout consistency [incubator-graphar]
Posted by "shivendra-dev54 (via GitHub)" <gi...@apache.org>.
shivendra-dev54 commented on PR #811:
URL: https://github.com/apache/incubator-graphar/pull/811#issuecomment-3664552809
I’ve run clang-format and cpplint locally and pushed the formatting fixes. Thanks for the guidance!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] feat(c++): Use fixed-width int32_t instead of platform-dependent int for binary layout consistency [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on PR #811:
URL: https://github.com/apache/incubator-graphar/pull/811#issuecomment-3664452157
https://github.com/apache/incubator-graphar/actions/runs/20261430580/job/58288535539
Please remember to execute
```
cmake ..
make graphar-clformat
make graphar-cpplint
```
format code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] feat(c++): specify underlying types for enums in fwd.h [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on PR #811:
URL: https://github.com/apache/incubator-graphar/pull/811#issuecomment-3691345915
I think these are enough. If we find there is anything else that needs to be changed, please sync them in the issue #809.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] feat(c++): specify underlying types for enums in fwd.h [incubator-graphar]
Posted by "shivendra-dev54 (via GitHub)" <gi...@apache.org>.
shivendra-dev54 commented on PR #811:
URL: https://github.com/apache/incubator-graphar/pull/811#issuecomment-3677480666
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] feat(c++): specify underlying types for enums in fwd.h [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 merged PR #811:
URL: https://github.com/apache/incubator-graphar/pull/811
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] feat(c++): Use fixed-width int32_t instead of platform-dependent int for binary layout consistency [incubator-graphar]
Posted by "Sober7135 (via GitHub)" <gi...@apache.org>.
Sober7135 commented on PR #811:
URL: https://github.com/apache/incubator-graphar/pull/811#issuecomment-3674139910
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] feat(c++): specify underlying types for enums in fwd.h [incubator-graphar]
Posted by "shivendra-dev54 (via GitHub)" <gi...@apache.org>.
shivendra-dev54 commented on PR #811:
URL: https://github.com/apache/incubator-graphar/pull/811#issuecomment-3681975900
I have pushed changes with `uint32_t` as the type
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] feat(c++): Use fixed-width int32_t instead of platform-dependent int for binary layout consistency [incubator-graphar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #811:
URL: https://github.com/apache/incubator-graphar/pull/811#issuecomment-3664469738
## [Codecov](https://app.codecov.io/gh/apache/incubator-graphar/pull/811?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 76.75%. Comparing base ([`bb4a1d3`](https://app.codecov.io/gh/apache/incubator-graphar/commit/bb4a1d335c9f8f50a534ccf08432b101aa493ae9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`2928858`](https://app.codecov.io/gh/apache/incubator-graphar/commit/29288580fd7ed2487b1a6f0883f32a837051a6c0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #811 +/- ##
============================================
+ Coverage 76.74% 76.75% +0.01%
Complexity 593 593
============================================
Files 84 85 +1
Lines 8883 8887 +4
Branches 1042 1042
============================================
+ Hits 6817 6821 +4
Misses 1836 1836
Partials 230 230
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-graphar/pull/811?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] feat(c++): Use fixed-width int32_t instead of platform-dependent int for binary layout consistency [incubator-graphar]
Posted by "shivendra-dev54 (via GitHub)" <gi...@apache.org>.
shivendra-dev54 commented on PR #811:
URL: https://github.com/apache/incubator-graphar/pull/811#issuecomment-3664823160
I’ve updated the code and reformatted it using clang-format-8 (via Docker) to match the CI environment. Thanks for your patience.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] feat(c++): Use fixed-width int32_t instead of platform-dependent int for binary layout consistency [incubator-graphar]
Posted by "shivendra-dev54 (via GitHub)" <gi...@apache.org>.
shivendra-dev54 commented on PR #811:
URL: https://github.com/apache/incubator-graphar/pull/811#issuecomment-3664604591
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] feat(c++): Use fixed-width int32_t instead of platform-dependent int for binary layout consistency [incubator-graphar]
Posted by "shivendra-dev54 (via GitHub)" <gi...@apache.org>.
shivendra-dev54 commented on PR #811:
URL: https://github.com/apache/incubator-graphar/pull/811#issuecomment-3675197283
Thanks for the suggestions! I’ll take a closer look and follow up in the comments shortly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org
Re: [PR] feat(c++): specify underlying types for enums in fwd.h [incubator-graphar]
Posted by "yangxk1 (via GitHub)" <gi...@apache.org>.
yangxk1 commented on PR #811:
URL: https://github.com/apache/incubator-graphar/pull/811#issuecomment-3691346786
cc @Sober7135
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org