add leaf reader conversion code - #3355
Draft
emkornfield wants to merge 17 commits into
Draft
emkornfield wants to merge 17 commits into
emkornfield wants to merge 17 commits into
Conversation
Migrate branch code to upstream API changes pulled from main: - Drop the removed `IntoEngineData` derive (unused here; conversions go through `struct_expr_from_schema`). Kept the test-only `IntoStructData`. - Update test helpers for `create_many`'s owned `Vec<Vec<Scalar>>` signature. - Construct `location`/`file_size_in_bytes` as required (non-Option) fields. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
…ta for both Reconciles the concurrent builder.rs refactor with the content-tree read path and returns FilteredEngineData from both AMT conversion functions for symmetry: - convert_append_metadata_to_entry_batch -> FilteredEngineData (all rows selected) - convert_root_entries_to_add_actions -> FilteredEngineData (live Data entries selected) De-duplicates struct_expr_from_schema (shared in content_tree/mod.rs). Co-authored-by: Isaac <no-reply@databricks.com>
The read-path WIP had flipped ContentTreeNodeEntry.location and file_size_in_bytes to Option, but main's delta-io#3304 makes both required and the schema contract test asserts non-nullable. Restore the required types and: - Rework null_file_size_becomes_zero to inject the null via a nullable-size input schema at the batch level (the reader still coalesces null size -> 0). - Update create_many test helpers for the owned Vec<Vec<Scalar>> signature. Co-authored-by: Isaac <no-reply@databricks.com>
…n-dev MODIFICATION_TIME_NAME and DATA_CHANGE_NAME are consumed only by the AMT read path (content_tree::reader), which is gated on adaptive-metadata-in-dev. The constants themselves were unconditional, so a default `cargo build` triggered dead-code errors under -D warnings. Co-authored-by: Isaac <no-reply@databricks.com>
Generalize the AMT content-tree builder from root-only to also produce leaf-manifest `Data` entries via a private `ManifestKind` enum threaded through a shared impl. Add `convert_append_metadata_to_leaf_entry_batch` alongside the unchanged root entry point. Leaf entries still write `snapshotId` but leave `sequenceNumber`, `fileSequenceNumber`, and `firstRowId` null so they are inherited/assigned from the parent manifest entry; the leaf required-non-null set drops to `stats.numRecords` only. Tests are parameterized over root/leaf. Co-authored-by: Isaac <no-reply@databricks.com>
Address PR review: instead of a `ManifestKind { Root, Leaf }` enum matched
in several places, capture the per-manifest-level differences (required
non-null columns + the sequenceNumber/firstRowId source expressions) in an
`AppendEntrySpec` struct with `root()`/`leaf()` constructors, consumed by a
single branch-free impl. Trim the module docstring to the high-level
description.
Apply the same data-driven pattern to tests: an `EntryPath { convert,
expected_versions }` struct (function pointers) drives the rstest cases in
place of matching on the enum.
Co-authored-by: Isaac <no-reply@databricks.com>
|
PR title does not match the required pattern. Please ensure you follow the conventional commits spec. Your title should start with Title: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3355 +/- ##
==========================================
+ Coverage 90.51% 90.55% +0.04%
==========================================
Files 255 257 +2
Lines 91213 92081 +868
Branches 91213 92081 +868
==========================================
+ Hits 82558 83387 +829
- Misses 5642 5653 +11
- Partials 3013 3041 +28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
1 participant
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.
🥞 Stacked PR
Use this link to review incremental changes.
What changes are proposed in this pull request?
How was this change tested?