docs: doc the requirement for ParquetHandler to do type widening - #3313
docs: doc the requirement for ParquetHandler to do type widening#3313dengsh12 wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3313 +/- ##
=======================================
Coverage 90.34% 90.34%
=======================================
Files 250 250
Lines 89131 89131
Branches 89131 89131
=======================================
Hits 80524 80524
Misses 5672 5672
Partials 2935 2935 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| /// truncated). The default engine does this via `arrow::compute::cast` while reordering columns | ||
| /// to the requested schema. |
There was a problem hiding this comment.
nit:
| /// truncated). The default engine does this via `arrow::compute::cast` while reordering columns | |
| /// to the requested schema. | |
| /// truncated). The default engine does this via `arrow::compute::cast`. |
| /// When `physical_schema` requests a type different from the matched Parquet column, the reader | ||
| /// must coerce it to the requested type when the conversion is allowed by the [Delta protocol | ||
| /// type widening rules]. Examples include `INTEGER` -> `LONG`, `FLOAT` -> `DOUBLE`, and `DATE` | ||
| /// -> `TIMESTAMP_NTZ`. See the protocol for the complete list. |
There was a problem hiding this comment.
DefaultEngine does this, but some engine has not implemented this -- will inform them
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
No blocking issues. This is a docs-only change to the ParquetHandler coercion contract. The protocol reviewer verified the type-widening examples (INTEGER -> LONG, FLOAT -> DOUBLE, DATE -> TIMESTAMP_NTZ) against PROTOCOL.md, and the docs reviewer confirmed each example and the INT32 -> DATE / INT64 -> TIMESTAMP/TIMESTAMP_NTZ checkpoint conversions map to real branches in check_cast_compat in kernel/src/engine/ensure_data_types.rs. The DATE -> TIMESTAMP_NTZ example correctly excludes the timezone-bearing TIMESTAMP variant, and the microsecond normalization text matches the protocol.
Summary
The documentation accurately reflects both the Delta protocol and the default engine's coercion behavior, and the change is safe to merge. Coverage note: the maintainer-codex, delta-protocol, and docs reviewers completed and reached quorum; the maintainer-claude reviewer did not return within the review window, so its pass is not reflected here.
Automated review - workflow run
| /// requested schema. | ||
| /// When `physical_schema` requests a type different from the matched Parquet column, the reader | ||
| /// must coerce it to the requested type when the conversion is allowed by the [Delta protocol | ||
| /// type widening rules]. Examples include `INTEGER` -> `LONG`, `FLOAT` -> `DOUBLE`, and `DATE` |
There was a problem hiding this comment.
Nit1 The examples INTEGER -> LONG, FLOAT -> DOUBLE, and DATE -> TIMESTAMP_NTZ sit directly under the type widening rules link, while the next paragraph lists INT32 -> DATE and INT64 -> TIMESTAMP/TIMESTAMP_NTZ as extra Kernel requirements. A reader could infer these coercions are gated on the type-widening table feature rather than being reader-side coercions needed to materialize the requested physical_schema. Raised by: maintainer-codex-reviewer. Suggested fix: add a short clause noting these coercions are required whenever physical_schema asks for a compatible type and are not conditional on the type-widening feature being enabled.
Benchmark results: ✅ PassSummary: 🚀 0 · ✅ 3 · ☑️ 2 · 🚧 10 · ❌ 0 Per-benchmark results (15 rows)
Legend: 🚀 ≥1.15x faster · ✅ faster or unchanged · ☑️ ≤1.03x slower · 🚧 1.03x-1.15x slower · ❌ ≥1.15x slower |
3 participants
What changes are proposed in this pull request?
doc the type widening expectation for
ParquetHandlerimplresolves #1612
How was this change tested?
doc only