search

apache / iceberg-rust

Apache Iceberg
https://rust.iceberg.apache.org/
Apache License 2.0
477 stars 97 forks source link

feat: add transform_literal #287

Closed ZENOTME closed 3 months ago

ZENOTME commented 3 months ago

complete #283

ZENOTME commented 3 months ago

cc @liurenjie1024 @Xuanwo @Fokko @marvinlanhenke

marvinlanhenke commented 3 months ago

@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 

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.

ZENOTME commented 3 months ago

@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

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. So I think in here what we need is a function extract the reference of literalšŸ¤”

marvinlanhenke commented 3 months ago

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.

ZENOTME commented 3 months ago

I guess I can do that when implementing https://github.com/apache/iceberg-rust/issues/264 since I need this anyway?

LGTM. Thanks!

ZENOTME commented 3 months ago

Thanks for your review! Have fixed it. @liurenjie1024 @marvinlanhenke