zeroshade commented on code in PR #932:
URL: https://github.com/apache/iceberg-go/pull/932#discussion_r3196864349
##########
table/substrait/substrait.go:
##########
@@ -170,6 +171,11 @@ func (convertToSubstrait) VisitUnknown() types.Type {
return nil
}
+func (convertToSubstrait) VisitVariant() types.Type {
+ // Variant has no Substrait equivalent
+ return nil
+}
Review Comment:
let's make a note that for now this means that the scanner can't perform any
filtering/predicates with variant columns. We're going to likely need to add a
user-defined type in arrow-go/arrow/compute that we can register with
substrait, and then add support in the compute libraries for variant.
That being said, when we add shredding support that will be viable for
predicates since the filtering/predicate would happen on the shredded and typed
column as opposed to the variant column directly.
##########
literals.go:
##########
@@ -1389,3 +1392,39 @@ func (d *DecimalLiteral) UnmarshalBinary(data []byte)
error {
return nil
}
+
+type VariantLiteral struct {
+ val variant.Value
+}
+
+func (VariantLiteral) Comparator() Comparator[variant.Value] {
+ return func(v1, v2 variant.Value) int {
+ panic("variant values are not comparable")
+ }
+}
+
+func (VariantLiteral) Type() Type { return VariantType{} }
+func (v VariantLiteral) Value() variant.Value { return v.val }
+func (v VariantLiteral) Any() any { return v.val }
+func (v VariantLiteral) String() string { return "variant(...)" }
+
+func (v VariantLiteral) MarshalBinary() ([]byte, error) {
+ return v.val.Bytes(), nil
+}
+
+func (v VariantLiteral) To(typ Type) (Literal, error) {
+ if _, ok := typ.(VariantType); ok {
+ return v, nil
+ }
+
+ return nil, fmt.Errorf("%w: VariantLiteral to %s", ErrBadCast, typ)
+}
Review Comment:
this is fine for now, but we should leave a note to improve this by getting
the actual value of the variant (using `.Type()`) and seeing if we can attempt
to convert or returning an error if it can't.
##########
literals.go:
##########
@@ -1389,3 +1392,39 @@ func (d *DecimalLiteral) UnmarshalBinary(data []byte)
error {
return nil
}
+
+type VariantLiteral struct {
+ val variant.Value
+}
+
+func (VariantLiteral) Comparator() Comparator[variant.Value] {
+ return func(v1, v2 variant.Value) int {
+ panic("variant values are not comparable")
+ }
+}
+
+func (VariantLiteral) Type() Type { return VariantType{} }
+func (v VariantLiteral) Value() variant.Value { return v.val }
+func (v VariantLiteral) Any() any { return v.val }
+func (v VariantLiteral) String() string { return "variant(...)" }
+
+func (v VariantLiteral) MarshalBinary() ([]byte, error) {
+ return v.val.Bytes(), nil
+}
+
+func (v VariantLiteral) To(typ Type) (Literal, error) {
+ if _, ok := typ.(VariantType); ok {
+ return v, nil
+ }
+
+ return nil, fmt.Errorf("%w: VariantLiteral to %s", ErrBadCast, typ)
+}
+
+func (v VariantLiteral) Equals(other Literal) bool {
+ rhs, ok := other.(VariantLiteral)
+ if !ok {
+ return false
+ }
+
+ return bytes.Equal(v.val.Bytes(), rhs.val.Bytes())
Review Comment:
hmm this isn't technically correct. We need to also compare the metadata
don't we? We should probably add an Equals method in the `parquet/variant`
package honestly.
##########
literals.go:
##########
@@ -1389,3 +1392,39 @@ func (d *DecimalLiteral) UnmarshalBinary(data []byte)
error {
return nil
}
+
+type VariantLiteral struct {
+ val variant.Value
+}
Review Comment:
```suggestion
type VariantLiteral variant.Value
```
Let's follow the same pattern as we do for the other literals
##########
literals.go:
##########
@@ -1389,3 +1392,39 @@ func (d *DecimalLiteral) UnmarshalBinary(data []byte)
error {
return nil
}
+
+type VariantLiteral struct {
+ val variant.Value
+}
+
+func (VariantLiteral) Comparator() Comparator[variant.Value] {
+ return func(v1, v2 variant.Value) int {
+ panic("variant values are not comparable")
+ }
+}
+
+func (VariantLiteral) Type() Type { return VariantType{} }
+func (v VariantLiteral) Value() variant.Value { return v.val }
+func (v VariantLiteral) Any() any { return v.val }
+func (v VariantLiteral) String() string { return "variant(...)" }
Review Comment:
`variant.Value` has a `String()` method, let's use that. It creates a JSON
representation of the value as a string.
--
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]
