[Feature](lance) Support nested Arrow Null types - #67921
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for three issues: FE schema discovery rejects deeper valid Null-bearing composites; non-nullable Null children can reach the UInt8/Boolean physical SerDe and crash BE materialization; and the canonical Lance fixture rebuild removes the new nested-null dataset.
Critical checkpoints: logical NULL protobuf round-tripping is sound; valid nullable NA slices, empty/null parents, destination-tail appends, scanner batching, and row-id materialization are aligned; recursive smooth-upgrade fencing covers catalog and TVF projections; no performance or observability regression was substantiated. Test gaps are the deeper FE shapes and non-nullable Null children, while fixture generation is not integrated into the authoritative staged rebuild. No additional user focus was provided.
This was a two-round converged static review. Per the runner contract, I did not build, run tests, or edit source files, so test claims were inspected but not independently executed.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated static review for 027201d
Approval opinion: no new actionable findings at this head. The three previously reported issues are statically verified fixed and duplicate-fenced.
Critical checkpoints:
- Schema and RPC identity: nullable logical Null is preserved recursively through FE types, Thrift/protobuf, and BE reconstruction; non-nullable Null leaves are rejected; unrelated unsupported descendants still propagate as unsupported.
- Materialization and lifecycle: Arrow NA slices append the exact logical range, nested ARRAY/MAP/STRUCT child alignment and null parents remain consistent, and no cleanup, I/O, or ownership path changed.
- Compatibility and dispatch: catalog and TVF projections fence smooth-upgrade source BEs for Null-bearing roots; the canonical fixture builder checks and publishes the dataset at the regression URI.
- Performance and observability: the production changes are schema-time checks plus linear default insertion, with no new I/O, cache, predicate, or observability regression identified.
Residual non-blocking test note: the SQL golden does not assert every nested descendant value end to end through lance-c, although the persisted fixture self-check and focused BE tests cover those values at adjacent layers.
No additional user review focus was provided. This was a two-round converged static review. Per the runner contract, I did not build, run tests, or edit source files; author-reported focused test and fixture-rebuild results were inspected but not independently executed.
|
run buildall |
|
/review |
|
Fixed the new external regression failure in 68cf7d8. The nested_null_projection golden expected uppercase NULL array elements, but complex-value serialization returns lowercase null. Updated the expectation and verified all three projection rows against the recorded CI output. Schema checks and nested_null_values completed before this mismatch. No production code changed. |
There was a problem hiding this comment.
Requesting changes for two distinct rolling-upgrade safety gaps in generic file() Lance scans: the S3-compatible delegate loses its projected-column current-reader capability, while the Local delegate loses its schema-backend execution affinity. Either route can send a newly supported nested Null-bearing root to a smooth-upgrade source BE whose old nullable SerDe can dereference a failed Arrow Boolean cast. The three earlier inline issues are fixed at this head and remain duplicate-fenced.
Critical checkpoints: recursive FE/BE schema identity, non-nullable Null rejection, Arrow NA range/materialization alignment, row-ID phase-two state, direct catalog/S3/search compatibility fencing, direct Local pinning, fixture staging/promotion, committed binary identity, MinIO publication, SQL oracles, lifecycle, performance, and observability were all inspected. No additional defect was substantiated outside the two generic-wrapper contracts.
This was a three-round converged static review. Per the review contract, I did not build, run tests, regenerate fixtures, or edit source files; author/CI test claims were inspected but not independently executed.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
There was a problem hiding this comment.
One blocking rolling-upgrade issue remains: TopN lazy materialization for vector_search() and full_text_search() can prune a projected nested-Null root before the current-reader compatibility check, then route phase-two take_rows back to the old source BE. The inline finding details the resulting crash path.
Critical checkpoints:
- Schema and materialization: recursive LIST/MAP/STRUCT mapping, nullable versus rejected Null leaves, sliced parent offsets/null maps, and Arrow NA ranges were traced; no additional defect was found.
- RPC and type identity: nested NULL_TYPE protobuf nodes preserve preorder shape and nullability without reclassifying BOOLEAN or
DataTypeNothing; existing old consumers understand the stable enum. - Scheduling and lifecycle: direct S3, generic
file(), and Local schema-backend affinity are otherwise consistent, and existing resolved threads were duplicate-fenced. - External compatibility and tests: the pinned external-writer fixture, staged rebuild/promotion, MinIO publication path, and C++/FE/regression coverage are aligned; the missing mixed-version lazy-search case is the blocking gap.
- Performance and observability: no distinct regression was substantiated in the changed paths.
No additional user review focus was provided. This review completed after two convergence rounds. It was static only: per the task contract I did not run builds or tests, so author/CI execution claims were inspected but not independently verified.
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes for one distinct fixture-integrity issue: the standalone nested-Null generator bypasses the pinned-writer and assertions-enabled safeguards required by the canonical fixture workflow, so it can self-certify a candidate binary fixture that other supported Lance readers cannot open or omit the schema/value equality check under optimized Python.
Critical checkpoints:
- Schema and materialization: recursive LIST/MAP/STRUCT logical-NULL identity, required-Null rejection, Arrow NA range/default insertion, nested offsets/null maps, sliced parents, and row-ID materialization are consistent.
- Scheduling and compatibility: direct and generic S3/Local TVFs preserve current-reader metadata or backend affinity, while lazy vector/full-text phase two carries deferred source names into the phase-one backend fence.
- Fixture lifecycle and tests: the canonical pinned staged rebuild, pre/post-promotion validation, complete-prefix upload, committed binary layout, and BE/FE/SQL coverage are aligned; only the helper's separate CLI lacks those guards.
- Lifecycle, performance, and observability: no additional correctness, cleanup, I/O, distribution, performance, or observability regression was substantiated.
All six earlier inline issues are statically verified fixed at this exact head and were treated as hard duplicate fences. No additional user review focus was provided. This was a two-round converged static review. Per the review contract, I did not build, run tests, regenerate fixtures, or edit source files; author/CI execution claims were inspected but not independently verified.
FE UT Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
FE Regression Coverage ReportIncrement line coverage |
3 participants
What problem does this PR solve?
Follow-up to #67325; partially addresses #66496.
Lance columns containing Arrow Null inside lists, structs, or map values are currently reported as unsupported. This change maps nullable Null leaves recursively, including deeper list/map/struct combinations, materializes them without accessing nonexistent Arrow validity/value buffers, and preserves their logical
NULL_TYPEthrough nested schema RPC serialization.Array offsets, parent nullability, empty collections, and sliced inputs remain intact. Unknown extensions and non-nullable Null leaves are rejected before materialization.
Generic
file()scans preserve the delegate’s reader compatibility requirements and Local schema-backend affinity during rolling upgrades.Search TVFs retain reader compatibility requirements for deferred columns across TopN lazy materialization. Both read phases are checked without adding deferred fields back to the first-phase tuple.
Tests
mvn checkstyle:check, clang-format 16 checks, andgit diff --checkpassed.--repin(one existing HNSW discriminator changes on retraining), and the check rejects a missing nested-null dataset.Full FE Maven tests are blocked locally by a Thrift compiler/runtime version mismatch (installed compiler 0.16 versus branch dependency 0.24). The complete BE suite and S3 regression have not been run locally and require CI.
Release note
Support Arrow Null elements and fields nested in Lance arrays, structs, and map values.