Repository navigation
Merge upstream/main: 62 commits (Contextual Sessions auth refactor, reader metadata refactor, bloom-filter/row-group perf, deletion-vector validation) - #84
Merged
Conversation
* feat(spec): add unknown primitive type support Co-authored-by: Codex <codex@openai.com> * fix(arrow): validate row ID synthesis inputs * fix(spec): centralize unknown field validation * fix(spec): address unknown type review feedback * fix(spec): address remaining review feedback * fix(spec): centralize dropped partition source handling * fix(inspect): recover historical partition bounds * fix(spec): validate default partition sources on metadata load * Revert "fix(inspect): recover historical partition bounds" This reverts commit 6403d48. --------- Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Kevin Liu <kevinjqliu@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* ci: make merge queue checks reliable Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * ci: keep website checks optional Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
) * fix(cache): drop schema-id from the manifest-list cache key ObjectCache::get_manifest_list built the cache key from snapshot.schema_id().unwrap(), but schema-id is optional on snapshots in every table version (v1-v3 per spec, and already read as Option<SchemaId>), so a snapshot that omits it panicked on every scan (the object cache is on by default). schema-id does not belong in this key: ManifestListReader::load parses the manifest list from its bytes and the table format version only and never reads schema-id, and the manifest-list location is already unique per snapshot. * Fix import * Harden test
* feat(transaction): support transform-based sort orders ReplaceSortOrderAction only supported sorting by a column's raw value: `asc`/`desc` hardcoded Transform::Identity and only accepted a column name. Java's SortOrderBuilder.asc/desc accept a Term, which can be a transform expression (bucket[N], year, truncate[W], ...). Add asc_with_transform / desc_with_transform. asc/desc are unchanged (same signature and behavior) and now delegate to the new methods with Transform::Identity, so no existing caller is affected. Transform-compatibility with the source column's type is checked at commit time via the existing SortOrder::builder().build(schema) -> check_compatibility path, matching the timing of Java's SortOrder.Builder.build() -- the spec-level type was already general (SortField.transform, check_compatibility already validate arbitrary transforms); only the transaction-layer API was narrow. Scope: this only extends the metadata-declaration API. Whether the write path sorts data to match a table's declared sort order -- for any transform, including the pre-existing identity case -- is a separate, pre-existing gap untouched here. Closes apache#2764 * test(transaction): metadata.json round-trip test for transform sort orders Per review: commit a transform-based sort order through the in-crate memory catalog (which serializes the updated metadata to a metadata.json file via TableMetadata::write_to), then reload the table (TableMetadata::read_from parses that file) and assert the transforms survive the JSON round-trip. * docs(transaction): note intended Term convergence; hoist test imports Per review: document on asc_with_transform/desc_with_transform that once Term becomes transform-carrying (apache#2665), sort-order declaration is expected to converge on Term-based asc/desc (as in Java's SortOrderBuilder) and the _with_transform variants can be deprecated. Also hoist the use statements scattered inside test fns to the test module top for consistency. * fix(transaction): validate sort transforms and preserve input errors * chore: update public API snapshot for sort transforms * fix(sort): use consistent validation errors and document void policy
…pache#3254) The eq, in, and starts_with row-group metric checks each read both the lower and upper bound via min_value and max_value, and each of those independently re-runs stats_and_type_for_field_id. Resolve the stats and type once per leaf and decode each bound in place, so the lower and upper bounds still decode lazily.
* Introduce invalid data error macro * fix * comments * fix * fix --------- Co-authored-by: blackmwk <257669749+blackmwk@users.noreply.github.com>
* rfc only * update rfc * make it more readable * update rfc * update rfc * remove default impl for cleanup * box self
…che#3259) * fix(cache): drop format-version from the manifest-list cache key The manifest-list cache key now uses the location alone; format_version is no longer part of it. ManifestList::parse_with_version(bytes, version) is a pure function of its inputs, the bytes are immutable and uniquely identified by their location, and an ObjectCache is bound to a single table. So format_version can never disambiguate two lookups at the same location. The only case where it could differ for one cache is an in-place format upgrade (v1 -> v2 mid-cache-lifetime). Dropping it is more correct there: the cache serves the original correct v1 parse instead of a miss that would reparse the v1 bytes under v2. Adds test_get_manifest_list_ignores_format_version, asserting two lookups at one location under different format versions return the same cached Arc. Follow-up to the review thread on apache#3235: apache#3235 (comment) * PR feedback
…pache#3236) * feat(encryption) [18N] wire in ags1 file length for tamper proofing * remove debug * expect * fix test * fix opendal * make more clear * Add oversized tests * don't do unreachable * import fix * Assert length in streaming tests * DRAFT try key_metadata_with_length * fix(encryption): address key metadata length review
* ci: replace MinIO with RustFS for integration tests `quay.io/minio/minio` no longer allows anonymous pulls, so `make docker-up` fails and the integration tests cannot start. DataFusion hit the same failure and switched to RustFS in apache/datafusion#25706. PyIceberg moved to RustFS earlier in apache/iceberg-python#3928. Run `rustfs/rustfs:1.0.0` as the shared S3 service instead. It keeps the `admin`/`password` credentials, the 9000/9001 ports, and virtual-hosted-style access through `RUSTFS_SERVER_DOMAINS` and the bucket aliases. The healthcheck uses `/health/ready`, which waits for storage and IAM. The `mc` bucket setup becomes `curl --aws-sigv4` run from the RustFS image, and the public bucket policy is dropped because no test reads anonymously. Point the REST fixture, HMS, and Spark at `http://rustfs:9000`, and rename the MinIO-specific test helpers and the `ICEBERG_TEST_MINIO_ENDPOINT` override to RustFS. Closes apache#3272 * ci: use vendor-neutral object-store naming for S3 test service Rename the RustFS service, hostname, bucket aliases, test helpers and env var to a generic object-store name (matching PyIceberg), so future backend swaps only need an image change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * ci: fail fast and gate readiness on test bucket creation Treat any non-200/409 response as a failure instead of ignoring it, and add a healthcheck so `docker compose up --wait` blocks until the buckets exist. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * ci: remove orphan containers on docker-up Renaming the minio service leaves stale containers holding port 9000 for anyone with a previously started stack; clean them up automatically. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * ci: make create-buckets a one-shot job RustFS returns 200 when re-creating an existing bucket, so plain chained curl -f calls are idempotent. Replace the status-code parsing, marker-file healthcheck and tail with a one-shot container, and gate spark-iceberg on its successful completion. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: ovoievodin <o_voievodin@apple.com> Co-authored-by: Kevin Liu <kevin.jq.liu@gmail.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* fix(spec): format pre-epoch timestamptz values * fix(spec): strengthen pre-epoch timestamptz regression * style: format temporal conversion * test(spec): match chrono fractional timestamp display * test(spec): match trimmed timestamp fraction
…3247) * fix(expr): decode BYTE_ARRAY page-index bounds by field type The page index evaluator decoded every BYTE_ARRAY min/max bound as a UTF-8 string via String::from_utf8(..).unwrap(), panicking during scan pruning whenever a binary (or byte-encoded decimal) column carried a predicate. Convert the bound according to the field's primitive type instead: string, binary, or decimal. * PR feedback * Round2 feedback * Fix trace
* feat(writer): add PositionDeleteInput builder Add PositionDeleteInputBuilder, a small helper that assembles a spec-conforming position delete RecordBatch from (path, pos) rows. The batch has exactly the two required position delete columns (file_path: Utf8, pos: Int64), both non-null and carrying the reserved field ids, so it passes PositionDeleteFileWriter's per-batch validation and can be handed straight to that writer. An empty builder still produces a valid 0-row batch. To keep a single source of truth for the field-id wiring, the Arrow schema is no longer hand-wired in the builder. This commit un-gates the previously test-only POSITION_DELETE_ARROW_SCHEMA static and position_delete_arrow_schema() helper in position_delete_writer.rs, making the helper pub(crate), and the builder now obtains the shared Arrow projection through it as a cheap Arc clone. extend accepts any iterator of (Into<String>, i64) rows, and build debug-asserts the paths/positions length invariant. The module docs note that rows are materialized into two heap Vecs before the batch is built (so very large inputs should use one builder per write batch), that the optional third row column is not supported yet, and that position delete files are a v2 construct which v3 forbids, so a format-version gate must be applied at the transaction/commit layer. Relates to apache#340 and apache#2218. * refactor(writer): replace PositionDeleteInput builder with a sorted position_delete_batch fn Per review, collapse the PositionDeleteInputBuilder surface (new/with_capacity/push/extend/len/is_empty/build) into a single free function, position_delete_batch(rows). A FooBuilder is expected to build a Foo, but the builder's build() only returned a bare RecordBatch, so a plain function reads more clearly and keeps the API as tight as possible. The function collects rows into a BTreeMap<String, RoaringTreemap> keyed by file path. This deduplicates repeated (file_path, pos) pairs and, when iterated, yields the spec-required ascending (file_path, pos) order for free, so callers no longer have to pre-sort their input. A negative pos is rejected with DataInvalid. The batch is still built from the writer's shared crate internal Arrow schema, and empty input yields a valid 0-row batch. * refactor(writer): model position deletes as a crate-internal PositionDeletes accumulator Following blackmwk's review of the redesigned position delete assembly, replace the free position_delete_batch function with a small crate-internal PositionDeletes accumulator. Callers build up deletes with insert(path, pos) and render them once via to_record_batch(). Positions are now taken as u64: a row position is non-negative by construction, so the old negative-i64 rejection is gone. The batch's Int64 pos column is filled with a checked u64-to-i64 conversion that returns DataInvalid rather than panicking on a value past i64::MAX. Rows stay keyed by a BTreeMap<String, RoaringTreemap>, so iteration yields the spec-required (file_path, pos) order with duplicate pairs removed, and the two columns are now built with Arrow StringBuilder and Int64Builder instead of intermediate Vecs. The batch is assembled against the writer's shared position_delete_arrow_schema, and an empty accumulator still yields a valid 0-row batch. Nothing outside the crate consumes this yet, so the whole surface is pub(crate) and the module carries a dead-code allowance until the write-path plumbing lands. The public API drops the position_delete_batch entry accordingly. * refactor(writer): stream PositionDeletes as fixed-size record batches Follow-up to blackmwk's review on apache#3230. - Add PositionDeletes::to_record_batches(max_rows), a lazy iterator that builds each fixed-size batch on demand instead of materializing the whole delete set at once, bounding peak memory over large delete sets. It walks the paths and positions in sorted (file_path, pos) order, splitting on max_rows boundaries; a path whose positions overflow one batch resumes in the next. A max_rows of 0 yields a single DataInvalid error rather than panicking, and the stream stops after any error. - Build the file_path column with StringBuilder::append_value_n, appending each path's run within a batch in one call rather than once per row. - Factor a shared private build_batch helper so to_record_batch (single batch) and to_record_batches share the column-building logic. - Document that duplicate (file_path, pos) inserts collapse silently, which is intentional for position deletes. * refactor(writer): debug_assert against duplicate and oversized position deletes
ci: auto-label issues and PRs that mention DataFusion Contributors without triage permission cannot add labels. Use github/issue-labeler to apply the datafusion label when an issue or PR title or body mentions DataFusion. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…he#3289) Co-authored-by: ovoievodin <o_voievodin@apple.com>
…che#3241) There's some assorted places that we need to raise errors instead of panicking.
…ource-ids (apache#3267) * Make UnboundPartitionField fields private and carry source-ids The four public fields are replaced by accessors, and the derived builder's visibility is limited to the crate, so an instance can only be built inside the crate or read out of a partition spec JSON. That is what lets the struct guarantee its own invariant: source_ids always holds at least one id. source_ids: Vec<i32> replaces source_id: i32. source_id() returns the single id, and an error for a multi-argument field rather than quietly handing back the first one. The serde module reads either spelling -- source-id, or source-ids for a v3 multi-argument transform -- and writes back the one that matches the field. Binding a multi-argument field now fails with a clear error. A bound PartitionField still carries a single source_id, so until that struct is converted there is nowhere to put the extra ids, and failing loudly beats dropping them on the way into TableCreation or TableUpdate::AddSpec. * Reject source-id and source-ids together in UnboundPartitionField PyIceberg already treats the two spellings as mutually exclusive, and no writer emits both, so accepting a matching pair only let the two readers disagree. Also name both spellings in the missing-id error and note why check_transform_compatibility rejects a multi-argument field. Co-authored-by: Isaac <no-reply@databricks.com> * Make UnboundPartitionField buildable and pass it to add_partition_field Address review: the UnboundPartitionField builder and with_field_id are public, since an unbound field is not expected to be validated yet, and UnboundPartitionSpecBuilder::add_partition_field now takes the field rather than a single source id, name and transform. With the builder public an empty source_ids can be built, so serializing one no longer panics and both spec builders reject it with a clear error. Co-authored-by: Isaac <no-reply@databricks.com> * Validate UnboundPartitionField when it is built Address review: the builder's build() now returns Result<UnboundPartitionField> and rejects an empty source_ids, following FileScanTask. Deserialization goes through the same builder, so the check lives in one place and the spec builders no longer need their own. Co-authored-by: Isaac <no-reply@databricks.com> --------- Co-authored-by: Isaac <no-reply@databricks.com>
* refactor(reader): build Arrow reader metadata once * refactor(reader): address review feedback on metadata rebuild - Defer materializing an owned Arrow schema until a rebuild is actually needed; the embedded-IDs/no-INT96/no-row-number fast path now returns early without cloning. - Interpolate missing_field_ids, install_row_number, and the resulting schema into the ArrowReaderMetadata error message so failures identify which transform was active. - Add a regression test covering the combined path: a field-id-less file with INT96 timestamps and a _pos projection, asserting coerced timestamps and the row-number column together. Co-authored-by: Isaac <no-reply@databricks.com> * refactor(reader): address remaining review feedback --------- Co-authored-by: Isaac <no-reply@databricks.com>
* refactor(scan) move test utils to test_utils.rs * move things around * move * fix * comments
…te_file_path (apache#3268) * fix: avoid lost-wakeup hang in get_equality_delete_predicate_for_delete_file_path The equality-delete loader signals completion with Notify::notify_waiters(), which stores no permit and only wakes Notified futures created before the call. get_equality_delete_predicate_for_delete_file_path observed the Loading state, released the read lock, and only then created the Notified future; a notify_waiters() fired in that window is lost and the await hangs forever. Create the Notified (via notified_owned) while still holding the read lock, so it is guaranteed to precede any notify_waiters() and be delivered. Signed-off-by: Dhruv Arya <aryadhruv@gmail.com> * docs: explain why building Notified under the read lock prevents the lost wakeup State the mechanism rather than the outcome: notified_owned() snapshots tokio's notify_waiters_calls counter at construction, and holding the read lock orders that snapshot before insert_equality_delete can advance it. This keeps a later refactor from moving notified_owned() out from under the lock unnoticed. Signed-off-by: Dhruv Arya <aryadhruv@gmail.com> --------- Signed-off-by: Dhruv Arya <aryadhruv@gmail.com> Co-authored-by: Dhruv Arya <aryadhruv@gmail.com>
* feat: add core COW rewrite primitive Add a core copy-on-write rewrite primitive that plans candidate data files, applies a caller-provided RecordBatch rewriter, writes replacement data files, and returns removed/added/unchanged file lists for future overwrite-style commit actions. - Plan candidates through the existing scan path so manifest pruning and delete-file application stay consistent with normal reads; the row predicate is cleared only for the full-file rewrite input. - Stream rewritten batches to the replacement writer: buffer only the prefix before the first changed batch, then open the writer lazily; unchanged files open no writer, fully-deleted files produce no replacement. - Bind the replacement writer and partition key to the planned snapshot's schema so rewrites survive schema evolution. - Build the replacement parquet writer via ParquetWriterBuilder::from_table_properties so replacement files honor the table's write.parquet.* properties, carry the table's encryption manager so encrypted tables are not downgraded to plaintext, and refuse to run when write.format.default is not parquet. - Keep the planner internal; CowRewriteBuilder is the only public entry and CowRewriteFile fields sit behind accessors. Cover delete-style, update-style, no-op, full-file delete, no-match delete, delete-file planning, writer edge cases, schema evolution, multi-source replacement path uniqueness, and partition-value preservation (including the null partition) end-to-end. Part of apache#2269. Co-Authored-By: Claude <noreply@anthropic.com> * fix: address COW rewrite review comments - derive the effective change flag as `changed || output.is_none()` so a rewriter returning {changed: false, output: None} cannot silently drop rows while leaving the file marked unchanged; add regression tests - treat a candidate whose visible rows are all removed by delete files as removed with no replacement, so it can be compacted away with its delete files instead of landing in unchanged_data_files forever - honor write.object-storage.enabled when building replacement writers (add the missing table property) so object-storage-layout tables keep hash-entropy paths - count output_rows only for rows actually written to replacement files - derive Debug for CowBatchRewrite - document the sync rewriter contract, the read-only CowRewriteFile surface, and the prefix buffer's whole-file worst case - drop avoidable FileScanTask/DataFile clones and replace the writer expect() with a let-else invariant * fix(cow_rewrite): flush kept prefix after a dropped batch A dropped batch could mark a file changed while skipping the Some(output) branch that initialized the writer and flushed kept rows. Open the writer whenever a changed file has buffered or current output, then flush the prefix independently of the current batch output. Cover the keep-first/drop-second data-loss case and full coverage by a real position-delete file; clarify delete-file and schema contracts. * fix(cow_rewrite): guard partition preservation and document rewrite() trade-offs Address the three non-blocking follow-ups from the approval review of apache#2752: - Surface the prefix-buffer memory ceiling in the public rewrite() rustdoc: a file that never changes (or whose first change is at EOF) buffers its entire decoded contents, one file at a time, which can be several GB for a compaction-sized file. - Add a debug-only partition guard: replacement files record the source partition verbatim, so a rewriter that mutates a partition-source column would silently write rows under a partition they do not belong to. In debug builds every emitted batch is checked against the source file's partition; release builds are unchanged. - Document in the module docs that keeping the source partition spec and staying unsorted is, like the snapshot-schema choice, a deliberate divergence from Java's RewriteDataFiles / Spark DML. * fix(cow_rewrite): use relocated scan test fixture --------- Co-authored-by: Claude <noreply@anthropic.com>
* fix(spec): Harden decimal and fixed type string parsing deserialize_decimal and deserialize_fixed used trim_start_matches and trim_end_matches, which strip repeated occurrences and never require the prefix, so "decimal(decimal(5, 2)))))" and "fixed[16" both parsed successfully. Precision was also unbounded, so "decimal(50, 2)" was accepted into Schema and TableMetadata even though the crate's own MAX_DECIMAL_PRECISION is 38, and the failure only surfaced much later in Arrow or Avro conversion. Parse strictly with strip_prefix and strip_suffix, and reject precision outside 1..=38 and scale greater than precision. Signed-off-by: Jeremy Schoemaker <jeremy@shoemoney.com> * fix(spec): Clarify zero precision errors and verify serde roundtrips * test(spec): cover decimal and fixed parsing edge cases * fix(spec): align decimal serde validation with constructor * fix(spec): preserve type parse errors and canonical serde coverage --------- Signed-off-by: Jeremy Schoemaker <jeremy@shoemoney.com>
…pache#3282) * perf(arrow): skip rebuilding top-level projected arrays * perf(arrow): drop in-crate benchmark harness * perf(arrow): skip rebuilding nested projected arrays * perf(arrow): tighten projector zero-copy path * perf(arrow): polish projector zero-copy path
…pache#3314) Page-index bounds for INT32/INT64 columns were built as Int/Long literals regardless of the Iceberg field type. For decimal fields, query datums use Int128, and Datum::partial_cmp only compares decimals when both sides are Int128, so every comparison returned None. This silently pruned all matching pages for inequality and IN predicates when page-index row selection was enabled. Convert INT32/INT64 page bounds to Int128 for decimal fields, matching the row-group statistics path. Follow-up: FIXED_LEN_BYTE_ARRAY decimals (precision > 18) still get no page-index pruning, even though the row-group stats path decodes them. This only over-selects pages (never drops rows) and is pre-existing. Decoding them here later must guard against truncated ColumnIndex bounds (column_index_truncate_length). Closes apache#3264 Co-authored-by: Fokko Driesprong <fokko@apache.org>
…#3286) * init * address review: doc sibling methods, tidy test, note break in changelog
* ci: check dependency licenses with cargo-deny on every PR Run `dev/release/dependencies.sh check` in the `lint` job, so a dependency whose license is not allowed by `deny.toml` fails the pull request instead of being found at release time. Set `[graph] all-features = true` in `deny.toml`. Without it, cargo-deny only follows default features. Since apache#3143 that graph no longer includes the optional OpenDAL backends, so the check skipped the MPL-2.0 crates that `deny.toml` has exceptions for. Closes apache#3234. * Tighten docs/comments, reuse dependencies.sh in create_rc.sh - Trim comments and docs that over-explain. - Condense the CONTRIBUTING.md license guidance. - create_rc.sh calls dependencies.sh check so the release uses the pinned cargo-deny version, same as CI. - Drop unused bzip2-1.0.6 from the deny.toml allow-list. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Kevin Liu <kevin.jq.liu@gmail.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
apache#3263) * feat(storage-opendal)!: make the per-IO-operation timeout configurable `iceberg-storage-opendal` wraps every FileIO operator in `TimeoutLayer::new()`, whose 10s `io_timeout` bounds each `read`/`write` and each method call on a returned reader or writer. Nothing in the property or builder surface can override it, so an operation that legitimately needs longer fails deterministically: the `RetryLayer` above re-sends the same request, which cannot fit in the budget either. Add `client.io-timeout-ms`, parsed into an `OpenDalClientConfig` built with `#[derive(Properties)]` and handed to `TimeoutLayer::with_io_timeout`. The config's fields are private, so later client settings such as retry are additive rather than breaking. The default is unchanged. BREAKING CHANGE: `OpenDalStorage` variants carry a `client` field, and `OpenDalStorage::Memory` is now a struct variant. * refactor(storage-opendal): address review feedback on the IO timeout Quote the rejected value in the parse error, so an empty input shows as `value: ""`. `TimeoutLayer` exposes no getters, so a new test compares its `Debug` output to pin `DEFAULT_IO_TIMEOUT_MS` to OpenDAL's default. The parsing test now uses the constant instead of repeating the literal. Add a serde round-trip test for the `client` field, including a payload that omits it. Drop the `DEFAULT` associated const, which only fed a fallback arm that cannot run. Update the upstream `test_writer_close_returns_stored_size` for the struct variant, and reuse `StorageConfig::with_prop` and `empty_resolving_storage` in the tests. * refactor(storage-opendal): rename the IO timeout key to opendal.io-timeout-ms Only `iceberg-storage-opendal` honors the key, so namespace it under `opendal.` and define it as `OPENDAL_IO_TIMEOUT_MS` next to `OpenDalClientConfig`, rather than in `iceberg::io`. This matches how `ADLS_SAS_TOKEN` and `GCS_ALLOW_ANONYMOUS` sit next to the configs that parse them. With the key moved, the `iceberg` crate needs no change. * refactor(storage-opendal): rename the client field to client_config `client` reads like an HTTP client handle. Rename the field on every `OpenDalStorage` variant, and the `OpenDalStorage::client()` accessor, to `client_config` to match `OpenDalClientConfig`. `opendal_config` would be ambiguous next to the backend `config` field, which holds an OpenDAL config too. `test_default_io_timeout_matches_opendal` compares `Debug` output. Both sides go through the same derived impl, so a format change cannot make them differ. A `Debug` that stopped printing `io_timeout` would make the check pass vacuously, so also assert that a different `io_timeout` produces a different string. * fix(storage-opendal): reject a zero IO timeout when deserializing `parse_io_timeout_ms` rejected zero, but the derived `Deserialize` did not go through it. A serialized `OpenDalStorage` carrying `"io_timeout_ms": 0` was rebuilt without error, and the zero reached `TimeoutLayer`. Store the timeout as a `NonZeroU64`. `str::parse` and serde's `NonZeroU64` impl both reject zero, so both construction paths share one check. The serialized form is still a plain integer. The generated getter now returns `&NonZeroU64`, because the `Properties` derive only returns by value for the primitive types it recognizes. The parsing test also covers the edges of the valid range. `1` and `u64::MAX` are accepted, and `u64::MAX + 1` is rejected. * refactor(storage-opendal): return the IO timeout as a Duration `OpenDalClientConfig::io_timeout_ms()` returned `&NonZeroU64`, because the `Properties` derive only returns primitives by value. Replace the generated getter with `io_timeout()`, which returns a `Duration`, and keep the millisecond field private. Expose the default as `OPENDAL_IO_TIMEOUT_MS_DEFAULT`, following the `*_DEFAULT` consts in `iceberg`, so the key's doc links to it instead of repeating the number. Keep the `ParseIntError` as the source of a rejected value, as `parse_pool_property` does, so the message says why it failed. Document that the serialized form of `OpenDalStorage` is not stable across crate versions, as `FileIO::serialize_all` already does for its own. Test that the configured timeout reaches `TimeoutLayer`. A memory operator behind `ConcurrentLimitLayer::new(0)` stalls every IO call, and paused tokio time skips the wait and the retry backoff. Check propagation through `OpenDalResolvingStorage` for every enabled scheme, not only S3. * refactor(storage-opendal): take the client config defaults from the derive Build `OpenDalClientConfig::default()` from `from_properties` with no properties, so each default is declared once in `#[property]` and serde and `from_properties` cannot disagree as settings are added. Say in the `parse_io_timeout_ms` doc that it exists to quote the value and name the unit, and that `NonZeroU64` is what rejects zero. Fold the valid-value asserts in the parsing test into one loop. * refactor(storage-opendal): build the client config default directly `Default` delegated to `from_properties` with no properties and called `expect`. A later setting without a `#[property]` default would make it panic, and because of the container-level `#[serde(default)]`, serde calls `default()` on every `client_config` it deserializes. Build the struct directly, and keep the two defaults equal with `test_default_matches_property_defaults`. Name OpenDAL's 60-second control-operation budget in the `OPENDAL_IO_TIMEOUT_MS` doc, along with the calls it covers here, `exists` and `metadata`. `test_default_timeouts_match_opendal` now pins both defaults that the doc states. Pin the serialized-form break with a test that rejects the old `"LocalFs"`, `"Memory"` and `{"Memory": null}` forms. Match the stall test's error on `{ timeout: 45 }`, so a 450-second budget cannot pass. * refactor(storage-opendal): keep the client config accessors crate-private `OpenDalStorage::client_config()`, `OpenDalClientConfig::io_timeout()` and `OPENDAL_IO_TIMEOUT_MS_DEFAULT` have no callers outside the crate. Keep them out of the public API so they can be exposed later without a breaking change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * refactor(storage-opendal): trim redundant docs, comments and test Fold the `u64` default into the `NonZeroU64` const, drop `test_default_matches_property_defaults` (one field, one shared const), shorten docs and comments that restated the code, and keep the original `op` binding in the `Memory` arm of `create_operator`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * docs(storage-opendal): clarify the IO timeout and serialization docs Name OpenDAL's `io_timeout` and `timeout` settings so it is clear the property bounds each IO call per retry attempt and leaves control operations such as `stat` at 60 seconds. Align the `OpenDalStorage` serialization note with the wording on `FileIO::serialize_all`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * test(storage-opendal): check OpenDalStorageFactory propagates the IO timeout Every `OpenDalStorageFactory::build` arm could fall back to the default `OpenDalClientConfig` without any test failing. Build each enabled backend with a 45000 ms timeout and read it back from the serialized storage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: ovoievodin <o_voievodin@apple.com> Co-authored-by: Kevin Liu <kevin.jq.liu@gmail.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Bumps [astral-sh/setup-uv](https://github.com/astral-sh/setup-uv) from 10.0.1 to 10.2.0. - [Release notes](https://github.com/astral-sh/setup-uv/releases) - [Commits](astral-sh/setup-uv@20cfd1b...c18668a) --- updated-dependencies: - dependency-name: astral-sh/setup-uv dependency-version: 10.2.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…he#3317) Bumps [taiki-e/install-action](https://github.com/taiki-e/install-action) from 2.87.8 to 2.87.21. - [Release notes](https://github.com/taiki-e/install-action/releases) - [Changelog](https://github.com/taiki-e/install-action/blob/main/CHANGELOG.md) - [Commits](taiki-e/install-action@d438492...4cef141) --- updated-dependencies: - dependency-name: taiki-e/install-action dependency-version: 2.87.20 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps the codeql-action group with 2 updates: [github/codeql-action/init](https://github.com/github/codeql-action) and [github/codeql-action/analyze](https://github.com/github/codeql-action). Updates `github/codeql-action/init` from 4.37.9 to 4.38.2 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@cdf488f...2892aa5) Updates `github/codeql-action/analyze` from 4.37.9 to 4.38.2 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@cdf488f...2892aa5) --- updated-dependencies: - dependency-name: github/codeql-action/init dependency-version: 4.38.2 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: codeql-action - dependency-name: github/codeql-action/analyze dependency-version: 4.38.2 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: codeql-action ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ache#3313) Bumps [urllib3](https://github.com/urllib3/urllib3) from 2.7.0 to 2.8.0. - [Release notes](https://github.com/urllib3/urllib3/releases) - [Changelog](https://github.com/urllib3/urllib3/blob/main/CHANGES.rst) - [Commits](urllib3/urllib3@2.7.0...2.8.0) --- updated-dependencies: - dependency-name: urllib3 dependency-version: 2.8.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* docs: add agent guidance on code comments and API surface Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * docs: add agent guidance on PR descriptions Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * docs: separate always-flag review rules Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * docs: soften always-flag wording Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * docs: drop refactor test from comment guidance Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * docs: base pub guidance on intended API Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * docs: clarify visibility guidance Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * docs: add reuse and test duplication guidance Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * docs: trim reuse guidance Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Apply batched suggestions from code review Co-authored-by: Matt Butrovich <mbutrovich@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: blackmwk <257669749+blackmwk@users.noreply.github.com> Co-authored-by: Matt Butrovich <mbutrovich@users.noreply.github.com>
* Add trait method `AuthManager::contextual_session` * Implement contextual session handling * Produce an auth_session per operation and re-use it for multiple Http requests * Rename client to RestCatalogClient * Mark context in contextual_session as used in API docs * Drop Java-derived comment on releasing resources * Revert "Produce an auth_session per operation and re-use it for multiple Http" This reverts commit 32a9dc5. * Expand test coverage * Pass auth session to client on each call This prevents cloning the HeaderMap of the client because a new session is passed for every HTTP request anyway * Leave post_form unauthenticated * Implementor guidance
…cripts and guides (apache#3338)
* docs: design delete task validation * fix(scan): validate delete task construction * refactor(scan): use validated delete task builder * refactor(scan): centralize deletion vector validation * style(scan): format validated delete task changes * fix(scan): satisfy public API and license checks * test(scan): adapt merged reader fixtures * refactor(scan): encapsulate delete file metadata * fix(scan): propagate invalid delete task errors * fix(scan): unwrap the delete-file builder in the scan test `FileScanTaskDeleteFile::builder().build()` returns `Result` since this branch made it validate deletion-vector metadata, and this call site was missed, so `cargo check --all-targets` failed with E0308 once the branch was rebased onto a main that also compiles the lib test target. The task built here is a complete equality delete, so validation cannot fail; `.unwrap()` matches how the other tests in this file build tasks. * refactor(scan): make delete-file byte offsets unsigned Addresses @blackmwk's review. content_offset and content_size_in_bytes are byte quantities, so Option<u64> makes a negative unrepresentable instead of something every layer re-checks. That collapses the three findings into one change: - 'Why not change it to u64?' / 'Ditto': the getters and fields are u64. - 'This duplicates a lot.': the duplication existed only to build the 'deletion vector' vs 'delete file' prefix for two near-identical negative-value errors. Both are gone, and the remaining presence checks collapse to one lookup plus one error site. - 'I think this should also apply to non deletion vector?': it now does, and unconditionally. The i64 -> u64 conversion happens once in TryFrom<&DeleteFileContext>, where the manifest value enters, so every delete-file kind is covered rather than only Puffin position deletes. Also addresses @mbutrovich's deserialization concern from the same review: FileScanTaskDeleteFile derives plain Deserialize, so a scan plan rebuilt on a worker never passes through build(). With u64 fields, serde itself rejects a negative offset, so a hand-written or corrupted plan fails before any loader code runs -- stronger than the runtime check it replaces. Downstream, validate_deletion_vector_task loses its two u64::try_from conversions; the values are already u64. Tests: the negative cases moved from the builder to the manifest boundary (test_delete_context_rejects_negative_*), including the non-DV case, and the two loader tests now assert serde refuses the value. cargo test -p iceberg --lib: 1683 passed, 0 failed. cargo fmt --all -- --check and cargo clippy -p iceberg --all-targets -D warnings: clean. public-api.txt updated for the two getters. Signed-off-by: linhongyu510 <linhongyu510@gmail.com> * refactor(scan): validate delete files on deserialize and enforce size pairing Address review follow-ups on the delete-file validation design: - Route FileScanTaskDeleteFile deserialization through a FileScanTaskDeleteFileSerde mirror that builds via builder().build(), so a deletion vector rebuilt from a serialized scan plan (planner->worker path) passes validate() like a manifest- built task. A malformed plan now fails at deserialize time instead of reaching a loader expect/ok_or_else. - Enforce the spec pairing rule for every delete-file kind: content_size_in_bytes is required whenever content_offset is present, not only for deletion vectors. - Replace two builder-only duplicate tests with one driving the missing-coordinate check through get_deletes_for_data_file, where the manifest entry is converted. - Reword PR-history-laden comments to describe the code as it is now. cargo test -p iceberg --lib: 1683 passed, 0 failed. * docs(scan): clarify deletion vector loader extraction --------- Signed-off-by: linhongyu510 <linhongyu510@gmail.com> Co-authored-by: linhongyu510 <linhongyu510@users.noreply.github.com> Co-authored-by: linhongyu510 <mailto:linhongyu510@gmail.com>
… without Value (apache#3354) * perf(manifest): upgrade to apache-avro 0.22 and read manifest entries without Value * add test cases, update docs * address PR feedback * address PR feedback. * address PR feedback
…pache#3328) Decimals with precision > 18 are stored as FIXED_LEN_BYTE_ARRAY, which the page-index evaluator skipped, so inequality and IN predicates on them got no page pruning (over-selection only, never dropped rows). Decode those bounds with i128_from_be_bytes, matching the row-group statistics path. Guard against truncated ColumnIndex bounds: a bound whose width differs from the column's declared type_length signals a column_index_truncate_length truncation, and a truncated byte prefix no longer preserves the two's complement numeric ordering of decimals. Reject it so the column skips pruning rather than decode the wrong number. parquet-rs never truncates decimal column indexes, so this only defends against files written by other implementations. Benchmark A throwaway benchmark on a 1,000,000-row single-column decimal(30,2) file (precision > 18 -> FIXED_LEN_BYTE_ARRAY), 100 pages in one row group, read fully in memory so timing is decode-only. A `col > threshold` predicate sweeps selectivity. "read full file" is today's behavior (FLBA decimals skip page pruning); "read ms" applies the page-index RowSelection this change produces. 1,000,000 rows in 100 pages, 1 row group read full file : 3.431 ms predicate (keep) | rows read | eval us | read ms | speedup 1% | 10,000 | 17.5 | 0.038 | 90.0x 10% | 100,000 | 17.1 | 0.330 | 10.4x 50% | 500,000 | 16.6 | 1.629 | 2.1x 100% | 1,000,000 | 16.3 | 3.397 | 1.0x The win tracks predicate selectivity; eval() adds a constant ~17 us per row group per scan, negligible against the millisecond read. The benefit only materializes where page-index row selection runs: MoR tables or with_row_selection_enabled(true). Reproducer in the PR description. Follow-up to apache#3314.
* fix(spec): treat -0.0 and 0.0 as distinct float literals PrimitiveLiteral equality and ordering now follow Java's Float.compare and Double.compare: -0.0 is less than 0.0 and all NaNs are equal. This stops the partitioned writers, the record batch splitter and the delete file index from merging -0.0 and 0.0 identity partitions. Closes apache#3325 * Address review: fail closed in PrimitiveLiteral fallback, test IN - Handle AboveMax and BelowMin explicitly and return None when the fallback arm sees two values of the same variant, with a debug assertion, instead of calling them equal. - Cite the spec's partition equality rule and document the variant_index ordering invariant. - Stop pinning the cross-variant order in tests. - Test IN with signed zeros in the expression evaluator.
apache#3379) This change migrates from the old shell script for cargo-deny to a new Python script. This new script will contain logic for dealing with dependencies and their licenses. This commit simply translates the existing logic. In later changes, this script will be extended with: - Clarifications for existing dependencies where licenses are misreported, with additional verifications added in the script itself - Logic for generating the attribution documentation for the pyiceberg-core package.
* feat(rest): add a SigV4 request signer * fix(rest): mute aws-sigv4 tracing and tighten payload hashing The crate traces the request it signs, and its redaction list covers `authorization` but not the `Original-` copy we make, so a delegate's bearer token could reach trace logs. Also reject non-UTF-8 headers rather than leave them unsigned, and hash a present-but-empty body as Java does. * refactor(rest): take credentials per call, as Java's signer does Java holds one `Aws4Signer` across sessions and resolves credentials from an `AwsCredentialsProvider` per request, so the signer itself carries no credential state. Follow that: drop `AwsCredentials`, take the AWS crate's `Credentials` as a `sign` argument, and leave the provider to the auth session. Also sign `HttpRequest` rather than the concrete request type, name the amz headers, split `convert_headers`/`update_request_headers` after their Java counterparts, collapse the one-file `sigv4` module, and put the AWS dependencies behind a `sigv4` feature. * docs(rest): trim the SigV4 signer comments * refactor(rest): break sign_at into named steps * docs(rest): state the redirect and log-always limits of the signer * fix(rest): keep relocation headers out of the signature, and the tracing flag untouched * docs(rest): name the log bridge that stays unmuted * docs(rest): log-always also bypasses the muted subscriber * fix(rest): return an error instead of unwrapping the content hash header * refactor(rest): make PayloadHashMode non_exhaustive and SigV4Signer::new take impl Into<String> * docs(rest): note the base64 header quirk and credential freshness * docs(rest): keep the plus note next to the rewrite it explains * fix(rest): spell out the headers Java leaves unsigned * test(rest): check signatures against Iceberg Java * test(rest): pin the signing tests to a fixed time * test(rest): check standard mode against the AWS test suite * test(rest): drop the helper-only AWS vector and the hmac dev-dependency * fix(rest): decide the tracing mute with public tracing API * docs(rest): tighten the payload hash and credential notes * test(rest): bound the live-clock stamp by readings around the call * chore(rest): update the public API for the signer changes * docs(rest): simplify the SigV4 signer comments * refactor(rest): build SigV4Signer with a typed builder * fix(rest): propagate a failed userinfo strip instead of signing the URL as is * docs(rest): name the flag the tracing mute is gated to avoid * docs(rest): say that as_bytes merges an absent body with an empty one * chore(rest): update the public API for the signer builder * fix(rest): validate requests before signing * fix(rest): address SigV4 review findings
apache#3366) Bumps [fsspec](https://github.com/fsspec/filesystem_spec) from 2026.4.0 to 2026.6.0. - [Commits](fsspec/filesystem_spec@2026.4.0...2026.6.0) --- updated-dependencies: - dependency-name: fsspec dependency-version: 2026.6.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…apache#3386) test(reader): widen FLBA decimal page-index coverage and clarify guard docs Address review feedback on apache#3328: - Reframe the width-guard doc and inline comment as defence-in-depth against a non-conforming writer - Hoist the duplicated min/max decode into a single closure. - Add page-index tests
* fix(scan): reject ambiguous case-insensitive fields Co-authored-by: Codex <codex@openai.com> * fix(schema): clarify ambiguous case-insensitive lookup Expose the checked lookup, document collision semantics, and cover the public fallback. Simplify the binding and scan callers. Co-authored-by: Codex <codex@openai.com> * fix(schema): improve case-insensitive collision diagnostics List sorted colliding field names with invalid_data!, simplify optional lookup, and loosen error assertions while checking the reported colliders. Co-authored-by: Codex <codex@openai.com> --------- Co-authored-by: Codex <codex@openai.com>
* feat(RTAS) Assign fresh ids * update tests with more types * avoid clone * document assign better * relax test * additional test * comment * Add debug assert * lowercase * doc * not nested * test * test comments * comments * starts_from
apache#3255) * fix(arrow): reconcile evolved nested structs by field id, not position When a struct gains a field (schema evolution) after a data file was written, the file's struct has fewer children than the table schema. RecordBatchTransformer promoted such columns with arrow_cast::cast, which matches struct/list/map children positionally -- so every child after the gap shifts and types collide, e.g. 'Casting from Utf8 to Struct' when a scalar lands on an added list<struct>. Replace the flat cast with a PromotePlan that is resolved once per file when the BatchTransform is built: nested struct children are matched to the target schema by PARQUET:field_id, recursing through list/large-list/map, so the per-batch work is just index lookups and array assembly. Absent optional children are filled with typed nulls; absent required children error, as do absent children with an initial-default (not yet wired up for nested fields) and source structs whose children carry no field ids, rather than silently producing wrong data. Primitives still use cast for valid Iceberg promotions. Mirrors iceberg-java's by-field-id nested readers. Closes apache#2617 * test(arrow): cover appended nested struct field on promote Add a unit test for struct<a, b> -> struct<a, b, c>, the append case of nested schema evolution. The existing middle-insert test already covers by-id matching; this pins the "expected 3 got 2" failure mode. * fix(arrow): reconcile nested reorder and rename by field id Always build a PromotePlan for a present column and pass through only when that plan is a no-op. equals_datatype ignored nested names and field ids, so a reorder or same-type rename kept the file layout. Id-less nested children still match by position when their types line up, which covers maps. Nested initial-default stays unsupported (apache#3261). * fix(arrow): error when only some nested fields have ids The positional fallback stays for a fully id-less struct. If some children have field ids and some do not, null-filling the rest would drop data, so that now errors. A same-type id-less reorder is still positional; it cannot be detected without ids. * fix(arrow): cast id-less nested fields by position Name mapping only stamps top-level ids. When the child counts match, pair by position and let build check each child, so an int still promotes to long. A different count, or ids on only some children, still errors. * test(arrow): compare remaining promote results as whole arrays Use invalid_data! for the PromotePlan DataInvalid errors, matching main. * fix(arrow): accept either list offset width from the file A Parquet Arrow schema hint can make a list column LargeList while the table schema is List. Main cast that column; PromotePlan rejected it. * fix(arrow): apply nested identity constants and drop LargeList targets A missing nested field that is an identity partition source takes that partition value before initial-default or null. schema_to_arrow_schema always emits List, so a LargeList target now errors instead of building a plan; a LargeList file column still promotes to List. The remaining promote tests compare whole arrays. Error tests check ErrorKind, including field ids on only some children. * Simplify PromotePlan and dedup nested promotion tests * fix(arrow): address final review on nested field-id promotion - Add a reader-level test that writes an old nested schema to Parquet and reads it back through an evolved one, pinning that parquet-rs attaches field ids to struct children, list elements, and map keys and values - Reject duplicate source field ids and nested sources read as a non-nested type when the plan is built - Report non-struct map entries as DataInvalid - Pin the id-less same-type reorder fallback with a test until apache#1845 - Assert map data types, since map array equality skips field names - Point at apache#3378 where the projection mask skips all-missing structs * test(arrow): cover sliced list promotion with a null slot --------- Co-authored-by: Jordan Epstein <jordan.epstein@imc.com>
* fix(spec): reject boolean literals for non-boolean types * test(spec): polish boolean literal validation coverage
… field names (apache#3303) * fix(arrow): strip metadata from list elements and map entries strip_metadata_from_schema failed with "Field stack underflow in list" on any schema with a list or map column. MetadataStripVisitor pushed a field onto its stack only in before_field, which the visitor does not call for list elements, map keys or map values, so rebuilding those popped from an empty stack. Push in before_list_element, before_map_key and before_map_value too. The stack now holds each field's name and nullability rather than a Field with a placeholder type, and one pop_field helper rebuilds fields for struct, list, map and primitive. Document the two normalizations the function makes besides removing metadata, which callers comparing stripped schemas rely on: a map's entries field is renamed to DEFAULT_MAP_FIELD_NAME, and a dictionary-encoded field becomes its value type. Closes apache#3297 * test(arrow): strip metadata from fields with repeated nested names * refactor(arrow): rename MetadataStripVisitor's field stack helpers to start_field and finish_field * docs(arrow): clarify visitor field hooks and strip_metadata_from_schema naming before_field and after_field run for every schema and struct field, not for list elements or map keys and values. strip_metadata_from_schema keeps list element names, and renames only a map's entries field. * normalize list and map field names when stripping metadata
apache#3396) get_arrow_projection_mask_with_field_ids carried each leaf's field ID from its first pass to its second through a HashMap<FieldRef, i32>. Hashing an Arrow Field hashes its whole DataType and allocates a sorted copy of its metadata keys, and the map grew from empty. Read the ID from the leaf's PARQUET:field_id metadata in both passes instead. Closes apache#3394
) * perf(reader): optimize row group filter intersection * perf(reader): add row group intersection benchmark * style: format iceberg benchmark manifest * perf(reader): avoid allocations in name mapping lookup * perf(reader): prune row groups from split candidates * fix(reader): format tests and preallocate name mapping index * fix(reader): tighten candidate pruning and name mapping coverage
) get_parquet_stat_min_as_datum / get_parquet_stat_max_as_datum decoded BYTE_ARRAY decimal bounds with a strict 16-byte conversion. Parquet stores these as minimal-length big-endian two's-complement values, so any bound shorter than 16 bytes failed to convert and aborted the row-group read. Use the variable-length, sign-extending i128_from_be_bytes helper, as the FIXED_LEN_BYTE_ARRAY arm already does. Bounds longer than 16 bytes still return DataInvalid. Empty bounds also return DataInvalid instead of decoding as zero, which could otherwise prune matching row groups. Closes apache#3280
* fix(reader): apply name mapping to nested fields apply_name_mapping_to_arrow_schema assigned field IDs to top-level columns only. The projection then never selected the leaves of a struct, list, or map in a file without embedded IDs, and those columns read as NULL whenever a top-level column was projected too. Apply the mapping at every depth, as Java's ApplyNameMapping does. Struct children are looked up within their parent's entry, and list elements and map keys and values as element, key, and value, whatever the file calls them. Closes apache#3374 * Size collections up front, shorten comments, dedupe tests - Build each rebuilt Fields from Vec::with_capacity instead of a fallible collect, and the map entries from a two-element array. - Size the maps and index list in get_arrow_projection_mask_with_field_ids. - Keep one reader test for the reported struct case and cover every nested Arrow type in one schema-level test. * Address review: skip entries without a field ID, pin types and errors - Treat a mapping entry without a field ID as missing, so its nested fields keep no IDs either. - Note that the map shape checks are defensive, and report the entry count when one fails. - Note that the projection treats ListView and LargeListView as leaves. - Test unmapped children, an entry without a field ID, the fixed-size list size, the map ordering flag, and a predicate on a nested field.
…3243) * fix(transform): correct decimal and binary truncate literals The Arrow array path already preserves decimal precision and scale and supports binary truncation. The literal path instead reset decimal scale to zero and rejected binary values. For example, truncate[50](10.65) returned 1050 instead of 10.50. Align literal truncation with the Arrow array path: preserve the source decimal precision and scale, and return the first W bytes of binary values, or the entire value when shorter. Decimal projection boundary adjustments must also preserve precision and scale to keep their values correct before truncation. * fix(transform): address truncate literal review feedback
…ories (apache#3425) chore(deps): Bump quinn-proto to fix RUSTSEC advisories Update Cargo.lock to pick up quinn-proto 0.11.19, which fixes RUSTSEC-2026-0338/0339/0340/0341/0342. Also bump event-listener (RUSTSEC-2026-0221, unsound) and the yanked chacha20 and spin releases so cargo audit runs clean.
…0261014 Brings in 62 upstream commits since the last sync (ec07ed0), while preserving this fork's own features (custom auth/retry, Azdls anonymous-access support, incremental-scan infrastructure, vended credentials, OverwriteAction, etc). Conflict resolution notes: - crates/catalog/rest/src/{auth/mod.rs,client.rs,catalog.rs}: upstream's "Contextual Sessions" (apache#3081) replaces HttpClient's single baked-in auth_session with an explicit `auth_session: &dyn AuthSession` parameter threaded through from a per-SessionContext resolution (RestCatalogClient::query_catalog -> AuthManager::contextual_session). This fork's 401/403-retry-with-invalidate logic in HttpClient::query_catalog needed every `self.auth_session` swapped for the new parameter -- the retry semantics themselves are unchanged. create_table/load_table's _internal helpers and invalidate_token/regenerate_token (not flagged as conflicts, but silently broken by the same refactor landing in non-overlapping hunks) were updated to thread a SessionContext through and to read RestCatalogClient::catalog_session directly, respectively. - crates/iceberg/src/arrow/reader/pipeline.rs: kept this fork's already more-advanced architecture (virtual columns requested upfront via open_parquet_stream_builder, consolidated into build_projected_record_batch_stream) over upstream's configure_arrow_reader_metadata refactor, which is a different refactor of logic this fork had already restructured further, including row-id/spec-id/last-updated-sequence-number materialization and bloom-filter integration that upstream's specific commits don't touch. Ported upstream's real perf fix (candidate_row_groups pre-narrowing in apply_parquet_filters, acd6109) since the helper landed un-wired to its call site across the merge. While verifying, found and fixed a real latent bug in open_parquet_stream_builder (present since this function's introduction in 4c94e86, predating any of these merges): resolving the schema across up to three *sequential* ArrowReaderMetadata rebuilds (field-id assignment, virtual columns, INT96 coercion) could silently drop the virtual-column registration on a later rebuild, or feed coerce_int96_timestamps a schema that already included a virtual field it doesn't expect. Restructured into a single final rebuild, matching upstream's own consolidation intent. Caught by two regression tests upstream added in 71e571e for exactly this combined path, both of which failed before the fix and pass now. This function is shared by both full-scan (pipeline.rs) and incremental-scan (arrow/incremental.rs), so the fix benefits both equally. - crates/iceberg/src/delete_file_index.rs: not conflict-flagged, but silently broken by b0d8baa changing FileScanTaskDeleteFile's conversion from From to TryFrom (deletion-vector validation). get_equality_deletes_for_data_file/all_deletes now propagate Result, matching the sibling get_deletes_for_data_file which already did. - crates/storage/opendal/src/{lib.rs,resolving.rs}: Azdls gained two independently-added fields (this fork's allow_anonymous, upstream's client_config) that simply needed to coexist. - crates/storage/opendal/src/azdls.rs: not conflict-flagged, but two fork-specific tests (test_azdls_create_operator, test_azdls_create_operator_anonymous -- covering the allow_anonymous parameter, which doesn't exist upstream) still asserted the stale, pre-fix leading-slash path from before upstream's 78fafa9 ("drop the leading slash from relative paths"). The production code already had the fix; only these two tests' expectations were stale. Confirmed via a disposable worktree against pre-merge main that both passed before and genuinely regressed after the merge, i.e. this wasn't a pre-existing failure. - crates/iceberg/src/scan/{mod.rs,task.rs}, arrow/reader/projection.rs: mechanical (comment wording, both sides independently adding equivalent test coverage). Verified: cargo test -p iceberg --lib (1996 passed), cargo test -p iceberg-catalog-rest -p iceberg-storage-opendal --lib (103 passed) and again with --features opendal-azdls (24 passed), cargo fmt --check and cargo clippy --all-targets -D warnings clean for iceberg, iceberg-catalog-rest, and iceberg-storage-opendal. public-api.txt regenerated for both crates with conflicting snapshots. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s shape This fork's open_parquet_stream_builder bundled "open the file" and "resolve schema + install virtual columns" into one function taking virtual_columns as an input parameter, so it could be used inside a single futures::join! with concurrent delete-file loading. Upstream keeps these as two steps: a plain open_parquet_file, then a separate configure_arrow_reader_metadata call afterward (hardcoded to only ever install its own row-number field). Split open_parquet_stream_builder back into those same two functions: open_parquet_file (now bytes_read: Option<&Arc<AtomicU64>>, so it also serves the three call sites that need virtual columns) and a generalized configure_arrow_reader_metadata that keeps accepting an arbitrary virtual_columns: Vec<Arc<Field>> rather than hardcoding row-number, preserving this fork's flexibility. The schema-resolution logic inside configure_arrow_reader_metadata is unchanged from the single-rebuild fix in the previous commit; only the function boundary moved. Updates all four callers: FileScanTaskReader::process (pipeline.rs), process_incremental_append_task and process_equality_delete_task (arrow/incremental.rs), and delete_file_loader.rs's simpler read path (now wrapping its always-on counter in Some(..)). Each non-delete-loader caller now computes has_missing_field_ids itself and builds the ParquetRecordBatchStreamBuilder directly, matching upstream's process() exactly; the futures::join! concurrency with delete-file loading in FileScanTaskReader::process is preserved unchanged, just wrapping the narrower open_parquet_file call instead of the combined one. Rationale: pipeline.rs is the single most frequently touched file across upstream syncs (4 of the last 62 commits touched it, more than any other file in this merge). Matching upstream's function boundary here should reduce conflict surface on the next sync, without touching the downstream row-id/spec-id/last-updated-sequence-number/bloom-filter logic, which was already independently convergent with upstream's own (both inherited it from shared ancestry, not fork-exclusive as earlier analysis assumed). Verified: cargo test -p iceberg --lib (1996 passed, 0 failed), cargo fmt --check and cargo clippy --all-targets -D warnings clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Syncs this fork with
upstream/main(62 commits since the last sync atec07ed0f7), preserving this fork's own features: custom auth + 401/403 retry-with-invalidate, Azdls anonymous-access support, incremental-scan infrastructure, vended credentials,OverwriteAction, etc.Conflict resolution notes
crates/catalog/rest/src/{auth/mod.rs,client.rs,catalog.rs}: upstream's "Contextual Sessions" (feat(rest_catalog::auth) Contextual Sessions apache/iceberg-rust#3081) replacesHttpClient's single baked-inauth_sessionwith an explicitauth_session: &dyn AuthSessionparameter, resolved per-SessionContext(RestCatalogClient::query_catalog→AuthManager::contextual_session, which defaults to a passthrough -- neither of this fork'sAuthManagerimpls override it, so behavior is unchanged). This fork's 401/403-retry-with-invalidate logic inHttpClient::query_catalogjust needed everyself.auth_sessionswapped for the new parameter.create_table/load_table's_internalhelpers andinvalidate_token/regenerate_tokenweren't flagged as conflicts but were silently broken by the same refactor landing in non-overlapping hunks -- updated to thread aSessionContextthrough (usingSessionContext::empty()for the context-less credential-vending convenience methods) and to readRestCatalogClient::catalog_sessiondirectly.crates/iceberg/src/arrow/reader/pipeline.rs: kept this fork's already more-advanced architecture (virtual columns requested upfront viaopen_parquet_stream_builder, consolidated intobuild_projected_record_batch_stream) over upstream'sconfigure_arrow_reader_metadatarefactor -- a different refactor of logic this fork had already restructured further, including row-id/spec-id/last-updated-sequence-number materialization and bloom-filter integration upstream's specific commits don't touch. Ported upstream's real perf fix (candidate_row_groupspre-narrowing inapply_parquet_filters,acd610997) since the helper landed un-wired to its call site across the merge.While verifying, found and fixed a real latent bug in
open_parquet_stream_builder(present since this function's introduction in4c94e868a, predating any of these merges): resolving the schema across up to three sequentialArrowReaderMetadatarebuilds (field-id assignment, virtual columns, INT96 coercion) could silently drop the virtual-column registration on a later rebuild, or feedcoerce_int96_timestampsa schema that already included a virtual field it doesn't expect. Restructured into a single final rebuild. Caught by two regression tests upstream added in71e571e53for exactly this combined path -- both failed before the fix, pass now. This function is shared by both full-scan (pipeline.rs) and incremental-scan (arrow/incremental.rs), so the fix benefits both equally.Follow-up commit: went further and converged the open/schema-resolution function boundary itself with upstream's shape -- split back into a plain
open_parquet_file(I/O) and a separateconfigure_arrow_reader_metadata(schema resolution + virtual columns, generalized to keep accepting an arbitraryVec<Arc<Field>>rather than upstream's hardcoded row-number field), matching upstream's two-step shape exactly across all four call sites (pipeline.rs::process,incremental.rs's two task processors,delete_file_loader.rs).pipeline.rsis the single most frequently-touched file across this sync (4 of 62 commits), and its row-id/spec-id/bloom-filter logic turned out to already be convergent with upstream (shared ancestry, not fork-exclusive as initially assessed) -- so narrowing this one remaining structural difference should meaningfully reduce conflict surface on the next sync.crates/iceberg/src/delete_file_index.rs: not conflict-flagged, but silently broken byb0d8baaf9changingFileScanTaskDeleteFile's conversion fromFromtoTryFrom(deletion-vector validation).get_equality_deletes_for_data_file/all_deletesnow propagateResult, matching the siblingget_deletes_for_data_filewhich already did.crates/storage/opendal/src/{lib.rs,resolving.rs}:Azdlsgained two independently-added fields (this fork'sallow_anonymous, upstream'sclient_config) that simply needed to coexist.crates/storage/opendal/src/azdls.rs: not conflict-flagged, but two fork-specific tests (test_azdls_create_operator,test_azdls_create_operator_anonymous-- covering theallow_anonymousparameter, which doesn't exist upstream) still asserted the stale, pre-fix leading-slash path from before upstream's78fafa99f("drop the leading slash from relative paths"). The production code already had the fix; only these two tests' expectations were stale. Confirmed via a disposable worktree against pre-mergemainthat both passed before and genuinely regressed after the merge.crates/iceberg/src/scan/{mod.rs,task.rs},arrow/reader/projection.rs: mechanical (comment wording, both sides independently adding equivalent test coverage).Test plan
cargo test -p iceberg --lib-- 1996 passed, 0 failedcargo test -p iceberg-catalog-rest -p iceberg-storage-opendal --lib-- 103 passedcargo test -p iceberg-storage-opendal --lib --features opendal-azdls-- 24 passedcargo fmt --checkandcargo clippy --all-targets -D warningsclean foriceberg,iceberg-catalog-rest,iceberg-storage-opendalpublic-api.txtregenerated for both crates with conflicting snapshots (the follow-up open/metadata-boundary convergence touches onlypub(crate)functions, no public API change)🤖 Generated with Claude Code