laskoviymishka commented on code in PR #1196:
URL: https://github.com/apache/iceberg-go/pull/1196#discussion_r3406065731
##########
transforms.go:
##########
@@ -668,6 +679,9 @@ func (YearTransform) Apply(value Optional[Literal]) (out
Optional[Literal]) {
case TimestampLiteral:
out.Valid = true
out.Val = Int32Literal(Timestamp(v).ToTime().Year() -
epochTM.Year())
+ case TimestampNsLiteral:
Review Comment:
I think this PR makes a latent panic reachable. `transformLiteral` (further
down, the `panic("invalid literal type")` arm) has no `TimestampNsLiteral` case.
Now that `Transformer` succeeds for ns timestamps, `projectTimeTransform`
obtains a transformer and runs the literal through
`truncateNumber`/`setApplyTransform` → `transformLiteral`, which panics. So a
range predicate like `ts_col > '2024-01-01'` on a year/month-partitioned
ns-timestamp column now panics at runtime in `Transform.Project(...)` — it used
to error out before this change.
I'd add the matching case alongside the `TimestampLiteral` one:
```go
case TimestampNsLiteral:
return NewLiteral(fn(TimestampNano(l)).Val)
```
Worth doing here since this PR is what makes it reachable for Year/Month.
wdyt — fix in this PR, or land with an explicit follow-up note?
--
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]
