Skip to content

Fix all findings from the table-format reader code review (fable) - #182

Open
schenksj wants to merge 2 commits into
mainfrom
fix/table-format-reader-review
Open

Fix all findings from the table-format reader code review (fable)#182
schenksj wants to merge 2 commits into
mainfrom
fix/table-format-reader-review

Conversation

@schenksj

@schenksj schenksj commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Addresses all 26 findings from the table-format reader code review (docs/TABLE_FORMAT_READER_REVIEW.md, included in this PR with a per-finding resolution-status table). Every finding was independently re-validated against the code before fixing — all 26 confirmed.

High severity

  • H1/H2 — parquet multi-level partitions: getTableInfo predicate pruning evaluated the full predicate against first-level-only partition values, where missing-column semantics are "exclude" — a filter on a deeper column (e.g. month = '01' on a year/month table) silently pruned everything. Added PartitionPredicate::evaluate_partial() (Kleene three-valued logic, unknown → keep); executors re-apply the full predicate. partition_columns now discovers all levels by walking one directory chain (one LIST per level).
  • H3 — multi-part checkpoints: get_snapshot_info now scans all checkpoint parts for the metaData row instead of requiring it in part 1.
  • H4 — protocol validation: the protocol action is read and validated; unsupported readerFeatures (v2Checkpoint, unknown future features), minReaderVersion > 3, and columnMapping mode id are rejected with clear errors.
  • H5 — stale _last_checkpoint: after the sequential commit probe, one offset-bounded LIST detects newer checkpoints; the reader re-probes from the newest checkpoint and errors loudly on a broken commit chain.
  • H6 — compact mode correctness flags: compact serialization now always includes has_deletion_vector (Delta) and content_type (Iceberg). The TANT format is name-keyed, so Java parsing is backward compatible.
  • H7 — abfss URLs: container is parsed from the URL username (abfss://container@account.dfs...); account derived from host when not configured.
  • H8 — credential chain: S3/Azure builders start from from_env() (env vars, standard chains); explicit config still overrides.

Medium severity

  • M1: percent-decode Url::path() before building object keys (paths with spaces/unicode). Delta add.path values remain verbatim per spec; convention documented.
  • M2: malformed non-empty commit lines now fail the call with file + line number instead of being silently skipped.
  • M3: catalog-style credential keys (aws_access_key_id, region_name, …) are translated to FileIO keys for executor-side Iceberg manifest reads; vended-credential catalogs documented as requiring the catalog path.
  • M4: sequence_number captured on every Iceberg entry (TANT full mode, new 8th Arrow FFI column, IcebergFileEntry.getSequenceNumber()).
  • M5: unresolvable snapshot schema_id is now an error instead of a silent current_schema() fallback.
  • M6: resolved_snapshot_id embedded in every serialized entry; missing per-entry snapshot-id fallback unified to -1.
  • M7: runtime creation unified to new_current_thread() (shared-runtime/client caching left as a follow-up optimization).
  • M8: date/timestamp/decimal partition literals rendered human-readably (ISO dates, scaled decimals) using each manifest's partition result types, so string predicates like date = '2024-01-01' match.

Low severity

L1–L6 and L8–L10 fixed (hidden-file filters, -1 sentinels, dead field removal, helper dedup into common.rs, serialization debug_assert + clearer 2 GiB error, throw on config-extraction failure, clear out-of-range version error, table_version = -1 sentinel). L7 (exception taxonomy) deferred as a cross-layer API design change and documented.

Breaking change note

IcebergTableReader.readManifestFileArrowFfi now exports 8 columns (sequence_number appended); callers must allocate 8 FFI addresses. The column is appended last, so positional consumers of the first 7 stay aligned.

Test plan

  • ✅ 152/152 Rust unit tests across delta_reader, iceberg_reader, parquet_reader, parquet_schema_reader, common (~20 new tests: partial-predicate Kleene logic, protocol validation, percent-encoded log prefixes, typed literal rendering, compact-mode field presence)
  • ✅ 107/107 Java tests: DeltaTableReaderTest, DeltaDistributedScannerTest, DeltaStreamingPrimitivesTest, IcebergTableReaderTest, IcebergDistributedScannerTest, ParquetDistributedScannerTest, ParquetSchemaReaderTest

🤖 Generated with Claude Code

@schenksj @claude
readers: fix all findings from the table-format reader review
9bcff88 
Addresses the 26 findings in docs/TABLE_FORMAT_READER_REVIEW.md (included
with a resolution-status table). Highlights:

High severity:
- H1/H2: parquet getTableInfo prunes first-level partition dirs with new
  three-valued PartitionPredicate::evaluate_partial (unknown column -> keep),
  and discovers all partition levels by walking one directory chain
- H3: delta get_snapshot_info scans all checkpoint parts for the metaData row
- H4: read + validate the protocol action; reject unsupported readerFeatures
  (e.g. v2Checkpoint), minReaderVersion > 3, and columnMapping mode 'id'
- H5: stale _last_checkpoint guard via one offset-bounded LIST; re-probe from
  the newest checkpoint, error on a broken commit chain
- H6: compact serialization keeps has_deletion_vector (delta) and
  content_type (iceberg)
- H7: abfss:// container parsed from URL username; account derived from host
- H8: S3/Azure builders start from from_env() so env/IMDS credential chains
  work; explicit config still overrides

Medium severity:
- M1: percent-decode Url::path() before building object keys (shared decoder
  in common.rs)
- M2: malformed non-empty commit lines fail loudly with file + line number
- M3: translate catalog-style credential keys to FileIO keys for
  executor-side iceberg manifest reads
- M4: capture sequence_number on iceberg entries (TANT, Arrow FFI col 8,
  Java getter)
- M5: error instead of silent current_schema() fallback on unresolvable
  snapshot schema_id
- M6: embed resolved_snapshot_id in every serialized iceberg entry; unify
  missing-snapshot-id fallback to -1
- M7: unify runtime creation to new_current_thread()
- M8: render date/timestamp/decimal partition literals human-readably using
  each manifest's partition result types

Low severity: L1-L6, L8-L10 fixed; L7 (exception taxonomy) deferred and
documented.

Tests: 152/152 Rust unit tests (incl. ~20 new); 107/107 Java tests across
the 7 Delta/Iceberg/Parquet reader test classes.

Note: IcebergTableReader.readManifestFileArrowFfi now exports 8 columns
(sequence_number appended); callers must allocate 8 FFI addresses.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 5, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude encountered an error —— View job

I'll analyze this and get back to you.

@schenksj @claude
readers: fix bugs found in PR #182 review
4c8013e 
Addresses 6 findings from an independent code review of PR #182
(fix all findings from the table-format reader review):

- delta_reader: read_metadata_from_checkpoint no longer assumes metaData
  precedes protocol in the projected Arrow batch; ProjectionMask::roots()
  returns columns in physical schema order, which the Delta spec does not
  guarantee, so column identity is now derived from each field's sorted
  projection index instead.
- delta_reader: the stale-_last_checkpoint guard (H5) now detects a second,
  deeper log gap beyond the newly-adopted checkpoint instead of silently
  discarding the already-known max_commit_version and returning a
  still-stale snapshot.
- delta_reader: a missing protocol row in a checkpoint is now a hard error
  (like metaData) instead of a debug-only skip, since H4's reader-feature
  validation must not be silently bypassed. Updated the checkpoint test
  helper to include a protocol column so the enforcement path is covered.
- parquet_reader: multi-level partition-column discovery now walks every
  first-level partition directory instead of only the first, so sibling
  partitions with a deeper structure (schema drift, partial backfill) are
  no longer under-reported.
- iceberg_reader: threaded an inherited_snapshot_id parameter through
  read_iceberg_manifest / read_manifest_with_file_io / the Arrow FFI path
  and IcebergTableReader's Java API (additive overloads) so getChangesSince
  passes ManifestFileInfo's known added_snapshot_id, resolving the same
  fallback as listFiles() instead of always defaulting to -1.
- parquet_reader: deduplicated the 4x-copied hidden-file filename check
  into a single is_hidden_parquet_file() helper.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 5, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude encountered an error —— View job

I'll analyze this and get back to you.

@schenksj schenksj changed the title Fix all findings from the table-format reader code review Fix all findings from the table-format reader code review (fable) Jul 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@schenksj