laskoviymishka commented on code in PR #1590:
URL: https://github.com/apache/iceberg-go/pull/1590#discussion_r3680750829
##########
table/substrait/substrait_test.go:
##########
@@ -137,6 +137,60 @@ func TestExprs(t *testing.T) {
}
}
+func TestNanosecondTimestampLiterals(t *testing.T) {
+ tests := []struct {
+ name string
+ fieldType iceberg.Type
+ predicate iceberg.BooleanExpression
+ wantValues []string
+ wantType string
+ }{
+ {
+ name: "timestamp equality",
+ fieldType: iceberg.PrimitiveTypes.TimestampNs,
+ predicate: iceberg.EqualTo(iceberg.Reference("ts"),
iceberg.TimestampNano(123456789)),
+ wantValues: []string{"1970-01-01 00:00:00.123456789"},
+ wantType: "precision_timestamp<9>",
+ },
+ {
+ name: "timestamp with timezone equality before
epoch",
+ fieldType: iceberg.PrimitiveTypes.TimestampTzNs,
+ predicate: iceberg.EqualTo(iceberg.Reference("ts"),
iceberg.TimestampNano(-123456789)),
+ wantValues: []string{"1969-12-31T23:59:59.876543211Z"},
+ wantType: "precision_timestamp_tz<9>",
+ },
+ {
+ name: "timestamp in",
+ fieldType: iceberg.PrimitiveTypes.TimestampNs,
+ predicate: iceberg.IsIn(iceberg.Reference("ts"),
iceberg.TimestampNano(1), iceberg.TimestampNano(1001)),
+ wantValues: []string{"1970-01-01 00:00:00.000000001",
"1970-01-01 00:00:00.000001001"},
+ wantType: "precision_timestamp<9>",
+ },
+ {
+ name: "timestamp with timezone not in",
+ fieldType: iceberg.PrimitiveTypes.TimestampTzNs,
+ predicate: iceberg.NotIn(iceberg.Reference("ts"),
iceberg.TimestampNano(-1), iceberg.TimestampNano(1001)),
+ wantValues: []string{"1969-12-31T23:59:59.999999999Z",
"1970-01-01T00:00:00.000001001Z"},
+ wantType: "precision_timestamp_tz<9>",
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ sc := iceberg.NewSchema(1, iceberg.NestedField{ID: 1,
Name: "ts", Type: tt.fieldType})
+ bound, err := iceberg.BindExpr(sc, tt.predicate, true)
+ require.NoError(t, err)
+
+ _, converted, err := substrait.ConvertExpr(sc, bound,
true)
+ require.NoError(t, err)
+ for _, value := range tt.wantValues {
+ assert.Contains(t, converted.String(), value)
Review Comment:
`assert.Contains` here is doing less than it looks — a wrong literal whose
rendering happened to contain this substring would still pass, and the
assertion goes quietly vacuous if substrait-go ever changes how it formats
`PrecisionTimestampLiteral`. `TestExprs` right above uses `assert.Equal` on the
full expression string; I'd match that here so the regression test actually
pins the value. wdyt?
##########
table/substrait/substrait_test.go:
##########
@@ -137,6 +137,60 @@ func TestExprs(t *testing.T) {
}
}
+func TestNanosecondTimestampLiterals(t *testing.T) {
Review Comment:
The original panic hit every predicate carrying a ns literal, including
range comparisons — but the cases here only cover equality and in/not-in. A
single `GreaterThan`/`LessThan` case per ns type would lock the comparison path
that was also broken. Not blocking, just cheap insurance while we're here.
##########
table/substrait/substrait.go:
##########
@@ -318,6 +318,16 @@ func toSubstraitLiteral(typ iceberg.Type, lit
iceberg.Literal) expr.Literal {
}
return toPrimitiveSubstraitLiteral(types.Timestamp(lit))
+ case iceberg.TimestampNsLiteral:
+ if typ.Equals(iceberg.PrimitiveTypes.TimestampTzNs) {
Review Comment:
This `err` can't actually be non-nil — `literal.NewPrecisionTimestamp[Tz]`
only fails on an unknown precision, and we hand it the `PrecisionNanoSeconds`
constant — so the panic is guarding a dead path.
`expr.NewPrecisionTimestampLiteral` / `NewPrecisionTimestampTzLiteral` are
already in scope and don't return an error, which lets this arm drop the var
block and mirror the two-line `TimestampLiteral` case above:
```go
case iceberg.TimestampNsLiteral:
if typ.Equals(iceberg.PrimitiveTypes.TimestampTzNs) {
return expr.NewPrecisionTimestampTzLiteral(int64(lit),
types.PrecisionNanoSeconds, types.NullabilityRequired)
}
return expr.NewPrecisionTimestampLiteral(int64(lit),
types.PrecisionNanoSeconds, types.NullabilityRequired)
```
--
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]
