[refine](function) Unify nullable array access with ColumnArrayView - #66234
Open
Mryange wants to merge 3 commits into
Open
[refine](function) Unify nullable array access with ColumnArrayView#66234Mryange wants to merge 3 commits into
Mryange wants to merge 3 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes: two blocking regressions remain.
array_applyno longer compiles for decimal scalar dispatch because this translation unit lacks the completeColumnDecimaldefinition; the current macOS BE UT fails on the reviewed head.element_atnow unconditionally treats the shared array/map nested column asColumnNullable, but synthetic arrays over non-nullableColumnMapvalues are valid and now fail a bad cast.
Critical checkpoint conclusions:
- Goal and functional proof: using
ColumnArrayViewis consistent with the enforcedDataTypeArraynested-nullable layout for true arrays, but the PR is not functionally complete until the compile failure and synthetic map path are fixed. - Scope and parallel paths: the refactor is otherwise focused. The main array, map, direct-function, lambda-function, const, outer-null, inner-null, empty-array, and multi-argument hash paths were traced; no additional correctness issue was substantiated.
- Concurrency and lifecycle: these scalar read/transform paths introduce no new threads, shared mutable state, locks, static initialization, or special lifetime ownership.
- Compatibility and persistence: no storage-format, FE-BE protocol, configuration, transaction, or persistence change is introduced. The map-value failure is an in-process physical-column compatibility regression and is called out inline.
- Error handling, memory, performance, and observability: no ignored
Status, new untracked significant allocation, redundant asymptotic work, or new observability requirement was found. The changed loops remain linear in flattened element count. - Tests: existing array regression suites cover the modified functions and many null/empty/decimal cases, and the element-at fixtures were updated to the valid array layout. However, the live macOS BE UT currently fails during compilation, and the non-nullable map-value
element_atcase needs coverage. No local build or tests were run because this is a review-only environment. - User focus: no additional focus was provided; the entire PR was reviewed.
| constexpr PrimitiveType PType = DispatchType::PType; | ||
| *dst = _apply_internal<typename PrimitiveTypeTraits<PType>::CppType, OP>( | ||
| src_column, src_offsets, cmp); | ||
| auto array_view = ColumnArrayView<PType>::create(array_column); |
Contributor
There was a problem hiding this comment.
This new view instantiation currently breaks the BE build for decimal arrays. dispatch_switch_scalar instantiates ColumnArrayView<TYPE_DECIMAL32/64/V2> here, but this translation unit does not include the complete ColumnDecimal definition, so the live macOS BE UT fails in column_execute_util.h with implicit instantiation of undefined template 'doris::ColumnDecimal<...>'. Please include core/column/column_decimal.h here (or make the view header provide the concrete type it requires) so every supported scalar instantiation compiles.
| } else { | ||
| nested_column = array_column.get_data_ptr(); | ||
| } | ||
| const auto& nested_null_column = |
Contributor
There was a problem hiding this comment.
This helper is also used by _execute_map, which wraps ColumnMap::get_values_ptr() in a synthetic ColumnArray. DataTypeMap preserves each declared child type, so a valid map with a non-nullable value type has a plain ColumnInt32/ColumnString here rather than ColumnNullable; this unconditional cast then fails instead of returning the mapped value. The Array storage invariant does not cover that synthetic map-value array. Please retain conditional unwrapping for this shared path (or split the map and true-array paths), and add a non-nullable-map-value element_at test.
### What problem does this PR solve? Issue Number: None Related PR: apache#66234 Problem Summary: ColumnArrayView scalar dispatch instantiates decimal column types in array_apply, but the translation unit did not include the complete ColumnDecimal definition. Include the decimal column header so decimal array dispatch can compile. ### Release note None ### Check List (For Author) - Test: No need to test (header dependency fix; build not run yet) - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29497 ms |
Contributor
TPC-DS: Total hot run time: 177371 ms |
Contributor
ClickBench: Total hot run time: 24.97 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#66234 Problem Summary: array_enumerate_uniq created a second mutable pointer to its result nested column before constructing ColumnNullable. The mutable constructor then observed a shared subcolumn and failed with COW::assert_mutable. Move the uniquely owned result column and null map into ColumnNullable, and remove the obsolete input array pointer. ### Release note None ### Check List (For Author) - Test: No need to test yet (clang-format and diff checks only; regression test not run) - Behavior changed: No, restores expected execution - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29522 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29812 ms |
Contributor
TPC-DS: Total hot run time: 178275 ms |
Contributor
ClickBench: Total hot run time: 24.83 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
2 participants
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.
What problem does this PR solve?
Problem Summary: Doris Array columns always store nested elements in
ColumnNullable, but several Array functions still manually handled an unreachable non-nullable nested layout and duplicated const/outer-null unwrapping. This change usesColumnArrayViewfor scalar Array read paths, preserves concrete columns where generic output or hashing is required, and updates the affectedelement_attest fixtures to the valid physical layout.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)