From 603c5b7d93e72068a43f1955fdfea107dd71daa9 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Mon, 26 Oct 2026 16:53:26 +0100 Subject: [PATCH 1/2] test: generate the taxi fixture instead of committing it Replace the checked-in taxi table (Parquet files tracked with Git LFS) with tables generated at test time by a new unpublished crate, iceberg-datafusion-test-tools. The crate provides a FixtureCatalog, an in-memory catalog over a temp dir, in which a TableBuilder creates tables and each FixtureTable::append commits one snapshot whose data files are exactly the batches given. Data files are written with iceberg-rust's writers, so they carry real column statistics. Snapshots are committed by the crate itself so that snapshot ids count up from 1, which keeps metadata-table snapshots deterministic. create_taxi_table generates a synthetic stand-in for the old fixture with the same schema and layout: 7 days x 25,000 rows, identity partitioned on pickup_date, one row group per file, without page indexes. Rows come from rand_pcg seeded per day, so they're the same on every run and platform. IcebergTestHarness moves from the published crate's test_utils module to crates/datafusion/tests/common, which drops the serde and typetag dependencies from iceberg-datafusion. Snapshot values change with the new data; plans don't. Git LFS is no longer needed by CI. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/ci.yml | 1 - AGENTS.md | 7 +- Cargo.lock | 16 +- Cargo.toml | 2 + README.md | 2 + crates/datafusion/Cargo.toml | 3 +- crates/datafusion/src/lib.rs | 3 - crates/datafusion/src/test_utils/harness.rs | 210 ---------- crates/datafusion/src/test_utils/mod.rs | 20 - .../testdata/iceberg/taxi/README.md | 107 ----- .../pickup_date=2024-01-08/data_0.parquet | 3 - .../pickup_date=2024-01-09/data_0.parquet | 3 - .../pickup_date=2024-01-10/data_0.parquet | 3 - .../pickup_date=2024-01-11/data_0.parquet | 3 - .../pickup_date=2024-01-12/data_0.parquet | 3 - .../pickup_date=2024-01-13/data_0.parquet | 3 - .../pickup_date=2024-01-14/data_0.parquet | 3 - ...-47e0-4c4b-9522-4a7c44d74036.metadata.json | 1 - ...9fdb82-eb66-7582-99a7-9f864b92a53f-m0.avro | Bin 5141 -> 0 bytes ...-019fdb82-eb66-7582-99a7-9f864b92a53f.avro | Bin 1607 -> 0 bytes .../iceberg/taxi/metadata/v1.metadata.json | 1 - crates/datafusion/tests/common/mod.rs | 99 +++++ crates/datafusion/tests/external_table.rs | 7 +- crates/datafusion/tests/filter_pushdown.rs | 52 +-- crates/datafusion/tests/limit_pushdown.rs | 34 +- .../datafusion/tests/metadata_projection.rs | 16 +- crates/datafusion/tests/page_index.rs | 6 +- .../datafusion/tests/projection_pushdown.rs | 44 +- .../test-tools/Cargo.toml | 26 +- crates/test-tools/src/append.rs | 389 ++++++++++++++++++ crates/test-tools/src/catalog.rs | 72 ++++ crates/test-tools/src/lib.rs | 50 +++ crates/test-tools/src/table.rs | 189 +++++++++ crates/test-tools/src/taxi.rs | 321 +++++++++++++++ dev/release/README.md | 2 +- dev/release/rat_exclude_files.txt | 1 - 36 files changed, 1257 insertions(+), 445 deletions(-) delete mode 100644 crates/datafusion/src/test_utils/harness.rs delete mode 100644 crates/datafusion/src/test_utils/mod.rs delete mode 100644 crates/datafusion/testdata/iceberg/taxi/README.md delete mode 100644 crates/datafusion/testdata/iceberg/taxi/data/pickup_date=2024-01-08/data_0.parquet delete mode 100644 crates/datafusion/testdata/iceberg/taxi/data/pickup_date=2024-01-09/data_0.parquet delete mode 100644 crates/datafusion/testdata/iceberg/taxi/data/pickup_date=2024-01-10/data_0.parquet delete mode 100644 crates/datafusion/testdata/iceberg/taxi/data/pickup_date=2024-01-11/data_0.parquet delete mode 100644 crates/datafusion/testdata/iceberg/taxi/data/pickup_date=2024-01-12/data_0.parquet delete mode 100644 crates/datafusion/testdata/iceberg/taxi/data/pickup_date=2024-01-13/data_0.parquet delete mode 100644 crates/datafusion/testdata/iceberg/taxi/data/pickup_date=2024-01-14/data_0.parquet delete mode 100644 crates/datafusion/testdata/iceberg/taxi/metadata/00000-00a113a6-47e0-4c4b-9522-4a7c44d74036.metadata.json delete mode 100644 crates/datafusion/testdata/iceberg/taxi/metadata/019fdb82-eb66-7582-99a7-9f864b92a53f-m0.avro delete mode 100644 crates/datafusion/testdata/iceberg/taxi/metadata/snap-3167948105555765929-0-019fdb82-eb66-7582-99a7-9f864b92a53f.avro delete mode 100644 crates/datafusion/testdata/iceberg/taxi/metadata/v1.metadata.json create mode 100644 crates/datafusion/tests/common/mod.rs rename .gitattributes => crates/test-tools/Cargo.toml (52%) create mode 100644 crates/test-tools/src/append.rs create mode 100644 crates/test-tools/src/catalog.rs create mode 100644 crates/test-tools/src/lib.rs create mode 100644 crates/test-tools/src/table.rs create mode 100644 crates/test-tools/src/taxi.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b5cf9d6..f2b8461 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -76,7 +76,6 @@ jobs: steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - lfs: true persist-credentials: false - name: Setup Rust uses: ./.github/actions/setup-rust diff --git a/AGENTS.md b/AGENTS.md index 103ff4c..12fa4c5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -26,6 +26,7 @@ Apache DataFusion integration for Apache Iceberg: catalog/table providers and ph | `crates/datafusion` | `iceberg-datafusion` | The published library | | `crates/sqllogictest` | `iceberg-sqllogictest` | SQL-level test harness (unpublished) | | `crates/playground` | `iceberg-playground` | REPL CLI built on `datafusion-cli` (unpublished) | +| `crates/test-tools` | `iceberg-datafusion-test-tools` | Builders for the Iceberg tables tests read (unpublished) | ## Before Committing @@ -52,7 +53,11 @@ cargo insta test -p iceberg-datafusion --test integration_datafusion_test --revi cargo run -p iceberg-playground -- --rc # REPL; defaults to ~/.icebergrc ``` -Tests are self-contained (in-memory catalogs, temp dirs, checked-in metadata JSON); no Docker or external services are needed. The playground config holds `[[catalogs]]` entries with `name`, `type` (`rest` or `memory`), and a `[catalogs.config]` table of string properties. +Tests are self-contained (in-memory catalogs, temp dirs, checked-in metadata JSON); no Docker or external services are needed. + +Build test tables with `iceberg-datafusion-test-tools` rather than by hand or by committing data files. A `FixtureCatalog` is an in-memory catalog over a temp dir, not tied to any dataset. `fixtures.table(ident, schema).partition_by(..).create()` creates a table, and each `table.append().file(batch)...commit()` adds one snapshot in which every batch becomes exactly one data file. Snapshot ids count up from 1, so snapshots can show them. Data files are written with iceberg-rust's writers, not this crate's insert path, so they carry real column statistics and a writer bug can't hide a reader bug. `create_taxi_table` generates a deterministic, synthetic stand-in for NYC taxi trips: 7 days × 25,000 rows, identity-partitioned on `pickup_date`, one file per day. + +The integration tests share `IcebergTestHarness` from `crates/datafusion/tests/common/`. It's a session with `datafusion.public.taxi` registered, so `FROM taxi` resolves unqualified. The playground config holds `[[catalogs]]` entries with `name`, `type` (`rest` or `memory`), and a `[catalogs.config]` table of string properties. ## Architecture (`crates/datafusion`) diff --git a/Cargo.lock b/Cargo.lock index 5182cdf..260f9a1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3244,15 +3244,27 @@ dependencies = [ "datafusion", "futures", "iceberg", + "iceberg-datafusion-test-tools", "insta", "parquet", - "serde", "tempfile", "tokio", - "typetag", "uuid", ] +[[package]] +name = "iceberg-datafusion-test-tools" +version = "0.12.0" +dependencies = [ + "datafusion", + "futures", + "iceberg", + "rand 0.10.2", + "rand_pcg", + "tempfile", + "tokio", +] + [[package]] name = "iceberg-playground" version = "0.12.0" diff --git a/Cargo.toml b/Cargo.toml index 1bfb0aa..5307fbf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,6 +20,7 @@ members = [ "crates/datafusion", "crates/sqllogictest", "crates/playground", + "crates/test-tools", ] resolver = "2" @@ -36,6 +37,7 @@ async-trait = "0.1.89" datafusion = "55.0.0" datafusion-cli = "55.0.0" iceberg-datafusion = { path = "crates/datafusion" } +iceberg-datafusion-test-tools = { path = "crates/test-tools" } datafusion-sqllogictest = "55.0.0" iceberg = { git = "https://github.com/apache/iceberg-rust", rev = "5b82ab99aeaf50bf4f5b3b776ac9cd6380a5f3e3" } iceberg-catalog-rest = { git = "https://github.com/apache/iceberg-rust", rev = "5b82ab99aeaf50bf4f5b3b776ac9cd6380a5f3e3" } diff --git a/README.md b/README.md index 07e3aa4..f3eb60d 100644 --- a/README.md +++ b/README.md @@ -30,6 +30,8 @@ through an Iceberg catalog. - [`crates/sqllogictest`](crates/sqllogictest): SQL logic tests for the integration. - [`crates/playground`](crates/playground): a command-line SQL playground backed by DataFusion and Iceberg catalogs. +- [`crates/test-tools`](crates/test-tools): builders for the Iceberg tables the + tests read. ## Query an existing table diff --git a/crates/datafusion/Cargo.toml b/crates/datafusion/Cargo.toml index cd6edd0..a005f2c 100644 --- a/crates/datafusion/Cargo.toml +++ b/crates/datafusion/Cargo.toml @@ -38,10 +38,9 @@ iceberg = { workspace = true } parquet = "59.2" tokio = { workspace = true } uuid = { version = "1.18", features = ["v7"] } -serde = { version = "1", features = ["derive"] } -typetag = "0.2" [dev-dependencies] +iceberg-datafusion-test-tools = { workspace = true } insta = "1" parquet = "59.2" tempfile = "3.18" diff --git a/crates/datafusion/src/lib.rs b/crates/datafusion/src/lib.rs index 41cbc8b..e714176 100644 --- a/crates/datafusion/src/lib.rs +++ b/crates/datafusion/src/lib.rs @@ -28,6 +28,3 @@ pub use table_provider::{ IcebergCatalogTableProvider, IcebergStaticTableProvider, IcebergTableProviderFactory, }; pub mod write; - -#[doc(hidden)] -pub mod test_utils; diff --git a/crates/datafusion/src/test_utils/harness.rs b/crates/datafusion/src/test_utils/harness.rs deleted file mode 100644 index d106467..0000000 --- a/crates/datafusion/src/test_utils/harness.rs +++ /dev/null @@ -1,210 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -use std::collections::HashMap; -use std::path::PathBuf; -use std::sync::Arc; - -use async_trait::async_trait; -use bytes::Bytes; -use datafusion::arrow::util::pretty::pretty_format_batches; -use datafusion::dataframe::DataFrame; -use datafusion::error::Result; -use datafusion::execution::SessionStateBuilder; -use datafusion::physical_plan::displayable; -use datafusion::prelude::{SessionConfig, SessionContext}; -use futures::StreamExt; -use futures::stream::BoxStream; -use iceberg::io::{ - FileMetadata, FileRead, FileWrite, InputFile, LocalFsStorage, OutputFile, Storage, - StorageConfig, StorageFactory, -}; -use iceberg::memory::{MEMORY_CATALOG_WAREHOUSE, MemoryCatalogBuilder}; -use iceberg::{ - Catalog, CatalogBuilder, Error, ErrorKind, NamespaceIdent, Result as IcebergResult, - TableIdent, -}; -use serde::{Deserialize, Serialize}; - -use crate::{IcebergCatalogProvider, IcebergTableProviderFactory}; - -pub const FIXTURE_URI: &str = "s3://iceberg-test/warehouse/taxi"; -const WAREHOUSE_URI: &str = "s3://iceberg-test/warehouse/"; -const CATALOG_NAME: &str = "datafusion"; -const NAMESPACE_NAME: &str = "public"; - -pub struct IcebergTestHarness { - ctx: SessionContext, -} - -impl IcebergTestHarness { - pub async fn new() -> Result { - let state = SessionStateBuilder::new() - .with_default_features() - // Plan snapshots must not vary with the runner's CPU count. - .with_config(SessionConfig::new().with_target_partitions(16)) - // We register a factory in addition to the catalog below - // so that tests can be written against the factory's behaviours - .with_table_factory( - "ICEBERG".to_string(), - Arc::new(IcebergTableProviderFactory::new_with_storage_factory( - Arc::new(FixtureStorageFactory::default()), - )), - ) - .build(); - - let catalog = MemoryCatalogBuilder::default() - .with_storage_factory(Arc::new(FixtureStorageFactory::default())) - .load( - CATALOG_NAME, - HashMap::from([( - MEMORY_CATALOG_WAREHOUSE.to_string(), - WAREHOUSE_URI.to_string(), - )]), - ) - .await - .map_err(crate::to_datafusion_error)?; - - let namespace = NamespaceIdent::new(String::from(NAMESPACE_NAME)); - catalog - .create_namespace(&namespace, HashMap::from([])) - .await - .map_err(crate::to_datafusion_error)?; - - let table = TableIdent::new(namespace, String::from("taxi")); - catalog - .register_table(&table, format!("{FIXTURE_URI}/metadata/v1.metadata.json")) - .await - .map_err(crate::to_datafusion_error)?; - - let provider = IcebergCatalogProvider::try_new(Arc::new(catalog)).await?; - - let ctx = SessionContext::new_with_state(state); - ctx.register_catalog(CATALOG_NAME, Arc::new(provider)); - - Ok(Self { ctx }) - } - - pub async fn query(&self, sql: &str) -> Result<(String, String)> { - let dataframe: DataFrame = self.ctx.sql(sql).await?; - let plan = dataframe.create_physical_plan().await?; - let batches = dataframe.collect().await?; - - Ok(( - displayable(plan.as_ref()).indent(true).to_string(), - pretty_format_batches(&batches)?.to_string(), - )) - } -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -struct FixtureStorageFactory { - root: PathBuf, -} - -impl Default for FixtureStorageFactory { - fn default() -> Self { - Self { - root: PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("testdata/iceberg"), - } - } -} - -#[typetag::serde] -impl StorageFactory for FixtureStorageFactory { - fn build(&self, _config: &StorageConfig) -> IcebergResult> { - Ok(Arc::new(FixtureStorage { - root: self.root.clone(), - })) - } -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -struct FixtureStorage { - root: PathBuf, -} - -impl FixtureStorage { - fn local_path(&self, path: &str) -> IcebergResult { - let relative = path.strip_prefix(WAREHOUSE_URI).ok_or_else(|| { - Error::new( - ErrorKind::DataInvalid, - format!("unsupported fixture URI: {path}"), - ) - })?; - Ok(self.root.join(relative).display().to_string()) - } - - fn local(&self) -> LocalFsStorage { - LocalFsStorage::new() - } -} - -#[async_trait] -#[typetag::serde] -impl Storage for FixtureStorage { - async fn exists(&self, path: &str) -> IcebergResult { - self.local().exists(&self.local_path(path)?).await - } - - async fn metadata(&self, path: &str) -> IcebergResult { - self.local().metadata(&self.local_path(path)?).await - } - - async fn read(&self, path: &str) -> IcebergResult { - self.local().read(&self.local_path(path)?).await - } - - async fn reader(&self, path: &str) -> IcebergResult> { - self.local().reader(&self.local_path(path)?).await - } - - async fn write(&self, path: &str, bytes: Bytes) -> IcebergResult<()> { - self.local().write(&self.local_path(path)?, bytes).await - } - - async fn writer(&self, path: &str) -> IcebergResult> { - self.local().writer(&self.local_path(path)?).await - } - - async fn delete(&self, path: &str) -> IcebergResult<()> { - self.local().delete(&self.local_path(path)?).await - } - - async fn delete_prefix(&self, path: &str) -> IcebergResult<()> { - self.local().delete_prefix(&self.local_path(path)?).await - } - - async fn delete_stream( - &self, - paths: BoxStream<'static, String>, - ) -> IcebergResult<()> { - let mut paths = paths; - while let Some(path) = paths.next().await { - self.delete(&path).await?; - } - Ok(()) - } - - fn new_input(&self, path: &str) -> IcebergResult { - Ok(InputFile::new(Arc::new(self.clone()), path.to_string())) - } - - fn new_output(&self, path: &str) -> IcebergResult { - Ok(OutputFile::new(Arc::new(self.clone()), path.to_string())) - } -} diff --git a/crates/datafusion/src/test_utils/mod.rs b/crates/datafusion/src/test_utils/mod.rs deleted file mode 100644 index cf0ae63..0000000 --- a/crates/datafusion/src/test_utils/mod.rs +++ /dev/null @@ -1,20 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -mod harness; - -pub use harness::*; diff --git a/crates/datafusion/testdata/iceberg/taxi/README.md b/crates/datafusion/testdata/iceberg/taxi/README.md deleted file mode 100644 index fe75234..0000000 --- a/crates/datafusion/testdata/iceberg/taxi/README.md +++ /dev/null @@ -1,107 +0,0 @@ -