zeroshade commented on code in PR #932:
URL: https://github.com/apache/iceberg-go/pull/932#discussion_r3182975341
##########
table/internal/parquet_files.go:
##########
@@ -613,7 +629,7 @@ func (p parquetFormat) DataFileStatsFromMeta(meta Metadata,
statsCols map[int]St
agg, ok := colAggs[fieldID]
if !ok {
- agg, err =
p.createStatsAgg(statsCol.IcebergTyp, stats.Type().String(), statsCol.Mode.Len)
+ agg, err =
p.createStatsAgg(statsCol.IcebergTyp.(iceberg.PrimitiveType),
stats.Type().String(), statsCol.Mode.Len)
Review Comment:
why the type assertion here?
##########
exprs.go:
##########
@@ -526,6 +528,49 @@ func (b *boundRef[T]) evalIsNull(st StructLike) bool {
return !v.Valid
}
+type boundVariantRef struct {
+ field NestedField
+ acc accessor
+}
Review Comment:
is there a reason we need this instead of being able to use
`boundRef[Variant]`?
##########
table/internal/utils.go:
##########
@@ -385,7 +385,7 @@ func MatchMetricsMode(mode string) (MetricsMode, error) {
type StatisticsCollector struct {
FieldID int
- IcebergTyp iceberg.PrimitiveType
+ IcebergTyp iceberg.Type
Review Comment:
why this change? I thought we can't do statistics for variant?
##########
exprs.go:
##########
@@ -671,6 +718,35 @@ func (bp *boundUnaryPredicate[T]) String() string {
return fmt.Sprintf("Bound%s(term=%s)", bp.op, bp.term)
}
+type boundVariantUnaryPred struct {
+ op Operation
+ term BoundTerm
+}
Review Comment:
same question as above, why can't we use the generic version?
##########
exprs.go:
##########
@@ -526,6 +528,49 @@ func (b *boundRef[T]) evalIsNull(st StructLike) bool {
return !v.Valid
}
+type boundVariantRef struct {
+ field NestedField
+ acc accessor
+}
+
+func (*boundVariantRef) isTerm() {}
+func (b *boundVariantRef) Ref() BoundReference { return b }
+func (b *boundVariantRef) Field() NestedField { return b.field }
+func (b *boundVariantRef) Type() Type { return b.field.Type }
+func (b *boundVariantRef) Pos() int { return b.acc.pos }
+func (b *boundVariantRef) Equals(other BoundTerm) bool {
+ rhs, ok := other.(*boundVariantRef)
+ if !ok {
+ return false
+ }
+
+ return b.field.Equals(rhs.field)
+}
+
+func (b *boundVariantRef) PosPath() []int {
+ out, inner := []int{b.acc.pos}, &b.acc
+ for inner.inner != nil {
+ inner = inner.inner
+ out = append(out, inner.pos)
+ }
+
+ return out
+}
+
+func (b *boundVariantRef) String() string {
+ return fmt.Sprintf("BoundReference(field=%s, accessor=%s)", b.field,
&b.acc)
+}
+
+func (b *boundVariantRef) evalToLiteral(StructLike) Optional[Literal] {
+ return Optional[Literal]{}
Review Comment:
this seems wrong, shouldn't this be using `b.acc.Get(st)` and then creating
the Literal?
##########
literals_test.go:
##########
@@ -580,6 +580,11 @@ func TestLiteralIdentityConversions(t *testing.T) {
}
}
+func TestVariantLiteralFromBytes(t *testing.T) {
+ _, err := iceberg.LiteralFromBytes(iceberg.VariantType{}, []byte{0x01,
0x00, 0x00})
+ assert.ErrorIs(t, err, iceberg.ErrType)
Review Comment:
should we also assert the error message itself?
--
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]
