NoahKusaba commented on code in PR #63:
URL: https://github.com/apache/datafusion-iceberg/pull/63#discussion_r4242557622
##########
crates/datafusion/src/read/scan.rs:
##########
@@ -130,6 +118,54 @@ impl IcebergDataSource {
}
}
+/// Builds an [`IcebergDataSource`], such as one rebuilt from another's
accessors
+/// in another process. Created by [`IcebergDataSource::builder`].
+#[derive(Debug, Clone)]
+pub struct IcebergDataSourceBuilder {
+ table: Table,
+ schema: ArrowSchemaRef,
+ snapshot_id: Option<i64>,
+ predicates: Option<Predicate>,
+ fetch: Option<usize>,
+}
+
+impl IcebergDataSourceBuilder {
+ /// Scans this snapshot, or the table's current one if `None` (the
default).
+ pub fn with_snapshot_id(mut self, snapshot_id: Option<i64>) -> Self {
+ self.snapshot_id = snapshot_id;
+ self
+ }
+
+ /// Filters the scan with `predicates`, or not at all if `None` (the
default).
+ pub fn with_predicates(mut self, predicates: Option<Predicate>) -> Self {
+ self.predicates = predicates;
+ self
+ }
+
+ /// Stops after `fetch` rows, or reads every row if `None` (the default).
+ pub fn with_fetch(mut self, fetch: Option<usize>) -> Self {
+ self.fetch = fetch;
+ self
+ }
Review Comment:
Added `with_projection(Option<&Vec<usize>>)`. It takes the indices as
`TableProvider::scan` passes them and applies them in `build()`, which now
returns a `Result` because an index can be out of range. The builder docs say
when to use it: a table provider passes the table schema plus a projection,
while a rebuild passes the source's already-projected `schema()` and no
projection.
##########
crates/datafusion/src/read/scan.rs:
##########
@@ -56,8 +56,13 @@ pub struct IcebergDataSource {
}
impl IcebergDataSource {
- /// Creates a new [`IcebergDataSource`] object.
- pub(crate) fn new(
+ /// Creates a source that scans `projection` of `schema`, with `filters`
+ /// converted to an Iceberg predicate where they can be.
+ ///
+ /// # Errors
+ ///
+ /// Returns an error if `projection` is out of bounds for `schema`.
+ pub(crate) fn try_new(
Review Comment:
Agreed, removed it. Both providers now build their scans with the builder.
So they don't have to convert filters themselves, I added
`with_filters(&[Expr])` alongside `with_predicate(Option<Predicate>)`. I also
renamed `predicates()`/`with_predicates` to the singular, since they hold a
single `Predicate`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
