zeroshade commented on code in PR #2088:
URL: https://github.com/apache/iceberg-go/pull/2088#discussion_r4159038116
##########
variant_cast.go:
##########
@@ -281,3 +278,35 @@ func literalFromCastValue(result any) Literal {
return nil
}
+
+func literalFromVariant(v variant.Value) (Literal, bool) {
+ // a zero Value has no header byte to read a type from
+ if len(v.Bytes()) == 0 {
+ return nil, false
+ }
+
+ raw := v.Value()
+ switch v.Type() {
+ case variant.Int8:
+ return Int32Literal(raw.(int8)), true
+ case variant.Int16:
+ return Int32Literal(raw.(int16)), true
+ case variant.Date:
+ return DateLiteral(raw.(arrow.Date32)), true
+ case variant.Time:
+ return TimeLiteral(raw.(arrow.Time64)), true
+ case variant.TimestampMicros, variant.TimestampMicrosNTZ:
+ return TimestampLiteral(raw.(arrow.Timestamp)), true
+ case variant.TimestampNanos, variant.TimestampNanosNTZ:
+ return TimestampNsLiteral(raw.(arrow.Timestamp)), true
Review Comment:
Tz-aware and NTZ timestamps both map to the tz-agnostic
`TimestampLiteral`/`TimestampNsLiteral`, so a tz-aware variant now converts to
`timestamp` and to `date` (UTC day), while `CastVariantLiteral` rejects a tz
mismatch (variant_cast.go:138-145, 227-231). I'm fine with that: it matches
Java's `TimestampLiteral.to`, where `TIMESTAMP` covers both and `DATE` goes
through `microsToDays`. Since the two variant paths now disagree, please
document it on `VariantLiteral.To` (literals.go:1527, outside the diff), e.g.:
```go
// To returns v for VariantType. For other types, a primitive variant is
// converted with the cast rules of the literal for its value (int8/int16
widen
// to int32, timestamps are tz-agnostic), not the stricter CastVariantLiteral
// rules. Null, object, array and zero-value variants return ErrBadCast.
```
##########
literals_test.go:
##########
@@ -1150,6 +1226,55 @@ func TestInvalidBinaryLiteralConversions(t *testing.T) {
})
}
+func TestInvalidVariantLiteralConversions(t *testing.T) {
+ // null, object and array variants hold no primitive value to convert
+ for _, v := range []any{nil, []any{int64(1)}, map[string]any{"a":
int64(1)}} {
+ testInvalidLiteralConversions(t, variantLiteralOf(t, v),
[]iceberg.Type{
+ iceberg.PrimitiveTypes.Bool,
+ iceberg.PrimitiveTypes.Int32,
+ iceberg.PrimitiveTypes.Int64,
+ iceberg.PrimitiveTypes.Float32,
+ iceberg.PrimitiveTypes.Float64,
+ iceberg.PrimitiveTypes.Date,
+ iceberg.PrimitiveTypes.Time,
+ iceberg.PrimitiveTypes.Timestamp,
+ iceberg.PrimitiveTypes.TimestampTz,
+ iceberg.DecimalTypeOf(9, 2),
+ iceberg.PrimitiveTypes.String,
+ iceberg.PrimitiveTypes.UUID,
+ iceberg.PrimitiveTypes.Binary,
+ iceberg.FixedTypeOf(2),
+ })
+ }
+
+ // a primitive variant rejects what the literal for its value rejects
+ testInvalidLiteralConversions(t, variantLiteralOf(t, true),
[]iceberg.Type{
+ iceberg.PrimitiveTypes.Int32,
+ iceberg.PrimitiveTypes.Int64,
+ iceberg.PrimitiveTypes.Float32,
+ iceberg.PrimitiveTypes.Float64,
+ iceberg.PrimitiveTypes.Date,
+ iceberg.PrimitiveTypes.Time,
+ iceberg.PrimitiveTypes.Timestamp,
+ iceberg.PrimitiveTypes.TimestampTz,
+ iceberg.DecimalTypeOf(9, 2),
+ iceberg.PrimitiveTypes.String,
+ iceberg.PrimitiveTypes.UUID,
+ iceberg.PrimitiveTypes.Binary,
+ iceberg.FixedTypeOf(2),
+ })
Review Comment:
Optional: this block re-runs `TestInvalidBoolLiteralConversions` through the
delegation. The int8 → string assertion at the end of this test already pins
that a rejected inner cast surfaces as `ErrBadCast`, so one representative row
(or none) is enough.
##########
literals.go:
##########
@@ -1529,9 +1529,17 @@ func (v VariantLiteral) To(typ Type) (Literal, error) {
return v, nil
}
- // TODO: improve by getting the actual value (using .Type()) and
attempting
- // to convert, or returning an error if it can't.
- return nil, fmt.Errorf("%w: VariantLiteral to %s", ErrBadCast, typ)
+ lit, ok := literalFromVariant(variant.Value(v))
+ if !ok {
+ return nil, fmt.Errorf("%w: VariantLiteral to %s", ErrBadCast,
typ)
+ }
+
+ out, err := lit.To(typ)
+ if err != nil {
+ return nil, fmt.Errorf("VariantLiteral: %w", err)
+ }
Review Comment:
The description says every error wraps `ErrBadCast`, but that only holds
when the inner `To` wraps it. A float/double variant cast to a decimal it
doesn't fit returns the raw `decimal128.FromFloat32`/`FromFloat64` error
(literals.go:638-640, 693-695, pre-existing), so `errors.Is(err, ErrBadCast)`
is false on this path. Either drop the claim from the description or add the
sentinel when it's missing; this keeps your `VariantLiteral:` prefix:
```suggestion
out, err := lit.To(typ)
if err != nil {
if !errors.Is(err, ErrBadCast) {
err = fmt.Errorf("%w: %w", ErrBadCast, err)
}
return nil, fmt.Errorf("VariantLiteral: %w", err)
}
```
--
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]
