badalprasadsingh commented on code in PR #2510:
URL: https://github.com/apache/iceberg-go/pull/2510#discussion_r4534236079
##########
literals.go:
##########
@@ -1095,19 +1094,14 @@ func (s StringLiteral) Equals(other Literal) bool {
return literalEq(s, other)
}
-func (s StringLiteral) MarshalBinary() (data []byte, err error) {
- // stored as UTF-8 bytes without length
- // avoid copying by just returning a slice of the raw bytes
- data = unsafe.Slice(unsafe.StringData(string(s)), len(s))
-
- return data, err
+func (s StringLiteral) MarshalBinary() ([]byte, error) {
+ // Return independent bytes so callers cannot modify the string's
storage.
+ return []byte(s), nil
Review Comment:
only the empty case in the new test pins the #2407 fix, and it checks bytes
only
please add the issue's repro as a regression test: a string identity
partition with rows "" and "us", where region = "" and region = "us" each
return one row
##########
literals.go:
##########
@@ -1095,19 +1094,14 @@ func (s StringLiteral) Equals(other Literal) bool {
return literalEq(s, other)
}
-func (s StringLiteral) MarshalBinary() (data []byte, err error) {
- // stored as UTF-8 bytes without length
- // avoid copying by just returning a slice of the raw bytes
- data = unsafe.Slice(unsafe.StringData(string(s)), len(s))
-
- return data, err
+func (s StringLiteral) MarshalBinary() ([]byte, error) {
+ // Return independent bytes so callers cannot modify the string's
storage.
+ return []byte(s), nil
}
func (s *StringLiteral) UnmarshalBinary(data []byte) error {
- // stored as UTF-8 bytes without length
- // avoid copy, but this means that the passed in slice is being given
- // to the literal for ownership
- *s = StringLiteral(unsafe.String(unsafe.SliceData(data), len(data)))
+ // The input buffer may be reused, so the decoded string must own its
bytes.
+ *s = StringLiteral(string(data))
Review Comment:
decoding now copies, so the clone in `equalityDeleteMetricValueFromBytes`
copies strings twice, and its comment ("String, binary, and fixed literals may
retain the input bytes") is out of date
better: clone only for bytes:
```go
if clone && kind == equalityDeleteRangeBytes {
// Binary and fixed literals keep the input bytes. Clone once so an
// index does not retain a borrowed metadata buffer.
data = slices.Clone(data)
}
```
##########
literals.go:
##########
@@ -1095,19 +1094,14 @@ func (s StringLiteral) Equals(other Literal) bool {
return literalEq(s, other)
}
-func (s StringLiteral) MarshalBinary() (data []byte, err error) {
- // stored as UTF-8 bytes without length
- // avoid copying by just returning a slice of the raw bytes
- data = unsafe.Slice(unsafe.StringData(string(s)), len(s))
-
- return data, err
+func (s StringLiteral) MarshalBinary() ([]byte, error) {
+ // Return independent bytes so callers cannot modify the string's
storage.
Review Comment:
nit: please say why "" must give a non-nil slice, so a later change does not
bring the nil back, and keep the spec note from the old comment:
```go
// Stored as UTF-8 bytes without length. Copy so callers cannot change the
// string, and so "" stays non-nil: a nil partition bound is written as null
(#2407).
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
