zeroshade commented on code in PR #1645:
URL: https://github.com/apache/iceberg-go/pull/1645#discussion_r3732154695
##########
table/internal/utils.go:
##########
@@ -538,8 +538,7 @@ func TruncateUpperBoundText(s string, trunc int) string {
result := []rune(s)[:trunc]
for i := len(result) - 1; i >= 0; i-- {
- next := result[i] + 1
- if utf8.ValidRune(next) {
+ if next, ok := nextValidRune(result[i]); ok {
result[i] = next
return string(result)
Review Comment:
Non-blocking: after carrying past one or more trailing U+10FFFF runes,
consider returning `string(result[:i+1])` here. Retaining the trailing maximum
runes produces a safe but looser upper bound; returning through the rune that
was actually incremented gives the tighter prefix bound.
##########
table/internal/utils_test.go:
##########
@@ -67,6 +67,7 @@ func TestMetricsModePairs(t *testing.T) {
func TestTruncateUpperBoundString(t *testing.T) {
assert.Equal(t, "ab", internal.TruncateUpperBoundText("aaaa", 2))
+ assert.Equal(t, "\uE000", internal.TruncateUpperBoundText("\uD7FFx", 1))
Review Comment:
Non-blocking: consider asserting `utf8.ValidString` and bytewise ordering
for the returned bound, since those are the two correctness properties behind
this regression. Neighboring scalar values and a preceding-rune carry case
would round out the boundary coverage.
--
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]
