You are viewing a plain text version of this content. The canonical link for it is here.

Posted to github@datafusion.apache.org by "NoahKusaba (via GitHub)" <gi...@apache.org> on 2026/10/07 00:09:44 UTC

[PR] feat: add IcebergTableProvider::new_with_schema [datafusion-iceberg]

NoahKusaba opened a new pull request, #48:
URL: https://github.com/apache/datafusion-iceberg/pull/48

   ## Which issue does this PR close?
   
   - Part of the Ballista-Iceberg integration (follows #20).
   
   ## Rationale for this change
   
   A distributed engine like Ballista serializes a catalog-backed `IcebergTableProvider` on one process and rebuilds it on another, such as its scheduler. #20 exposes the provider's parts (`catalog()`, `table_ident()` and `schema()`), but the only public constructor, `try_new`, reloads the table and takes the schema the table has now. If the table's schema has changed since the original provider was built, the rebuilt provider has a different schema and plans differently.
   
   ## What changes are included in this PR?
   
   - Adds `IcebergTableProvider::new_with_schema(catalog, table_ident, schema)`. It builds a provider from those accessors' return values without loading the table, so the rebuilt provider plans exactly as the original.
   - `try_new` now loads the schema and delegates to `new_with_schema`, and its docs gain an `# Errors` section.
   
   ## Are there any user-facing changes?
   
   One new public constructor. Nothing breaks.
   
   ## Are these changes tested?
   
   - `test_provider_rebuilt_with_schema_plans_like_the_original` adds a column after the original provider is built. It then checks that both the original and the rebuilt provider return the full, unchanged `SELECT *` output, without the new column.
   - A doctest rebuilds a provider from the original's accessors.
   


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] feat: add IcebergCatalogTableProvider::new_with_schema [datafusion-iceberg]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.

andygrove commented on PR #48:
URL: https://github.com/apache/datafusion-iceberg/pull/48#issuecomment-6110865826

   > LGTM, thanks @NoahKusaba!
   > 
   > @gabotechs, I agree on moving to `insta` for consistency with DataFusion. It probably fits best in a follow-up that converts the existing `expect!` snapshots, rather than in this PR.
   > 
   > Also, the files that #18 added are missing the ASF license header, which the RAT check in #32 will flag. Could you add it to these?
   > 
   > * `crates/datafusion/src/test_utils/mod.rs`
   > * `crates/datafusion/src/test_utils/harness.rs`
   > * `crates/datafusion/tests/external_table.rs`
   > * `crates/datafusion/tests/filter_pushdown.rs`
   > * `crates/datafusion/tests/limit_pushdown.rs`
   > * `crates/datafusion/tests/projection_pushdown.rs`
   > * `crates/datafusion/testdata/iceberg/taxi/README.md`
   
   I also created a separate PR split out of #32 to add the RAT checks. https://github.com/apache/datafusion-iceberg/pull/60


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] feat: add IcebergTableProvider::new_with_schema [datafusion-iceberg]

Posted by "gabotechs (via GitHub)" <gi...@apache.org>.

gabotechs commented on code in PR #48:
URL: https://github.com/apache/datafusion-iceberg/pull/48#discussion_r4203591485


##########
crates/datafusion/src/table/mod.rs:
##########
@@ -96,11 +101,76 @@ impl IcebergTableProvider {
                 .map_err(to_datafusion_error)?,
         );
 
