[fix](adbc) Materialize Arrow list view arrays - #66819
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review result: request changes
The review converged after two rounds, each with full-scope and risk-focused review coverage. Three blocking findings are covered by two inline comments:
- Arrow 24's
FromListViewmisreads validity for nullable nonzero-offset slices, which can silently flip valid/null rows. - Malformed list-view ranges are copied before Doris's default-on Arrow validation runs, allowing an out-of-bounds child-buffer read instead of a clean error.
- Canonicalization allocates a potentially amplified full logical-size intermediate from Arrow's untracked default pool, bypassing query memory limits while Doris copies the values again.
Critical checkpoint conclusions:
- Goal and correctness: the offset-zero reordered/shared-range cases work for both widths, but the valid-slice corruption and pre-validation read mean the ADBC materialization goal is not safely complete.
- Scope and parallel paths: the change is small and localized, and both
LIST_VIEWandLARGE_LIST_VIEWare wired consistently. The same three mechanisms affect both widths and both query/partition streams that feed the shared ADBC materialization path. - Concurrency and lifecycle: normalization is synchronous and per-call, with no new shared mutable state, locks, threads, atomics, static initialization dependency, or ownership cycle. An explicit caller-supplied ExecEnv Arrow pool is lifecycle-safe here because the temporary is destroyed before the synchronous materialization call returns.
- Configuration and error handling: no new configuration is added and both Arrow
Results are checked, but the existing default-onenable_arrow_input_validationgate currently runs only after this conversion, which is too late for malformed ranges. - Compatibility, persistence, and writes: there is no FE-BE protocol, storage format, persisted metadata, transaction, data-write, failover, or rolling-upgrade change.
- Performance and memory: conversion is O(rows + logical child values), but shared ranges can greatly amplify the intermediate. Keeping that buffer live through the second Doris copy makes tracked allocation, or direct SerDe materialization, necessary.
- Tests and results: the new unit tests are registered and their expected reordered values are correct, but they omit nullable slices, malformed ranges, pool accounting, and a ListView-through-
AdbcFileReadercase. The review contract forbids local builds; the PR reports ASAN normalizer/ADBC unit runs. Clang Formatter passed, while compile and BE UT were still pending at review time. The external performance check failed without accessible evidence tying it to this patch. - Observability: the existing
AdbcNormalizeTimeprofile timer covers the new CPU work; no additional log or metric issue remains once memory accounting is fixed. - Additional user focus: none was provided; the complete PR was reviewed.
- Dismissed scope point: nested encoding-only children remain a documented pre-existing top-level-only limitation and still fail loudly, so no duplicate changed-code finding was submitted for that case.
Review completion: complete and converged. Every candidate is accepted in the inline set or dismissed with concrete evidence; no unresolved suspicious point remains.
| // List views may share or reorder value ranges, so rebuild canonical offsets instead of | ||
| // exposing their buffers to a serde that requires contiguous list values. | ||
| if (type.id() == arrow::Type::LIST_VIEW) { | ||
| auto converted = arrow::ListArray::FromListView( |
There was a problem hiding this comment.
[P1] Avoid this unsafe Arrow 24 conversion path
This API is unsafe for two inputs ADBC can deliver. On a nullable slice, Arrow 24 calls GetValues<uint8_t>(0) (which already advances by ArrayData::offset bytes) and then GetBit(..., offset + i), so a nonzero offset reads the wrong validity bit and can silently flip null rows. On a malformed range, ListFromListViewImpl calls AppendArraySlice(values, offset, size) before Doris's default-on SerDe validation, and the numeric builder copies without checking offset + size <= values.length(), allowing an out-of-bounds read. C-stream import preserves child offsets and does not ValidateFull. Please validate/rebase the source or use a corrected conversion, and add nullable-slice and out-of-range tests for both widths.
| if (type.id() == arrow::Type::LIST_VIEW) { | ||
| auto converted = arrow::ListArray::FromListView( | ||
| static_cast<const arrow::ListViewArray&>(*current), | ||
| arrow::default_memory_pool()); |
There was a problem hiding this comment.
[P1] Route the canonicalization allocation through Doris tracking
FromListView computes SumOfLogicalListSizes, reserves that full child capacity, and copies each logical range from this pool. With N rows sharing an M-element range, a small M-element physical child creates an NM intermediate, which remains live while the Doris array SerDe copies NM values again. Arrow's default system pool is not Doris's ArrowMemoryPool, and no repository override connects it to the thread/query MemTracker, so a large batch bypasses query limits and can exhaust process memory. Please pass an explicit caller-owned tracked pool (for example, the ADBC runtime's ExecEnv pool, whose lifetime encloses out) for both widths; direct SerDe materialization could also remove the extra peak.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review result: request changes
The review converged after two rounds of full-scope and risk-focused coverage. The current head fixes the two prior inline threads by validating ListView ranges before copying, preserving sliced validity correctly, and routing all conversion allocations through a caller-owned Doris pool. Two residual blocking findings are covered by the new inline comments:
- Aggregate logical length is not checked before Arrow 24 child builders copy shared ranges, allowing signed overflow before the parent list builder's late capacity check; nested view builders require the same protection.
- The manual conversion reserves only parent rows, omitting the exact child reserve used by Arrow's conversion and creating an avoidable geometric tracked-memory peak that can reject a valid batch.
Critical checkpoint conclusions:
- Goal and correctness: valid ordinary ListView/LargeListView inputs now preserve logical order, nulls, slices, and malformed-range failure, but the unchecked aggregate and allocation-growth cases mean safe ADBC materialization is not complete.
- Scope and focus: the production change is localized to the normalizer and its sole ADBC caller. The signature change reaches every caller, and no unrelated source change is present.
- Concurrency and lifecycle: normalization state is per call. Concurrent readers share only atomic pool accounting; there is no new lock order or mutable reader state. The process pool outlives outputs, and the fallback pool is destroyed only after each normalized array is synchronously copied and released.
- Error handling: validation, reserve, append, finish, cast, and SerDe statuses are consumed, and existing exception boundaries convert Doris allocation failures. The remaining signed overflow occurs before those status checks can protect the path.
- Configuration and compatibility: no configuration, protocol, storage-format, persisted-state, transaction, data-write, FE/BE variable, failover, or rolling-upgrade surface changes.
- Parallel paths: Remote Doris, Paimon/C Data, IPC, and generic Arrow materializers do not use this ADBC-specific helper and have distinct producer contracts; no parallel-path update is required.
- Performance and memory: the supplied pool repairs untracked allocation, but exact checked child reservation is still needed to avoid the new manual loop's false memory-limit failure.
- Tests and results: changed tests cover both offset widths, reordered/shared ranges, nullable slices, invalid individual ranges, caller-pool use, and 32-bit ADBC-to-Doris materialization. They omit the no-allocation aggregate-overflow and geometric-growth regressions in the inline findings. Per the review contract, no local build or test was run. Clang Formatter, CheckStyle, license, and related static checks pass. macOS BE UT fails during unrelated CMake configuration on a missing unity-skip source before reaching changed code; TeamCity BE UT exposes no accessible PR-specific evidence, while compile and performance checks remain pending.
- Observability: the existing AdbcNormalizeTime profile timer covers the added CPU work; no extra log or metric issue was established.
- Existing-thread and scope suppression: neither new comment repeats the fixed unsafe-FromListView or untracked-default-pool threads. The documented nested encoding-only SerDe limitation was previously dismissed and is not re-raised; nested builders are mentioned only where they reproduce the accepted pre-SerDe overflow mechanism.
- Additional user focus: none was provided; the complete authoritative diff was reviewed.
Review completion: complete and converged. Every candidate is accepted in the two-comment inline set, merged into those findings, or dismissed with evidence; no unresolved suspicious point remains.
| } | ||
| std::shared_ptr<arrow::ArrayBuilder> child_builder(child_builder_result.MoveValueUnsafe()); | ||
| ListBuilder builder(pool, child_builder); | ||
| auto reserve_status = builder.Reserve(source.length()); |
There was a problem hiding this comment.
[P1] Reserve the expanded child before the append loop
Only the parent slots are reserved, so the child grows geometrically while shared ListView ranges are expanded. For example, 1,025 rows viewing the same 100,000 Int32 values produce a roughly 391 MiB value buffer, but Arrow's final doubling reallocates it from about 391 MiB to 781 MiB; Doris charges the new allocation before releasing the old one, transiently accounting roughly 1.17 GiB for that buffer alone. An exact reserve keeps the canonical and Doris value buffers near 782 MiB combined, leaving roughly 391 MiB less required headroom (other common query allocations aside). After validation, please compute a checked non-null logical total and reserve it on child_builder before this loop.
| if (!append_status.ok()) { | ||
| return append_status; | ||
| } | ||
| append_status = child_builder->AppendArraySlice(values, source.value_offset(i), |
There was a problem hiding this comment.
[P1] Check aggregate lengths before copying into builders
A valid LargeListView<Null> can reach signed overflow here without allocating a huge buffer: use a Null child of length INT64_MAX and two shared ranges of length INT64_MAX - 1. ValidateFull() accepts both ranges independently, but Arrow 24's NullBuilder::AppendArraySlice performs unchecked length_ += length, so the second append overflows before LargeListBuilder checks capacity. Checking only this source's top-level total is not enough either: an outer large_list_view<large_list_view<null>> can have total 2 while the nested builder overflows on the same ranges during this call. Please compute checked non-null logical totals before any append at every copied view level, or reject nested view children before building, and add a no-allocation regression test.
|
Codex automated review failed and did not complete. Error: This content was flagged for possible cybersecurity risk. If this seems wrong, try rephrasing your request. To get authorized for security work, join the Trusted Access for Cyber program: https://chatgpt.com/cyber Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
…count probe (FE) (#66831) ### What Two independent breakages that each make **current master fail to build** — one in BE configure, one in FE compile. They share a shape: a pair of PRs that never conflict textually, merge cleanly, and only break once combined, so each PR's own pipeline was green. | | Breakage | Colliding PRs | |---|---|---| | BE | `cmake` configure aborts | #66052 moved a file, #66789 made a dangling unity-skip entry fail loud | | FE | `fe-connector-iceberg` does not compile | #66778 deleted `getCountFromSnapshot()`, #66413 added a caller for it | CI merges each PR into the latest master before building, so **every PR pipeline that picks up current master is red** on one or both. --- ## 1. BE — stale `STORAGE_UNITY_SKIP` entry for a moved file Remove the stale `STORAGE_UNITY_SKIP` entry (and its comment block) for `compaction/collection_statistics.cpp`, which no longer exists. ### Why — master configure is currently broken #66052 moved `storage/compaction/collection_statistics.{cpp,h}` to `storage/index/inverted/similarity/` (rewritten), but left behind the unity-skip entry that #66789 had added for the old path. The fail-loud validation introduced by #66789 turns a dangling skip entry into a configure-time error — which is exactly what it is designed to catch (a skip list rotting after a file move), so BE configure on current master fails immediately: ``` CMake Error at CMakeLists.txt:1002 (message): unity skip entry does not exist (renamed or moved?): .../be/src/storage/compaction/collection_statistics.cpp ``` #66826, #66824, #66819, #66820 were the first hits — same error on multiple independent agents. ### Why deletion (not a path update) is correct The old entry existed because the old `collection_statistics_test` `#include`d the `.cpp` into a second TU (unity batching would then produce a duplicate definition at link time). The rewritten file at the new location is not `#include`d by any test (`grep -rn 'collection_statistics.cpp' be/test/` is empty on master), so the new path needs no skip entry. ### Verification - Full BE build from a clean tree at master + this change (clang20 / macOS arm64, unity=ON, PCH=ON): configure passes the skip-list validation and the build compiles. (The same tree without this change fails configure with the error above.) - Timeline note: #66052's last green CI round presumably predates #66789's validation landing (2026-08-16), which is how the dangling entry slipped through. --- ## 2. FE — the metadata-only COUNT(\*) probe calls a deleted method #66778 replaced the snapshot-summary COUNT(\*) pushdown with a manifest-derived count and deleted `getCountFromSnapshot()`. `canServeMetadataOnlyCount()`, added by #66413, still calls it, so FE compilation fails: ``` [ERROR] .../connector/iceberg/IcebergScanPlanProvider.java:[505,16] cannot find symbol [ERROR] symbol: method getCountFromSnapshot(org.apache.iceberg.TableScan,org.apache.doris.connector.spi.ConnectorSession) [ERROR] location: class org.apache.doris.connector.iceberg.IcebergScanPlanProvider ``` ### Why re-express the probe instead of restoring the method Bringing `getCountFromSnapshot()` back would reintroduce precisely what #66778 removed: a query result derived from optional, writer-provided snapshot summary fields. The probe is rebuilt in #66778's own terms instead. It reuses that PR's delete gate and additionally requires the data manifests to carry aggregate row counters, so the answer is proved from the manifest list alone — `O(manifests)`, no data-file enumeration, which is what a probe running before planning can afford. Manifest lists that omit those aggregates now answer `false`, where count planning still serves them through its bounded per-file fallback. A capability probe should under-promise rather than over-promise; the planner itself is untouched. ### Verification - `mvn package` over the full FE reactor (74 modules): all green. This also confirms no second semantic break is hiding behind the first — CI's maven stops at `fe-connector-iceberg` and never reaches the rest. - `IcebergScanPlanProviderTest`: 153 tests, 0 failures, including a new case pinning that the probe follows the same delete gate as count planning. - FE checkstyle: 0 violations. ### Release note None (both fixes only restore a buildable master; no user-visible behavior change). ### Check List (For Author) - Test - [x] Regression test — not applicable: neither change alters query behavior - [x] Unit test: `IcebergScanPlanProviderTest` (153 tests, 0 failures) - [x] Manual test: full BE configure/build and full FE reactor build - Behavior changed: No - Does this need documentation: No --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
ab92556 to
1043bd1
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: one new, non-duplicate P1 correctness finding is attached inline. A nullable runtime ListView can silently become [] when the FE-planned ARRAY slot is required. Existing threads already cover malformed/sliced conversion, tracked allocation, expanded-child reservation, and aggregate capacity; I did not repeat them.
Critical checkpoint conclusions:
- Goal, scope, and tests: The focused normalizer implements top-level ListView/LargeListView canonicalization, and the added BE unit tests correctly cover logical order, both offset widths, sliced validity, malformed ranges, pool routing, nested-view rejection, and one nullable ADBC happy path. They do not cover the required-root mismatch in the inline finding.
- Correctness, nullability, and error handling: Supported nullable shapes preserve row count, order, child values, and parent validity, and Arrow/SerDe errors are propagated. The accepted finding is the remaining silent-error path: a valid runtime null loses its validity bit in a non-nullable ARRAY SerDe and becomes an empty value.
- Memory, lifecycle, and performance: The process pool outlives readers; fallback-pool locals outlive every normalized array; SerDes retain no Arrow buffers; allocations remain tracker-charged and unwind on all exits. The two remaining builder capacity/reservation concerns are already covered by live inline threads and were duplicate-suppressed here.
- Concurrency and parallel paths: Builders and normalized arrays are call-local, the shared pool's statistics are atomic, and no new locking or shared mutable state is introduced. Ordinary List/LargeList materialization exposes the same target-nullability boundary, so the fix belongs at the shared ADBC materialization check.
- Compatibility, configuration, and persistence: No configuration, FE-BE protocol, storage format, transaction, persistence, or data-write behavior changes. This is a read-path encoding compatibility change only.
- Observability: Existing ADBC normalization/materialization timers cover the new work; failures already propagate with Arrow type context, and the proposed guard should add the column context. No additional metric or log is needed for the accepted fix.
- Test execution: Review-only instructions prohibited local builds and tests. The test code and expected assertions were reviewed; at submission time the macOS BE UT check is still in progress.
- User focus: No additional user-provided review focus was supplied.
Completion: converged after two rounds. Every subagent returned NO_NEW_VALUABLE_FINDINGS; all candidates were independently verified and either accepted, duplicate-suppressed, or dismissed with code evidence. This review is complete.
| { | ||
| SCOPED_TIMER(_normalize_time); | ||
| RETURN_IF_ERROR(normalize_arrow_array(batch.column(arrow_idx), &array)); | ||
| RETURN_IF_ERROR(normalize_arrow_array(batch.column(arrow_idx), arrow_pool, &array)); |
There was a problem hiding this comment.
[P1] Reject null ListView rows for required ARRAY slots
The file schema here is synthesized from cached FE slots, not the imported batch schema. If the source changes this field from required to nullable while the 10-minute ADBC schema cache is live, the runtime batch can validly contain a null ListView while target_type is still non-nullable. Normalization preserves that null, but _materialize_arrow_column then calls DataTypeArraySerDe directly; it consumes only offsets, so the repeated offset for the null row is silently materialized as []. Before this change the same ListView failed as unsupported. Please reject array->null_count() > 0 for non-nullable targets before SerDe (with column context), and add an end-to-end required ARRAY test containing a null ListView row (and LargeListView if both remain supported).
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review result: request changes
The review converged after three rounds of full-scope and risk-focused coverage. Three distinct P1 silent-result-corruption paths are attached inline:
- ADBC ignores the mapper's partial nested projection, so a requested Struct child can receive a different physical child.
- Cached and runtime recursive schemas are not reconciled, so same-width child-type drift can be reinterpreted as the cached type and reordered Struct fields can be swapped by ordinal.
- A dictionary-encoded numeric child survives ListView canonicalization and reaches the leaf SerDe as dictionary indices rather than logical values.
Critical checkpoint conclusions:
- Goal and correctness: basic ListView/LargeListView ordering, nulls, slices, malformed ranges, tracked allocation, and required outer-null rejection are covered, but the three inline corruption paths mean safe ADBC materialization is not complete.
- Scope and parallel paths: the five-file patch is focused and all internal signature callers are updated. Both ListView widths share the same builder and ADBC paths and the same findings.
- Concurrency and lifecycle: no new shared mutable reader state, lock order, race, or deadlock was found. The fallback pool outlives loop-local normalized arrays, the ExecEnv pool has process lifetime, and EOF, cancellation, close, and per-batch cleanup remain intact.
- Error handling and observability: Arrow and SerDe statuses propagate through the scanner exception boundary, and normalization has a dedicated timer. The accepted issues are silent value corruption before any useful error can be raised.
- Configuration, compatibility, persistence, and writes: there is no configuration, FE-BE protocol, storage-format, persisted-state, transaction, data-write, failover, or rolling-upgrade change.
- Performance and memory: conversion allocations are Doris-tracked. Exact capacity and recursive child-reservation residuals are already covered by existing live threads and were not duplicated.
- Tests and results: changed tests cover ordinary and reordered ranges, nullable slices, invalid ranges, both offset widths, tracked allocation, direct expansion, nested-view rejection, and the required outer-null case. They omit TableReader nested projection, recursive cached/runtime schema drift, and dictionary-encoded children. Per the review contract, no local build or test was run. Clang Formatter, CheckStyle, license, and related static checks pass; BE UT, macOS BE UT, compile, and performance checks are pending.
- Existing-thread suppression: the five live findings about unsafe conversion, pool tracking, expanded-child reservation, aggregate capacity, and required outer ARRAY nulls were not repeated. A proposed required nested Struct-null issue was dismissed after proving production ADBC metadata makes ARRAY elements and STRUCT fields nullable.
- Additional user focus: none was supplied; the complete authoritative diff was reviewed.
Completion: complete and converged. Every candidate was independently rechecked and accepted, duplicate-suppressed, or dismissed with code evidence; the final round produced no new valuable finding.
| { | ||
| SCOPED_TIMER(_normalize_time); | ||
| RETURN_IF_ERROR(normalize_arrow_array(batch.column(arrow_idx), &array)); | ||
| RETURN_IF_ERROR(normalize_arrow_array(batch.column(arrow_idx), arrow_pool, &array)); |
There was a problem hiding this comment.
[P1] Honor the mapper's nested projection
ADBC exposes synthesized complex children and inherits the default TableColumnMapper, so a query may legitimately produce a partial LocalColumnIndex. TableReader then builds the file-block type from that projected subtree, but this loop checks only the root id and passes the complete normalized Arrow root to SerDe. For list_view<struct<a,b>> projected to only b, the target is ARRAY<STRUCT<b>> while Arrow still contains {a,b}; in release builds DataTypeStructSerDe iterates the one target child and reads source field(0), placing a into b (the arity check is only a DCHECK). Either force full complex scan projection with MaterializedColumnMapper and let TableReader rematerialize, or project/reorder the Arrow value from the request. Please add a TableReader-level ListView Struct-subfield projection test.
| auto& columns = columns_guard.mutable_columns(); | ||
| const auto& target_type = columns_guard.get_datatype_by_position(block_position.value()); | ||
|
|
||
| // The cached FE target remains authoritative when a source schema changes mid-cache-window; |
There was a problem hiding this comment.
[P1] Validate the recursive runtime type before SerDe
The cached FE target and runtime Arrow batch are matched only by the top-level column name. If an ARRAY<INT> source changes to ListView<Float32> during the cache window, canonicalization produces List<Float32> and this guard passes; the target Int32 SerDe then takes its generic fixed-width path and reinterprets the float buffer (1.0f becomes 1065353216) instead of rejecting the mismatch. The same missing boundary lets reordered same-typed Struct children be mapped by ordinal. Please validate recursive child types and normalized field identity against the cached target before mutating the block, and add ListView/LargeListView schema-drift tests.
| if (type.id() == arrow::Type::LIST_VIEW) { | ||
| auto converted = canonicalize_list_view<int32_t, arrow::ListViewArray, | ||
| arrow::ListBuilder, arrow::ListArray>( | ||
| static_cast<const arrow::ListViewArray&>(*current), pool); |
There was a problem hiding this comment.
[P1] Normalize encoding wrappers below the rebuilt list
For list_view<dictionary<int8,int8>>, MakeBuilder creates a dictionary child builder. Its AppendArraySlice inserts decoded values, but Finish still emits a DictionaryArray backed by indices. The next normalizer pass sees only the outer LIST as acceptable and returns it unchanged; ADBC maps the dictionary's logical value type to TINYINT, so Array/Nullable/Number SerDes copy the one-byte index buffer. Logical values [42,43] are therefore returned as [0,1], and default Arrow validation cannot distinguish the equally wide buffers. Before this change the outer ListView was rejected. Please recursively normalize or reject encoding-only descendants before accepting a complex outer type, with ListView/LargeListView dictionary-child tests.
TPC-H: Total hot run time: 16869 ms |
|
run buildall |
|
/review |
TPC-H: Total hot run time: 17245 ms |
TPC-DS: Total hot run time: 81213 ms |
|
Codex automated review failed and did not complete. Error: This content was flagged for possible cybersecurity risk. If this seems wrong, try rephrasing your request. To get authorized for security work, join the Trusted Access for Cyber program: https://chatgpt.com/cyber Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Aug 20th, 2026 3:35 AM. The selected account is excluded until 2026-08-20T03:35:00Z. Please trigger /review again; another configured account may be available. |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
2 participants
What problem does this PR solve?
ADBC maps Arrow ListView and LargeListView fields to Doris arrays, but these encoding variants must be converted to canonical lists before materialization. Incremental child-builder growth can also cause avoidable allocation spikes, and nested views can overflow before a parent builder reports capacity. A source schema can also become nullable while a cached Doris target remains non-nullable.
What is changed and how does it work?
Check List