Skip to content

refactor(sdk): change int attribute to int32 and refactor identifier.#1995

Merged
spetz merged 14 commits into
apache:masterfrom
chengxilo:refactor-identifier
Jul 12, 2025
Merged

refactor(sdk): change int attribute to int32 and refactor identifier.#1995
spetz merged 14 commits into
apache:masterfrom
chengxilo:refactor-identifier

Conversation

@chengxilo
Copy link
Copy Markdown
Contributor

  1. refactor iggcon.Identifier, to enhance type safe.
  2. change int => uint32 to align with rust sdk and avoid overflow.

Comment thread foreign/go/contracts/identifier.go
@chengxilo chengxilo marked this pull request as ready for review July 10, 2025 20:17
Comment thread foreign/go/contracts/identifier.go Outdated
Comment thread foreign/go/contracts/identifier.go Outdated
Comment thread foreign/go/contracts/identifier.go Outdated
Comment thread foreign/go/contracts/identifier.go Outdated
Comment thread foreign/go/contracts/identifier.go
Comment thread foreign/go/binary_serialization/identifier_serializer.go
@chengxilo
Copy link
Copy Markdown
Contributor Author

I didn't change the value type of Length in this PR, I will explain why it should be addressed in the future. For now, I’ve left it as int to ensure the Go SDK can still compile.

In the future, we need to provide a function like this to Identifier:

fn get_size_bytes(&self) -> IggyByteSize {
IggyByteSize::from(u64::from(self.length) + 2)
}

The return type should be uint64, similar to how IggyByteSize is implemented in the Rust SDK:

https://github.com/apache/iggy/blob/master/core/common/src/utils/byte_size.rs

The entire byte size calculation logic—not just for Identifier—needs to be updated. Currently, we calculate all byte sizes using int, but uint64 would be more appropriate (because rust sdk use uint64). That said, this change would require touching thousands of lines of code across the project, so I believe it’s best handled in a separate PR.

@chengxilo chengxilo requested a review from haze518 July 11, 2025 18:58
@thepatrykk thepatrykk self-assigned this Jul 12, 2025
@thepatrykk thepatrykk self-requested a review July 12, 2025 16:58
Copy link
Copy Markdown

@thepatrykk thepatrykk left a comment

Choose a reason for hiding this comment

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

LGTM

@spetz spetz merged commit 9758c90 into apache:master Jul 12, 2025
23 checks passed
@chengxilo chengxilo deleted the refactor-identifier branch July 12, 2025 20:23
T1B0 pushed a commit to T1B0/iggy that referenced this pull request Jul 16, 2025
…apache#1995)

1. refactor iggcon.Identifier, to enhance type safe.
2. change int => uint32 to align with rust sdk and avoid overflow.
hageshiame pushed a commit to hageshiame/iggy that referenced this pull request Nov 7, 2025
…apache#1995)

1. refactor iggcon.Identifier, to enhance type safe.
2. change int => uint32 to align with rust sdk and avoid overflow.
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.

4 participants