mbutrovich commented on PR #91:
URL:
https://github.com/apache/datafusion-iceberg/pull/91#issuecomment-6512557697
> This PR should likely be updated to address the other side (unless we just
want to move forward with #59, but doing this separately works)
The old code didn't handle the literal side correctly either. It stripped
the cast, so `ts >= timestamp '2023-01-05T00:00:00'` became `ts >=
Datum::string("2023-01-05T00:00:00")` (the assertion this PR deletes from
`test_predicate_conversion_with_cast`), and Iceberg parsed the string at bind
time. #59 evaluates the cast with `ScalarValue::cast_to` in `literal_value`,
which is what I'd want for the literal side. Through SQL this case doesn't come
up: the optimizer folds constant casts before pushdown, as the `EXPLAIN` output
in this PR's sqllogictest shows (`TimestampNanosecond(1672920000000000000,
None)` and `Utf8("5")`). Only direct callers of `convert_filters_to_predicate`
see the difference.
Should we merge this PR with the `isnan` arm removed, and have #59 rebase
onto it for literal-side casts and #44?
--
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]
