fix: resolve HRTB compiler error in cleanup_expired_files() #2134
Conversation
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
address comments
Signed-off-by: xxchan <xxchan22f@gmail.com> Co-authored-by: ZENOTME <43447882+ZENOTME@users.noreply.github.com> Co-authored-by: ZENOTME <st810918843@gmail.com>
Signed-off-by: xxchan <xxchan22f@gmail.com> Co-authored-by: ZENOTME <43447882+ZENOTME@users.noreply.github.com> Co-authored-by: ZENOTME <st810918843@gmail.com>
--------- Signed-off-by: xxchan <xxchan22f@gmail.com> Co-authored-by: ZENOTME <43447882+ZENOTME@users.noreply.github.com> Co-authored-by: ZENOTME <st810918843@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Dylan <chenzilin25@gmail.com>
* fix plan files with deletes * fmt
* feat(iceberg): rewrite_files support use_starting_sequence_number * chore(test): add test_sequence_number_in_manifest_entry
* fix: delete file lost wake Signed-off-by: xxchan <xxchan22f@gmail.com> * . Signed-off-by: xxchan <xxchan22f@gmail.com> * . Signed-off-by: xxchan <xxchan22f@gmail.com> * revert * typo --------- Signed-off-by: xxchan <xxchan22f@gmail.com> Co-authored-by: xxchan <xxchan22f@gmail.com> Co-authored-by: Li0k <yuli@singularity-data.com>
* fix(iceberg): fix rewrite-files partition-spec-id * fix(docker): update docker file * add test * update minio * Revert "update minio" This reverts commit 4464d90.
* feat: support to branch * fix: fix ref name * fix: current_snapshot_id * refactor: refactor interface * fmt
* fix: fix snapshot for entries * refactor: refactor starting_sequence_number * feat: OverwriteFiles Action * fix: fix compile * fix: add more test * typo * fix: fix ut * fix: fix check
* feat: manifest filter feat: manifest filter * feat: add ut * feat: add snapshot-id * feat: integrate filter manager and snapshot producer * feat: remove drop partition * feat: intergrate snapshot producer and filter manager * address comments * fix: address comments and UT * typo * update UT * feat: add configuration to enable filter manager * typo fix: fix drop dangling delete files (#88)
* fix(azdls): enable append mode for AZDLS write operations * fix: fix doc
fix: use branch_snapshot instead of current snashot (#94)
* support position writer * fmt * fix: fix integration-tests and bug * fix --------- Co-authored-by: Li0k <yuli@singularity-data.com>
* feat: expose task writer * feat: impl IcebergWriter * add new_with_partition_splitter for TaskWriter * support equality delta writer * fix CurrentFileStatus so that it can be called when uninitialized * fix task writer close * fmt * expose position delete schema and genreate snapshot id * better validate_partition_value error msg * rename equality_delta_writer to delta_writer --------- Co-authored-by: Dylan Chen <zilin@singularity-data.com>
* fix record to struct deserialization * refine the test case
* add update_schema action for Transaction * minor * fmt
* chore: add public access of object cache
## Which issue does this PR close? - Closes apache#1973 ## What changes are included in this PR? - Write `content` to V3 manifest so the field is preserved correctly for delete entry ## Are these changes tested? Yes
…chTransformer (apache#1821) (#107) Partially address apache#1749. This PR adds partition spec handling to `FileScanTask` and `RecordBatchTransformer` to correctly implement the Iceberg spec's "Column Projection" rules for fields "not present" in data files. Prior to this PR, `iceberg-rust`'s `FileScanTask` had no mechanism to pass partition information to `RecordBatchTransformer`, causing two issues: 1. **Incorrect handling of bucket partitioning**: Couldn't distinguish identity transforms (which should use partition metadata constants) from non-identity transforms like bucket/truncate/year/month (which must read from data file). For example, `bucket(4, id)` stores `id_bucket = 2` (bucket number) in partition metadata, but actual `id` values (100, 200, 300) are only in the data file. iceberg-rust was incorrectly treating bucket-partitioned source columns as constants, breaking runtime filtering and returning incorrect query results. 2. **Field ID conflicts in add_files scenarios**: When importing Hive tables via `add_files`, partition columns could have field IDs conflicting with Parquet data columns. Example: Parquet has field_id=1→"name", but Iceberg expects field_id=1→"id" (partition). Per spec, the correct field is "not present" and requires name mapping fallback. Per the Iceberg spec (https://iceberg.apache.org/spec/#column-projection), when a field ID is "not present" in a data file, it must be resolved using these rules: 1. Return the value from partition metadata if an **Identity Transform** exists 2. Use `schema.name-mapping.default` metadata to map field id to columns without field id 3. Return the default value if it has a defined `initial-default` 4. Return null in all other cases **Why this matters:** - **Identity transforms** (e.g., `identity(dept)`) store actual column values in partition metadata that can be used as constants without reading the data file - **Non-identity transforms** (e.g., `bucket(4, id)`, `day(timestamp)`) store transformed values in partition metadata (e.g., bucket number 2, not the actual `id` values 100, 200, 300) and must read source columns from the data file 1. **Added partition fields to `FileScanTask`** (`scan/task.rs`): - `partition: Option<Struct>` - Partition data from manifest entry - `partition_spec: Option<Arc<PartitionSpec>>` - For transform-aware constant detection - `name_mapping: Option<Arc<NameMapping>>` - Name mapping from table metadata 2. **Implemented `constants_map()` function** (`arrow/record_batch_transformer.rs`): - Replicates Java's `PartitionUtil.constantsMap()` behavior - Only includes fields where transform is `Transform::Identity` - Used to determine which fields use partition metadata constants vs. reading from data files 3. **Enhanced `RecordBatchTransformer`** (`arrow/record_batch_transformer.rs`): - Added `build_with_partition_data()` method to accept partition spec, partition data, and name mapping - Implements all 4 spec rules for column resolution with identity-transform awareness - Detects field ID conflicts by verifying both field ID AND name match - Falls back to name mapping when field IDs are missing/conflicting (spec rule #2) 4. **Updated `ArrowReader`** (`arrow/reader.rs`): - Uses `build_with_partition_data()` when partition information is available - Falls back to `build()` when not available 5. **Updated manifest entry processing** (`scan/context.rs`): - Populates partition fields in `FileScanTask` from manifest entry data 1. **`bucket_partitioning_reads_source_column_from_file`** - Verifies that bucket-partitioned source columns are read from data files (not treated as constants from partition metadata) 2. **`identity_partition_uses_constant_from_metadata`** - Verifies that identity-transformed fields correctly use partition metadata constants 3. **`test_bucket_partitioning_with_renamed_source_column`** - Verifies field-ID-based mapping works despite column rename 4. **`add_files_partition_columns_without_field_ids`** - Verifies name mapping resolution for Hive table imports without field IDs (spec rule 5. **`add_files_with_true_field_id_conflict`** - Verifies correct field ID conflict detection with name mapping fallback (spec rule #2) 6. **`test_all_four_spec_rules`** - Integration test verifying all 4 spec rules work together Yes, there are 6 new unit tests covering all 4 Iceberg spec rules. This also resolved approximately 50 Iceberg Java tests when running with DataFusion Comet's experimental apache/datafusion-comet#2528 PR. --------- Co-authored-by: Matt Butrovich <mbutrovich@users.noreply.github.com> Co-authored-by: Renjie Liu <liurenjie2008@gmail.com>
* feat: support clean_up_expired_files * refactor: refactor interface * fmt
* add support for biglake * fix * fmt * remove project id * remove debug code * fmt + rename
Add support for configuring IO timeout and retry behavior through FileIOBuilder properties: - io.timeout: timeout duration in seconds - io.max-retries: maximum number of retry attempts - io.retry.min-delay-ms: minimum delay between retries - io.retry.max-delay-ms: maximum delay between retries Rename create_operator to create_operator_with_config to pass config to the operator builder, enabling timeout and retry layer configuration. Extract parse_config helper function to reduce code duplication when parsing configuration values.
* feat: delete orphan files * refactor: refactor to stream interface and improve doc * fmt * chore: improve doc * chore: rename * refactor: unify load manifest and load manifest_list * fix: ci * chore: set DEFAULT_OLDER_THAN_MS to 7days
* support v3 delete vector writer * support v3 deletion vectors readers from puffin * fmt * fix
* parallel get byte ranges * cap concurrency * improve
Eagerly collect manifest list paths into Vec<String> before the
await point in clean_files to avoid a higher-ranked trait bound
(HRTB) compiler error.
The closure `|s| s.to_string()` on HashSet<&'a str> gets inferred
with a concrete lifetime that the compiler cannot generalize across
async generator boundaries, producing:
"closure with signature fn(&'0 str) -> String must implement
FnOnce<(&'1 str,)>, for any two lifetimes '0 and '1"
Collecting into Vec<String> before the await removes the
lifetime-bound closure from the future's state machine entirely.
Adds a variant of cleanup_expired_files that accepts an explicit FileIO,
for cases where the committed table's file_io lacks S3 configuration
(e.g. region) that was present on the original table.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 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.
Which issue does this PR close?
What changes are included in this PR?
Eagerly collect manifest list paths into Vec before the
await point in clean_files to avoid a higher-ranked trait bound
(HRTB) compiler error.
The closure
|s| s.to_string()on HashSet<&'a str> gets inferredwith a concrete lifetime that the compiler cannot generalize across
async generator boundaries, producing:
Collecting into Vec before the await removes the
lifetime-bound closure from the future's state machine entirely.
Adds a variant of cleanup_expired_files that accepts an explicit FileIO,
for cases where the committed table's file_io lacks S3 configuration
(e.g. region) that was present on the original table.