feat: add transform_literal #287
Conversation
|
@ZENOTME One question though, regarding the input: However, for #264 I'm working with This means I'd still have to expose the underlying If we don't need the extra flexibility, changing fn transform_literal(&self, input: &Datum) -> Result<Option<Datum>>would make the implementation of #264 a lot easier. Perhaps it should be named transform_datum then. Anyway, thanks again - I'd appreciate your thoughts on this. |
For now, |
I guess I can do that when implementing #264 since I need this anyway? Other than that thanks again for the PR. |
LGTM. Thanks! |
There was a problem hiding this comment.
Thanks @ZENOTME for this pr, it looks great! I found some small problems and left some comments.
crates/iceberg/src/transform/mod.rs
Outdated
| @@ -31,6 +34,8 @@ pub trait TransformFunction: Send { | |||
| /// The implementation of this function will need to check and downcast the input to specific | |||
| /// type. | |||
| fn transform(&self, input: ArrayRef) -> Result<ArrayRef>; | |||
| /// transform_literal will take an input literal and transform it into a new literal. | |||
| fn transform_literal(&self, input: &Literal) -> Result<Option<Literal>>; | |||
There was a problem hiding this comment.
I agree with @marvinlanhenke that we should accept Datum and return Datum. In theory Literal should be physical value only, while Datum is type + value. I'm thinking reducing literal enums in future.
| Literal::Primitive(PrimitiveLiteral::TimestampTZ(v)) => Ok(Some(Literal::Primitive( | ||
| PrimitiveLiteral::Int(Self::timestamp_to_year(*v)), | ||
| ))), | ||
| _ => unreachable!("Should not call internally for unsupported literal type"), |
There was a problem hiding this comment.
We should not panic here, we should return error.
| Self::truncate_str(v, len).to_string(), | ||
| )) | ||
| })), | ||
| _ => unreachable!("Truncate transform only supports (int,long,decimal,string) types"), |
| .unwrap() | ||
| .signed_duration_since(DateTime::from_timestamp(0, 0).unwrap()) | ||
| .num_days() | ||
| / 365) as i32 |
There was a problem hiding this comment.
This is incorrect, leap year maybe not 365 days. We should use NaiveDate::years_since
There was a problem hiding this comment.
Or you can use NaiveTime::year
| } | ||
|
|
||
| /// Extract a date or timestamp month, as months from 1970-01-01 | ||
| #[derive(Debug)] | ||
| pub struct Month; | ||
|
|
||
| impl Month { | ||
| #[inline] | ||
| fn timestamp_to_month(timestamp: i64) -> i32 { |
There was a problem hiding this comment.
This is incorrect, we should the month is accumulated month, see https://github.com/apache/iceberg/blob/9d42eb44a32ff4a1f86e8427fe023bde1e9652f7/api/src/test/java/org/apache/iceberg/transforms/TestDates.java#L56
ZENOTME
force-pushed
the
transform_literal
branch
from
eda5787
to
788f315
Compare
|
Thanks for your review! Have fixed it. @liurenjie1024 @marvinlanhenke |
There was a problem hiding this comment.
Thanks @ZENOTME This looks great to me! We just need to fix some unnecessary panic.
| impl Year { | ||
| #[inline] | ||
| fn timestamp_to_year(timestamp: i64) -> i32 { | ||
| DateTime::from_timestamp_micros(timestamp).unwrap().year() - UNIX_EPOCH_YEAR |
There was a problem hiding this comment.
We should not panic here. This is user input, and we should throw error.
| // unix epoch date: 1970-01-01 | ||
| // if date > unix epoch date, delta month = (aa - 1) + 12 * (aaaa-1970) | ||
| // if date < unix epoch date, delta month = (12 - (aa - 1)) + 12 * (1970-aaaa-1) | ||
| let date = DateTime::from_timestamp_micros(timestamp).unwrap(); |
There was a problem hiding this comment.
Ditto, timestamp maybe user input and we should not panic.
complete #283