-
Notifications
You must be signed in to change notification settings - Fork 189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Implement project for Transform. #264 #269
Conversation
@liurenjie1024 @Xuanwo
I would really appreciate your thoughts on this |
crates/iceberg/src/spec/transform.rs
Outdated
PrimitiveLiteral::Long(v) => func | ||
.transform(Arc::new(Int64Array::from_value(v, 1)))? | ||
.as_any() | ||
.downcast_ref::<Int32Array>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be Int64Array
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdd
if i understood correctly the fn transform
implementation of Bucket
always returns arrow_array::Int32Array
.
https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/transform/bucket.rs#L89
@liurenjie1024 @sdd @Xuanwo @ZENOTME |
Thanks for this job!
I think the transform can provide an interface like
|
self.op | ||
} | ||
|
||
pub(crate) fn literals(&self) -> &FnvHashSet<Datum> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub(crate) fn literals(&self) -> &FnvHashSet<Datum> { | |
pub(crate) fn literals(&self) -> impl Iterator<Item=Datum> { |
How about returning iterator only? It's better not expose the inner data structure.
Hi, @marvinlanhenke This pr looks great to me. But I think @ZENOTME is right, we should implement #283 first. The reason we implement transform on arrow array could be found here |
I will init the interface as soon as possible to avoid blocking this. |
Thank you for providing the reason why we use arrow array in the first place. |
Sounds good to me |
#264
I just wanted to share this as a starting point - and to get some feedback, thoughts about the approach here.
@liurenjie1024 @Xuanwo
Please take a look if this is a viable solution at all.
I'm quite unsure about the
fn transform
and the handling of the arrow_array - seems kinda clunky?Anyway, let me know what you think. best regards.