Skip to content

[WIP] Implement project for Transform. #264#269

Closed
marvinlanhenke wants to merge 6 commits intoapache:mainfrom
marvinlanhenke:wip_project_transform
Closed

[WIP] Implement project for Transform. #264#269
marvinlanhenke wants to merge 6 commits intoapache:mainfrom
marvinlanhenke:wip_project_transform

Conversation

@marvinlanhenke
Copy link
Contributor

@marvinlanhenke marvinlanhenke commented Mar 14, 2024 •
edited
Loading

#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.

@marvinlanhenke marvinlanhenke marked this pull request as draft March 14, 2024 19:50
@marvinlanhenke marvinlanhenke changed the title wip: prototype project_transform [WIP] Implement project for Transform. #264 Mar 14, 2024
@sdd sdd mentioned this pull request Mar 17, 2024
Closed
@marvinlanhenke
Copy link
Contributor Author

marvinlanhenke commented Mar 17, 2024

@liurenjie1024 @Xuanwo
the main design concerns I want to discuss:

  1. exposing op and literals from expressions. For prototyping I did this directly on the expression - it might be better to implement this on BoundPredicate itself and expose op via pub(crate) from the struct?
  2. Once I receive a PrimitiveLiteral I need to convert it to an arrow_array in order to perform the transformation. The conversion to the arrow array could also be implemented on PrimitiveLiteral itself?

I would really appreciate your thoughts on this

sdd
sdd reviewed Mar 17, 2024
View reviewed changes
crates/iceberg/src/spec/transform.rs Outdated
PrimitiveLiteral::Long(v) => func
.transform(Arc::new(Int64Array::from_value(v, 1)))?
.as_any()
.downcast_ref::<Int32Array>()
Copy link
Contributor

@sdd sdd Mar 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be Int64Array?

Copy link
Contributor Author

@marvinlanhenke marvinlanhenke Mar 18, 2024

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

@marvinlanhenke
Copy link
Contributor Author

marvinlanhenke commented Mar 18, 2024

@liurenjie1024 @sdd @Xuanwo @ZENOTME
I went on and implemented fn project for Transform::Bucket with some design assumptions. PTAL and tell me what you think, before we can move on and implement the missing transforms.
Thank you so much for your effort

@ZENOTME
Copy link
Contributor

ZENOTME commented Mar 18, 2024

Thanks for this job!

I'm quite unsure about the fn transform and the handling of the arrow_array - seems kinda clunky?

I think the transform can provide an interface like transform_literal later.

pub trait TransformFunction: Send {
    /// transform will take an input array and transform it into a new array.
    /// The implementation of this function will need to check and downcast the input to specific
    /// type.
    fn transform(&self, input: ArrayRef) -> Result<ArrayRef>;
+ fn transform_literal(&self, literal:Literal) -> Result<Literal>; 
}

@ZENOTME ZENOTME mentioned this pull request Mar 18, 2024
Closed
liurenjie1024
liurenjie1024 reviewed Mar 19, 2024
View reviewed changes
crates/iceberg/src/expr/predicate.rs
self.op
}

pub(crate) fn literals(&self) -> &FnvHashSet<Datum> {
Copy link
Contributor

@liurenjie1024 liurenjie1024 Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

@liurenjie1024
Copy link
Contributor

liurenjie1024 commented Mar 19, 2024

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

@ZENOTME
Copy link
Contributor

ZENOTME commented Mar 19, 2024

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.

@marvinlanhenke
Copy link
Contributor Author

marvinlanhenke commented Mar 19, 2024

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

Thank you for providing the reason why we use arrow array in the first place.

@marvinlanhenke
Copy link
Contributor Author

marvinlanhenke commented Mar 19, 2024

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.

Sounds good to me

@marvinlanhenke marvinlanhenke deleted the wip_project_transform branch April 23, 2024 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sdd sdd sdd left review comments

+1 more reviewer

@liurenjie1024 liurenjie1024 liurenjie1024 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@marvinlanhenke @ZENOTME @liurenjie1024 @sdd