-        Ok(IcebergTableProvider {
+        Ok(Self::new_with_schema(catalog, table_ident, schema))
+    }
+
+    /// Creates a catalog-backed table provider with a known schema, without
+    /// loading the table.
+    ///
+    /// For rebuilding a provider elsewhere, such as on a distributed engine's
+    /// scheduler, from the parts of one built with [`Self::try_new`]. Each
+    /// argument takes what the matching accessor returns ([`Self::catalog`],
+    /// [`Self::table_ident`], and [`TableProvider::schema`]), and the rebuilt
+    /// provider then plans exactly as the original does, even after the
+    /// table's schema has changed.
+    ///
+    /// Scans read the columns named in `schema` from the table's current
+    /// snapshot. A name the table lacks fails the scan when it runs.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use std::collections::HashMap;
+    /// # use std::sync::Arc;
+    /// #
+    /// use datafusion::catalog::TableProvider;
+    /// use datafusion_iceberg::IcebergTableProvider;
+    /// # use iceberg::memory::{MEMORY_CATALOG_WAREHOUSE, MemoryCatalogBuilder};
+    /// # use iceberg::spec::{NestedField, PrimitiveType, Schema, Type};
+    /// # use iceberg::{Catalog, CatalogBuilder, NamespaceIdent, TableCreation};
+    ///
+    /// # tokio::runtime::Runtime::new()?.block_on(async {
+    /// # let warehouse = tempfile::tempdir()?;
+    /// # let props = HashMap::from([(
+    /// #     MEMORY_CATALOG_WAREHOUSE.to_string(),
+    /// #     warehouse.path().display().to_string(),
+    /// # )]);
+    /// # let catalog = MemoryCatalogBuilder::default().load("memory", props).await?;
+    /// # let namespace = NamespaceIdent::new("ns".to_string());
+    /// # catalog.create_namespace(&namespace, HashMap::new()).await?;
+    /// # let schema = Schema::builder()
+    /// #     .with_fields(vec![
+    /// #         NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(),
+    /// #     ])
+    /// #     .build()?;
+    /// # let creation = TableCreation::builder().name("t".to_string()).schema(schema).build();
+    /// # catalog.create_table(&namespace, creation).await?;
+    /// # let catalog: Arc<dyn Catalog> = Arc::new(catalog);
+    /// let original = IcebergTableProvider::try_new(catalog, namespace, "t").await?;
+    ///
+    /// // Rebuild an equivalent provider from the original's accessors alone,
+    /// // without loading the table again.
+    /// let rebuilt = IcebergTableProvider::new_with_schema(
+    ///     original.catalog().clone(),
+    ///     original.table_ident().clone(),
+    ///     original.schema(),
+    /// );
+    /// assert_eq!(rebuilt.table_ident(), original.table_ident());
+    /// assert_eq!(rebuilt.schema(), original.schema());
+    /// # Ok::<(), Box<dyn std::error::Error>>(())
+    /// # })?;
+    /// # Ok::<(), Box<dyn std::error::Error>>(())

Review Comment:
   This example here seems unreasonably long and verbose for a method that is just:
   
   ```rust
       pub fn new_with_schema(
           catalog: Arc<dyn Catalog>,
           table_ident: TableIdent,
           schema: ArrowSchemaRef,
       ) -> Self {
           IcebergTableProvider {
               catalog,
               table_ident,
               schema,
           })
       }
   ```
   
   TBH, for a public method like this, I don't think it needs more than a couple of lines of docs. An explanation of how `IcebergTableProvider` works is better given as a doc comment of the `IcebergTableProvider` struct itself, rather than a constructor method.



##########
crates/datafusion/src/table/mod.rs:
##########
@@ -997,4 +1069,55 @@ mod tests {
             )
         );
     }
+
+    /// Runs `sql` on `ctx` and renders its rows as a table.
+    async fn query(ctx: &SessionContext, sql: &str) -> String {
+        let batches = ctx.sql(sql).await.unwrap().collect().await.unwrap();
+        pretty_format_batches(&batches).unwrap().to_string()
+    }
+
+    /// A provider rebuilt from another's parts plans like the original even
+    /// after the table's schema changes: it keeps the schema the original was
+    /// built with, rather than loading the current one.
+    #[tokio::test]
+    async fn test_provider_rebuilt_with_schema_plans_like_the_original() {
+        let (catalog, namespace, table_name, _temp_dir) =
+            get_test_catalog_and_table().await;
+        let original =
+            IcebergTableProvider::try_new(catalog.clone(), namespace, table_name)
+                .await
+                .unwrap();
+        let ctx = SessionContext::new();
+        ctx.register_table("t", Arc::new(original.clone())).unwrap();
+        query(&ctx, "INSERT INTO t VALUES (1, 'a')").await;
+
+        // The schema changes after the original provider was built.
+        let table = catalog.load_table(original.table_ident()).await.unwrap();
+        let tx = Transaction::new(&table);
+        let tx = tx
+            .update_schema()
+            .add_column(AddColumn::optional(
+                "email",
+                Type::Primitive(PrimitiveType::String),
+            ))
+            .apply(tx)
+            .unwrap();
+        tx.commit(catalog.as_ref()).await.unwrap();
+
+        let rebuilt = IcebergTableProvider::new_with_schema(
+            original.catalog().clone(),
+            original.table_ident().clone(),
+            original.schema(),
+        );
+
+        let rebuilt_ctx = SessionContext::new();
+        rebuilt_ctx.register_table("t", Arc::new(rebuilt)).unwrap();
+        let expected = "+----+------+\n\
+                        | id | name |\n\
+                        +----+------+\n\
+                        | 1  | a    |\n\
+                        +----+------+";
+        assert_eq!(query(&rebuilt_ctx, "SELECT * FROM t").await, expected);
+        assert_eq!(query(&ctx, "SELECT * FROM t").await, expected);

Review Comment:
   We should not do these kind of expectations here, prefer using `insta` so that expectations over ascii graphics are automatically managed.



-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] feat: add IcebergCatalogTableProvider::new_with_schema [datafusion-iceberg]

Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.

mbutrovich commented on PR #48:
URL: https://github.com/apache/datafusion-iceberg/pull/48#issuecomment-6147665074

   There both seems to be eagerness to merge this, and concern about public API.
   
   > I don't love the fact that the public API of this project makes incorrect state representable.
   
   Is it in scope for this PR, or do we need to file issues to follow up? I don't want this to get lost and am catching up on PRs on this repo.


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org