You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iggy.apache.org by "chengxilo (via GitHub)" <gi...@apache.org> on 2025/07/10 19:13:54 UTC
[PR] refactor(sdk): change int attribute to int32 and refactor identifier. [iggy]
chengxilo opened a new pull request, #1995:
URL: https://github.com/apache/iggy/pull/1995
1. refactor iggcon.Identifier, to enhance type safe.
2. change int => uint32 to align with rust sdk and avoid overflow.
--
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@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(sdk): change int attribute to int32 and refactor identifier. [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on PR #1995:
URL: https://github.com/apache/iggy/pull/1995#issuecomment-3063402086
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:
https://github.com/apache/iggy/blob/09f608fe961df351b24231fdc32605b2d8124bdd/core/common/src/types/identifier/mod.rs#L211-L213
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.
--
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@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(sdk): change int attribute to int32 and refactor identifier. [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1995:
URL: https://github.com/apache/iggy/pull/1995#discussion_r2201516141
##########
foreign/go/binary_serialization/identifier_serializer.go:
##########
@@ -18,31 +18,14 @@
package binaryserialization
import (
- "encoding/binary"
-
iggcon "github.com/apache/iggy/foreign/go/contracts"
)
-const (
- idKindOffset = 0
- idLengthOffset = 1
- stringIdLength = 2
- numericIdLength = 4
-)
-
func SerializeIdentifier(identifier iggcon.Identifier) []byte {
- bytes := make([]byte, int(identifier.Length)+2)
- bytes[idKindOffset] = byte(identifier.Kind)
- bytes[idLengthOffset] = byte(identifier.Length)
-
- switch identifier.Kind {
- case iggcon.StringId:
- valAsString := identifier.Value.(string)
- copy(bytes[stringIdLength:], []byte(valAsString))
- case iggcon.NumericId:
- valAsInt := identifier.Value.(uint32)
- binary.LittleEndian.PutUint32(bytes[stringIdLength:stringIdLength+numericIdLength], valAsInt)
- }
+ bytes := make([]byte, identifier.Length+2)
+ bytes[0] = byte(identifier.Kind)
+ bytes[1] = byte(identifier.Length)
+ copy(bytes[2:], identifier.Value)
return bytes
}
Review Comment:
Since the attribute Identifier.Value was changed to []byte. I re-write the serialize function. I deleted those constant because I think the code we have now is already clear enough and the constant name is a little bit confusing.
--
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@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(sdk): change int attribute to int32 and refactor identifier. [iggy]
Posted by "haze518 (via GitHub)" <gi...@apache.org>.
haze518 commented on code in PR #1995:
URL: https://github.com/apache/iggy/pull/1995#discussion_r2202211714
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
+ if value == 0 {
+ return Identifier{}, ierror.InvalidIdentifier
}
+ return Identifier{
+ Kind: NumericId,
+ Length: 4,
+ Value: value,
+ }, nil
+}
+// NewStringIdentifier creates a new identifier from the given string value.
+func NewStringIdentifier(value string) (Identifier, error) {
+ length := len(value)
+ if length == 0 || length > 255 {
+ return Identifier{}, ierror.InvalidIdentifier
+ }
return Identifier{
- Kind: kind,
- Length: length,
- Value: id,
+ Kind: StringId,
Review Comment:
ok, so let's stick with it
--
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@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(sdk): change int attribute to int32 and refactor identifier. [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1995:
URL: https://github.com/apache/iggy/pull/1995#discussion_r2201253065
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
+ if value == 0 {
+ return Identifier{}, ierror.InvalidIdentifier
}
+ return Identifier{
+ Kind: NumericId,
+ Length: 4,
+ Value: value,
+ }, nil
+}
+// NewStringIdentifier creates a new identifier from the given string value.
+func NewStringIdentifier(value string) (Identifier, error) {
+ length := len(value)
+ if length == 0 || length > 255 {
+ return Identifier{}, ierror.InvalidIdentifier
+ }
return Identifier{
- Kind: kind,
- Length: length,
- Value: id,
+ Kind: StringId,
Review Comment:
In rust sdk , they use as_code to map the enum to its actual code.
```rust
pub fn as_code(&self) -> u8 {
match self {
IdKind::Numeric => 1,
IdKind::String => 2,
}
}
```
However, Go doesn't have real enums; instead, we typically use constants. So why not use it like constant.
```golang
type IdKind int // I will change it to uint8 later, might be better for understanding.
const (
NumericId IdKind = 1
StringId IdKind = 2
)
```
--
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@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(sdk): change int attribute to int32 and refactor identifier. [iggy]
Posted by "spetz (via GitHub)" <gi...@apache.org>.
spetz merged PR #1995:
URL: https://github.com/apache/iggy/pull/1995
--
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@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(sdk): change int attribute to int32 and refactor identifier. [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1995:
URL: https://github.com/apache/iggy/pull/1995#discussion_r2198529745
##########
foreign/go/contracts/identifier.go:
##########
Review Comment:
I choose to use two seperate function to create new Identifier with Numeric and String. This make sure that user would not use int, int32, uint8 etc as the value of identifier and casue some problem.
--
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@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(sdk): change int attribute to int32 and refactor identifier. [iggy]
Posted by "haze518 (via GitHub)" <gi...@apache.org>.
haze518 commented on code in PR #1995:
URL: https://github.com/apache/iggy/pull/1995#discussion_r2200425737
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
+ if value == 0 {
+ return Identifier{}, ierror.InvalidIdentifier
}
+ return Identifier{
+ Kind: NumericId,
+ Length: 4,
+ Value: value,
+ }, nil
+}
+// NewStringIdentifier creates a new identifier from the given string value.
+func NewStringIdentifier(value string) (Identifier, error) {
+ length := len(value)
+ if length == 0 || length > 255 {
+ return Identifier{}, ierror.InvalidIdentifier
+ }
return Identifier{
- Kind: kind,
- Length: length,
- Value: id,
+ Kind: StringId,
+ Length: len(value),
+ Value: value,
+ }, nil
+}
+
+// GetU32Value returns the numeric value of the identifier.
+func (id Identifier) GetU32Value() (uint32, error) {
+ if id.Kind != NumericId || id.Length != 4 {
+ return 0, ierror.ResourceNotFound
}
+
+ return id.Value.(uint32), nil
Review Comment:
I don't really like that value is of type any, because there’s a risk of panic in this place if someone constructs an Identifier incorrectly
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
+ if value == 0 {
+ return Identifier{}, ierror.InvalidIdentifier
}
+ return Identifier{
+ Kind: NumericId,
+ Length: 4,
+ Value: value,
+ }, nil
+}
+// NewStringIdentifier creates a new identifier from the given string value.
+func NewStringIdentifier(value string) (Identifier, error) {
+ length := len(value)
+ if length == 0 || length > 255 {
+ return Identifier{}, ierror.InvalidIdentifier
+ }
return Identifier{
- Kind: kind,
- Length: length,
- Value: id,
+ Kind: StringId,
Review Comment:
By the way, what do you think - maybe it makes sense to switch the enum definition to use iota, since that’s the most common way to define such constants?
```go
type IdKind int
const (
NumericId IdKind = iota
StringId
)
```
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
+ if value == 0 {
+ return Identifier{}, ierror.InvalidIdentifier
}
+ return Identifier{
+ Kind: NumericId,
+ Length: 4,
+ Value: value,
+ }, nil
+}
+// NewStringIdentifier creates a new identifier from the given string value.
+func NewStringIdentifier(value string) (Identifier, error) {
+ length := len(value)
+ if length == 0 || length > 255 {
+ return Identifier{}, ierror.InvalidIdentifier
+ }
return Identifier{
- Kind: kind,
- Length: length,
- Value: id,
+ Kind: StringId,
+ Length: len(value),
+ Value: value,
+ }, nil
+}
+
+// GetU32Value returns the numeric value of the identifier.
+func (id Identifier) GetU32Value() (uint32, error) {
+ if id.Kind != NumericId || id.Length != 4 {
+ return 0, ierror.ResourceNotFound
}
+
+ return id.Value.(uint32), nil
Review Comment:
What do you think about changing the value field from any to []byte, just like it’s currently done in rs
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
+ if value == 0 {
+ return Identifier{}, ierror.InvalidIdentifier
}
+ return Identifier{
+ Kind: NumericId,
+ Length: 4,
+ Value: value,
+ }, nil
+}
+// NewStringIdentifier creates a new identifier from the given string value.
+func NewStringIdentifier(value string) (Identifier, error) {
+ length := len(value)
+ if length == 0 || length > 255 {
+ return Identifier{}, ierror.InvalidIdentifier
+ }
return Identifier{
- Kind: kind,
- Length: length,
- Value: id,
+ Kind: StringId,
+ Length: len(value),
+ Value: value,
+ }, nil
+}
+
+// GetU32Value returns the numeric value of the identifier.
+func (id Identifier) GetU32Value() (uint32, error) {
+ if id.Kind != NumericId || id.Length != 4 {
+ return 0, ierror.ResourceNotFound
}
+
+ return id.Value.(uint32), nil
+}
+
+// GetStringValue returns the string value of the identifier.
+func (id Identifier) GetStringValue() (string, error) {
+ if id.Kind != StringId {
+ return "", ierror.InvalidIdentifier
+ }
+
+ return id.Value.(string), nil
Review Comment:
and here via string(id.Value)
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
Review Comment:
It seems that the init function could be implemented using generics, what do you think?
For example, something like this (and if needed, the logic can be moved to helper functions):
```go
type IdentifierInput interface {
~uint32 | ~string
}
func NewIdentifier[T IdentifierInput](val T) (Identifier, error) {
switch any(val).(type) {
case uint32:
if val.(uint32) == 0 {
return Identifier{}, ErrInvalidIdentifier
}
num := val.(uint32)
bytes := make([]byte, 4)
binary.LittleEndian.PutUint32(bytes, num)
return Identifier{
Kind: NumericId,
Length: 4,
Value: bytes,
}, nil
case string:
s := val.(string)
n := len(s)
if n == 0 || n > 255 {
return Identifier{}, ErrInvalidIdentifier
}
return Identifier{
Kind: StringId,
Length: n,
Value: []byte(s),
}, nil
default:
return Identifier{}, ErrInvalidIdentifier
}
}
```
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
+ if value == 0 {
+ return Identifier{}, ierror.InvalidIdentifier
}
+ return Identifier{
+ Kind: NumericId,
+ Length: 4,
+ Value: value,
+ }, nil
+}
+// NewStringIdentifier creates a new identifier from the given string value.
+func NewStringIdentifier(value string) (Identifier, error) {
+ length := len(value)
+ if length == 0 || length > 255 {
+ return Identifier{}, ierror.InvalidIdentifier
+ }
return Identifier{
- Kind: kind,
- Length: length,
- Value: id,
+ Kind: StringId,
+ Length: len(value),
+ Value: value,
+ }, nil
+}
+
+// GetU32Value returns the numeric value of the identifier.
+func (id Identifier) GetU32Value() (uint32, error) {
+ if id.Kind != NumericId || id.Length != 4 {
+ return 0, ierror.ResourceNotFound
}
+
+ return id.Value.(uint32), nil
Review Comment:
In that case, we could safely extract the value like this: binary.LittleEndian.Uint32(id.Value)
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
+ if value == 0 {
+ return Identifier{}, ierror.InvalidIdentifier
}
+ return Identifier{
+ Kind: NumericId,
+ Length: 4,
+ Value: value,
+ }, nil
+}
+// NewStringIdentifier creates a new identifier from the given string value.
+func NewStringIdentifier(value string) (Identifier, error) {
+ length := len(value)
+ if length == 0 || length > 255 {
+ return Identifier{}, ierror.InvalidIdentifier
+ }
return Identifier{
- Kind: kind,
- Length: length,
- Value: id,
+ Kind: StringId,
+ Length: len(value),
+ Value: value,
+ }, nil
+}
+
+// GetU32Value returns the numeric value of the identifier.
+func (id Identifier) GetU32Value() (uint32, error) {
Review Comment:
In go, such methods are usually implemented without the get prefix. You can simply name them Uint32 or StringValue
--
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@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(sdk): change int attribute to int32 and refactor identifier. [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1995:
URL: https://github.com/apache/iggy/pull/1995#discussion_r2201170917
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
Review Comment:
Will change my code like this. But I am not going to use `~uint32 | ~string`. Instead, I will use `uint32 | string`.
There is some explaination about the potential problem with ~xxxx in generic.
https://go.googlesource.com/proposal/+/master/design/43651-type-parameters.md#identifying-the-matched-predeclared-type
I also have a small example here:
```go
package main
import "fmt"
type MyUint32 uint32
func TPrint[T ~uint32 | ~string](val T) {
switch any(val).(type) {
case uint32:
fmt.Printf("uint32: %v", val)
case float64:
fmt.Printf("float64: %v", val)
default:
fmt.Printf("other: %v", val)
}
}
func main() {
TPrint(MyUint32(42))
}
```
But overall it's a good idea.
I tried to use the generic in the Identifier struct. However, I gave up since it would not compile because methods are not allowed to have generic type params. Using generic in this function can be a good choice.
--
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@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(sdk): change int attribute to int32 and refactor identifier. [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1995:
URL: https://github.com/apache/iggy/pull/1995#discussion_r2201431388
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
+ if value == 0 {
+ return Identifier{}, ierror.InvalidIdentifier
}
+ return Identifier{
+ Kind: NumericId,
+ Length: 4,
+ Value: value,
+ }, nil
+}
+// NewStringIdentifier creates a new identifier from the given string value.
+func NewStringIdentifier(value string) (Identifier, error) {
+ length := len(value)
+ if length == 0 || length > 255 {
+ return Identifier{}, ierror.InvalidIdentifier
+ }
return Identifier{
- Kind: kind,
- Length: length,
- Value: id,
+ Kind: StringId,
+ Length: len(value),
+ Value: value,
+ }, nil
+}
+
+// GetU32Value returns the numeric value of the identifier.
+func (id Identifier) GetU32Value() (uint32, error) {
+ if id.Kind != NumericId || id.Length != 4 {
+ return 0, ierror.ResourceNotFound
}
+
+ return id.Value.(uint32), nil
Review Comment:
It does make sense. I will change it to byte slice.
--
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@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(sdk): change int attribute to int32 and refactor identifier. [iggy]
Posted by "haze518 (via GitHub)" <gi...@apache.org>.
haze518 commented on code in PR #1995:
URL: https://github.com/apache/iggy/pull/1995#discussion_r2202211461
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
Review Comment:
yeah, now it’s clear thanx. I agree with you - let’s stick to your option
--
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@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org