You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ZENOTME (via GitHub)" <gi...@apache.org> on 2024/03/20 15:10:45 UTC
[PR] feat: add transform_literal [iceberg-rust]
ZENOTME opened a new pull request, #287:
URL: https://github.com/apache/iceberg-rust/pull/287
complete #283
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add transform_literal [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on PR #287:
URL: https://github.com/apache/iceberg-rust/pull/287#issuecomment-2011235235
> So I think in here what we need is a function extract the reference of literalš¤
I guess I can do that when implementing #264 since I need this anyway? Other than that thanks again for the PR.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add transform_literal [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #287:
URL: https://github.com/apache/iceberg-rust/pull/287#discussion_r1539127406
##########
crates/iceberg/src/transform/temporal.rs:
##########
@@ -51,12 +57,48 @@ impl TransformFunction for Year {
.unary(|v| v - UNIX_EPOCH_YEAR),
))
}
+
+ fn transform_literal(&self, input: &crate::spec::Datum) -> Result<Option<crate::spec::Datum>> {
+ let val = match input.literal() {
+ PrimitiveLiteral::Date(v) => Date32Type::to_naive_date(*v).year() - UNIX_EPOCH_YEAR,
+ PrimitiveLiteral::Timestamp(v) => Self::timestamp_to_year(*v),
+ PrimitiveLiteral::TimestampTZ(v) => Self::timestamp_to_year(*v),
+ _ => {
+ return Err(crate::Error::new(
+ crate::ErrorKind::FeatureUnsupported,
+ format!(
+ "Unsupported data type for year transform: {:?}",
+ input.data_type()
+ ),
+ ))
+ }
+ };
+ Ok(Some(Datum::int(val)))
+ }
}
/// 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 {
+ // date: aaaa-aa-aa
+ // 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();
+ let unix_epoch_date = DateTime::from_timestamp_micros(0).unwrap();
Review Comment:
Oh, yes, this one is always valid.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add transform_literal [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on PR #287:
URL: https://github.com/apache/iceberg-rust/pull/287#issuecomment-2018117523
Thanks for your review! Have fixed it. @liurenjie1024 @marvinlanhenke
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add transform_literal [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on PR #287:
URL: https://github.com/apache/iceberg-rust/pull/287#issuecomment-2009808470
cc @liurenjie1024 @Xuanwo @Fokko @marvinlanhenke
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add transform_literal [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #287:
URL: https://github.com/apache/iceberg-rust/pull/287#discussion_r1538988833
##########
crates/iceberg/src/transform/temporal.rs:
##########
@@ -39,6 +38,13 @@ const UNIX_EPOCH_YEAR: i32 = 1970;
#[derive(Debug)]
pub struct Year;
+impl Year {
+ #[inline]
+ fn timestamp_to_year(timestamp: i64) -> i32 {
+ DateTime::from_timestamp_micros(timestamp).unwrap().year() - UNIX_EPOCH_YEAR
Review Comment:
We should not panic here. This is user input, and we should throw error.
##########
crates/iceberg/src/transform/temporal.rs:
##########
@@ -51,12 +57,48 @@ impl TransformFunction for Year {
.unary(|v| v - UNIX_EPOCH_YEAR),
))
}
+
+ fn transform_literal(&self, input: &crate::spec::Datum) -> Result<Option<crate::spec::Datum>> {
+ let val = match input.literal() {
+ PrimitiveLiteral::Date(v) => Date32Type::to_naive_date(*v).year() - UNIX_EPOCH_YEAR,
+ PrimitiveLiteral::Timestamp(v) => Self::timestamp_to_year(*v),
+ PrimitiveLiteral::TimestampTZ(v) => Self::timestamp_to_year(*v),
+ _ => {
+ return Err(crate::Error::new(
+ crate::ErrorKind::FeatureUnsupported,
+ format!(
+ "Unsupported data type for year transform: {:?}",
+ input.data_type()
+ ),
+ ))
+ }
+ };
+ Ok(Some(Datum::int(val)))
+ }
}
/// 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 {
+ // date: aaaa-aa-aa
+ // 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();
Review Comment:
Ditto, timestamp maybe user input and we should not panic.
##########
crates/iceberg/src/transform/temporal.rs:
##########
@@ -51,12 +57,48 @@ impl TransformFunction for Year {
.unary(|v| v - UNIX_EPOCH_YEAR),
))
}
+
+ fn transform_literal(&self, input: &crate::spec::Datum) -> Result<Option<crate::spec::Datum>> {
+ let val = match input.literal() {
+ PrimitiveLiteral::Date(v) => Date32Type::to_naive_date(*v).year() - UNIX_EPOCH_YEAR,
+ PrimitiveLiteral::Timestamp(v) => Self::timestamp_to_year(*v),
+ PrimitiveLiteral::TimestampTZ(v) => Self::timestamp_to_year(*v),
+ _ => {
+ return Err(crate::Error::new(
+ crate::ErrorKind::FeatureUnsupported,
+ format!(
+ "Unsupported data type for year transform: {:?}",
+ input.data_type()
+ ),
+ ))
+ }
+ };
+ Ok(Some(Datum::int(val)))
+ }
}
/// 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 {
+ // date: aaaa-aa-aa
+ // 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();
+ let unix_epoch_date = DateTime::from_timestamp_micros(0).unwrap();
Review Comment:
Ditto.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add transform_literal [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 merged PR #287:
URL: https://github.com/apache/iceberg-rust/pull/287
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add transform_literal [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on PR #287:
URL: https://github.com/apache/iceberg-rust/pull/287#issuecomment-2011287654
> I guess I can do that when implementing https://github.com/apache/iceberg-rust/issues/264 since I need this anyway?
LGTM. Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add transform_literal [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on PR #287:
URL: https://github.com/apache/iceberg-rust/pull/287#issuecomment-2010456207
@ZENOTME
Thank you for this PR. I think I can work with that.
One question though, regarding the input: `&Literal` - I guess we do this, due to the greater flexibility?
However, for #264 I'm working with `BoundPredicates` as input.
This means I'd still have to expose the underlying `Datum` and its `PrimitiveLiteral` in order to use `fn transform_literal`. Also, when creating a new `UnboundPredicate` I'd have to construct a new `Datum` from the transformed literal.
If we don't need the extra flexibility, changing `fn transform_literal` to
```rust
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add transform_literal [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on PR #287:
URL: https://github.com/apache/iceberg-rust/pull/287#issuecomment-2011068196
> @ZENOTME Thank you for this PR. I think I can work with that.
>
> One question though, regarding the input: `&Literal` - I guess we do this, due to the greater flexibility?
>
> However, for #264 I'm working with `BoundPredicates` as input.
>
> This means I'd still have to expose the underlying `Datum` and its `PrimitiveLiteral` in order to use `fn transform_literal`. Also, when creating a new `UnboundPredicate` I'd have to construct a new `Datum` from the transformed literal.
>
> If we don't need the extra flexibility, changing `fn transform_literal` to
>
> ```rust
> 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, `transform` does not rely on the type info, so it can do transform without type info. And `transform_literal` will also be used in manifest metadata, and [manifest store the literal directly](https://github.com/liurenjie1024/iceberg-rust/blob/470e6f08e1291df2bc2ecd22ae9415851beea6ba/crates/iceberg/src/spec/manifest_list.rs#L674). So I think in here what we need is a function extract the reference of literalš¤
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add transform_literal [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #287:
URL: https://github.com/apache/iceberg-rust/pull/287#discussion_r1533295727
##########
crates/iceberg/src/transform/mod.rs:
##########
@@ -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>>;
Review 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.
##########
crates/iceberg/src/transform/temporal.rs:
##########
@@ -51,12 +61,48 @@ impl TransformFunction for Year {
.unary(|v| v - UNIX_EPOCH_YEAR),
))
}
+
+ fn transform_literal(
+ &self,
+ input: &crate::spec::Literal,
+ ) -> Result<Option<crate::spec::Literal>> {
+ match input {
+ Literal::Primitive(PrimitiveLiteral::Date(v)) => Ok(Some(Literal::Primitive(
+ PrimitiveLiteral::Int(Date32Type::to_naive_date(*v).year() - UNIX_EPOCH_YEAR),
+ ))),
+ Literal::Primitive(PrimitiveLiteral::Timestamp(v)) => Ok(Some(Literal::Primitive(
+ PrimitiveLiteral::Int(Self::timestamp_to_year(*v)),
+ ))),
+ 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"),
Review Comment:
We should not panic here, we should return error.
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -99,13 +118,44 @@ impl TransformFunction for Truncate {
.downcast_ref::<arrow_array::LargeStringArray>()
.unwrap()
.iter()
- .map(|v| v.map(|v| Self::truncate_str_by_char(v, len))),
+ .map(|v| v.map(|v| Self::truncate_str(v, len))),
);
Ok(Arc::new(res))
}
_ => unreachable!("Truncate transform only supports (int,long,decimal,string) types"),
}
}
+
+ fn transform_literal(&self, input: &Literal) -> crate::Result<Option<Literal>> {
+ match input {
+ Literal::Primitive(PrimitiveLiteral::Int(v)) => Ok(Some({
+ let width: i32 = self.width.try_into().map_err(|_| {
+ Error::new(
+ crate::ErrorKind::DataInvalid,
+ "width is failed to convert to i32 when truncate Int32Array",
+ )
+ })?;
+ Literal::Primitive(PrimitiveLiteral::Int(Self::truncate_i32(*v, width)))
+ })),
+ Literal::Primitive(PrimitiveLiteral::Long(v)) => Ok(Some({
+ let width = self.width as i64;
+ Literal::Primitive(PrimitiveLiteral::Long(Self::truncate_i64(*v, width)))
+ })),
+ Literal::Primitive(PrimitiveLiteral::Decimal(v)) => Ok(Some({
+ let width = self.width as i128;
+ Literal::Primitive(PrimitiveLiteral::Decimal(Self::truncate_decimal_i128(
+ *v, width,
+ )))
+ })),
+ Literal::Primitive(PrimitiveLiteral::String(v)) => Ok(Some({
+ let len = self.width as usize;
+ Literal::Primitive(PrimitiveLiteral::String(
+ Self::truncate_str(v, len).to_string(),
+ ))
+ })),
+ _ => unreachable!("Truncate transform only supports (int,long,decimal,string) types"),
Review Comment:
Ditto
##########
crates/iceberg/src/transform/temporal.rs:
##########
@@ -51,12 +61,48 @@ impl TransformFunction for Year {
.unary(|v| v - UNIX_EPOCH_YEAR),
))
}
+
+ fn transform_literal(
+ &self,
+ input: &crate::spec::Literal,
+ ) -> Result<Option<crate::spec::Literal>> {
+ match input {
+ Literal::Primitive(PrimitiveLiteral::Date(v)) => Ok(Some(Literal::Primitive(
+ PrimitiveLiteral::Int(Date32Type::to_naive_date(*v).year() - UNIX_EPOCH_YEAR),
+ ))),
+ Literal::Primitive(PrimitiveLiteral::Timestamp(v)) => Ok(Some(Literal::Primitive(
+ PrimitiveLiteral::Int(Self::timestamp_to_year(*v)),
+ ))),
+ 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"),
+ }
+ }
}
/// 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 {
Review 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
##########
crates/iceberg/src/transform/temporal.rs:
##########
@@ -39,6 +38,17 @@ const UNIX_EPOCH_YEAR: i32 = 1970;
#[derive(Debug)]
pub struct Year;
+impl Year {
+ #[inline]
+ fn timestamp_to_year(timestamp: i64) -> i32 {
+ (DateTime::from_timestamp_micros(timestamp)
+ .unwrap()
+ .signed_duration_since(DateTime::from_timestamp(0, 0).unwrap())
+ .num_days()
+ / 365) as i32
Review Comment:
This is incorrect, leap year maybe not 365 days. We should use `NaiveDate::years_since`
##########
crates/iceberg/src/transform/temporal.rs:
##########
@@ -39,6 +38,17 @@ const UNIX_EPOCH_YEAR: i32 = 1970;
#[derive(Debug)]
pub struct Year;
+impl Year {
+ #[inline]
+ fn timestamp_to_year(timestamp: i64) -> i32 {
+ (DateTime::from_timestamp_micros(timestamp)
+ .unwrap()
+ .signed_duration_since(DateTime::from_timestamp(0, 0).unwrap())
+ .num_days()
+ / 365) as i32
Review Comment:
Or you can use [`NaiveTime::year`](https://docs.rs/chrono/latest/chrono/trait.Datelike.html#tymethod.year)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add transform_literal [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on code in PR #287:
URL: https://github.com/apache/iceberg-rust/pull/287#discussion_r1539123455
##########
crates/iceberg/src/transform/temporal.rs:
##########
@@ -51,12 +57,48 @@ impl TransformFunction for Year {
.unary(|v| v - UNIX_EPOCH_YEAR),
))
}
+
+ fn transform_literal(&self, input: &crate::spec::Datum) -> Result<Option<crate::spec::Datum>> {
+ let val = match input.literal() {
+ PrimitiveLiteral::Date(v) => Date32Type::to_naive_date(*v).year() - UNIX_EPOCH_YEAR,
+ PrimitiveLiteral::Timestamp(v) => Self::timestamp_to_year(*v),
+ PrimitiveLiteral::TimestampTZ(v) => Self::timestamp_to_year(*v),
+ _ => {
+ return Err(crate::Error::new(
+ crate::ErrorKind::FeatureUnsupported,
+ format!(
+ "Unsupported data type for year transform: {:?}",
+ input.data_type()
+ ),
+ ))
+ }
+ };
+ Ok(Some(Datum::int(val)))
+ }
}
/// 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 {
+ // date: aaaa-aa-aa
+ // 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();
+ let unix_epoch_date = DateTime::from_timestamp_micros(0).unwrap();
Review Comment:
This should be always valid I think.š¤
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org