search

apache / iceberg-rust

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

Enhancement: refine the reader interface #401

Closed ZENOTME closed 1 week ago

ZENOTME commented 2 weeks ago

This PR is a draft for #398. If it looks good, I will fill out the test later. The basic here is to move the info needed for the reader to FileScanTask. In this way, we can avoid the inconsistency between the reader and FileScanTask and it's more friendly for the user to use the reader.

To store the predicate in FileScanTask, this PR also makes the predicate serializable and deserializable.

liurenjie1024 commented 2 weeks ago

Sorry for late reply, let's discuss here:https://github.com/apache/iceberg-rust/issues/398

sdd commented 2 weeks ago

Can we at least split the code that makes Predicate serializable out of this PR into it's own PR? I think that is pretty uncontroversial and very useful on its own.

liurenjie1024 commented 2 weeks ago

Can we at least split the code that makes Predicate serializable out of this PR into it's own PR? I think that is pretty uncontroversial and very useful on its own.

I agree with @sdd about this and suggest to split it into three prs:

  1. Ser/de of datum
  2. Ser/de of expression
  3. Refine arrow builder

We can still keep this pr open and waiting for 1 and 2 to get merged.

ZENOTME commented 2 weeks ago

Can we at least split the code that makes Predicate serializable out of this PR into it's own PR? I think that is pretty uncontroversial and very useful on its own.

I agree with @sdd about this and suggest to split it into three prs:

  1. Ser/de of datum
  2. Ser/de of expression
  3. Refine arrow builder

We can still keep this pr open and waiting for 1 and 2 to get merged.

I separate a PR for 1 and 2 #406 because I find that 2 is dependent on 1. Let's complete it first.

ZENOTME commented 1 week ago

After #406 is complete, let's move on this.

liurenjie1024 commented 1 week ago

cc @sdd Do you have other concerns?

sdd commented 1 week ago

No more concerns - looks good, thanks! In fact I've been already using this in some prototypes in anticipation of it getting merged, it works well 🙌🏼