gabotechs commented on code in PR #55:
URL: https://github.com/apache/datafusion-iceberg/pull/55#discussion_r4311125179
##########
crates/datafusion/src/read/scan.rs:
##########
@@ -53,20 +67,47 @@ pub struct IcebergDataSource {
predicate: Option<Predicate>,
/// Optional limit on the number of rows to return
fetch: Option<usize>,
+ /// The data files to merge in the order the source reports, listed while
+ /// the scan was planned. `None` lists the files when the source runs and
+ /// reports no order.
+ sorted: Option<SortedTasks>,
+ metrics: ExecutionPlanMetricsSet,
+}
+
+/// Data files that all record the same sort order, and the part of that order
+/// the source reports, over the columns of its output.
+#[derive(Debug, Clone)]
+struct SortedTasks {
+ tasks: Vec<FileScanTask>,
+ ordering: LexOrdering,
}
impl IcebergDataSource {
/// Starts building a source that scans `table` and returns the columns of
/// `schema`, all of them unless
[`IcebergDataSourceBuilder::with_projection`]
/// selects some. Columns are read from Iceberg by name.
- pub fn builder(table: Table, schema: ArrowSchemaRef) ->
IcebergDataSourceBuilder {
+ ///
+ /// `sorted_tasks` are the data files to merge in the sort order they all
+ /// record, which the source then reports to DataFusion. With `None`, the
+ /// source lists its files when it runs and reports no order, as a table
+ /// provider's source does. A source rebuilt from another, such as in
+ /// another process, must be given what that source's
+ /// [`sorted_tasks`](Self::sorted_tasks) returns, because a plan built
over a
+ /// sorted source may rely on its order.
[`IcebergDataSourceBuilder::build`]
+ /// fails if the tasks don't share a sort order the source can report.
+ pub fn builder(
+ table: Table,
+ schema: ArrowSchemaRef,
+ sorted_tasks: Option<Vec<FileScanTask>>,
Review Comment:
I just saw that @andygrove actually requested this change in
https://github.com/apache/datafusion-iceberg/pull/55#pullrequestreview-5522748603.
That comment says:
> A codec written against
https://github.com/apache/datafusion-iceberg/pull/63's builder still compiles
after this PR and silently drops the order.
This applies not only to the new argument, but to every other parameter
passed through the builder. I still think we should not maintain a mix of two
different API designs as part of the public `IcebergDataSource` constructor:
either we go with the builder, or with plain arguments, but not a mix of both,
as that will not yield an ideal user experience for users building a
`IcebergDataSource` themselves.
Some options that come to mind:
1. Keep the `IcebergDataSourceBuilder` as the only API for constructing an
`IcebergDataSource`, and catch issues like the one @andygrove described at test
time.
2. Remove the `IcebergDataSourceBuilder` and just require everything as
explicit arguments, and expect that every time we want to add a new parameter
it will be a breaking change that will require a major release.
3. Expose a new `pub(crate)` constructor that explicitly takes all the
required positional arguments and use that internally everywhere, to bring back
the compile time guarantees while keeping a nice public API design resilient to
breaking changes.
--
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]
