eneskeles commented on PR #2088: URL: https://github.com/apache/iceberg-go/pull/2088#issuecomment-5938262811
> Implements #2081 as asked; I answered the Java-parity question there. Every variant primitive is handled (int8/int16 widen to int32, decimal4/8/16, micros/nanos with and without tz, uuid, short/long strings), null/object/array/zero `Value` return `ErrBadCast`, and the zero-`Value` guard is pinned. The new tests fail against the base `literals.go`/`variant_cast.go` (22 table subtests, the tz-aware case and the error-message assertion) and pass on [7741ecc](https://github.com/apache/iceberg-go/commit/7741ecc24d38a8985a073020d6a88fbbbd75ba97); `go test .` and the `table` variant tests pass, CI is green. Extracting `decimalFromVariant` doesn't change `CastVariantLiteral`. > > Pre-existing, not blocking; I'll file these separately: > > * `DecimalLiteral.To(int32/int64)` ignores scale (literals.go:1346, 1363; literals_test.go:492 pins 12.34 → 1234), and variant decimals now reach it. > * `IsIn`/`NotIn` with two or more `VariantLiteral`s should panic in `newLiteralSet` (utils.go:135): `VariantLiteral` holds a `[]byte`, so it can't be a map key. > * arrow-go `Value()` does `unsafe.String(&v.value[5], sz)` (parquet/variant/variant.go:1103), which goes out of range for a zero-length long-form string that passes validation. > > The inline nits can land here or in a follow-up; I'll merge once the doc comment on `VariantLiteral.To` is in. Thank you for the thorough review! All three comments are addressed in c57eecb -- 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]
