[refactor](storage) Drop PredicateColumnType #64128
Open
csun5285 wants to merge 1 commit into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
csun5285
force-pushed
the
refactor/drop-predicate-column-type
branch
from
9f59008 to
aa7c0ff
Compare
Contributor
Author
|
run buildall |
csun5285
force-pushed
the
refactor/drop-predicate-column-type
branch
from
aa7c0ff to
671bc54
Compare
Contributor
Author
|
run buildall |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
PredicateColumnType<T> was a storage-layer wrapper that flattened every
predicate column into PaddedPODArray<value_type> so the SIMD-friendly
predicate-eval loops could use a uniform `data_array[i]` access regardless
of the underlying type. The cost was: a parallel column hierarchy with
restricted API, an extra 16 bytes/row (StringRef header) for strings on
top of the actual chars, and the need for callers to thread the column's
"role" (predicate vs output) down through the read path.
This change retires PredicateColumnType entirely:
- Schema::get_predicate_column_ptr now allocates the canonical
PrimitiveTypeTraits<T>::ColumnType (ColumnVector / ColumnDecimal /
ColumnString / ColumnIPv4 / ...). ColumnDictI32 is unchanged: it
still serves the low-cardinality string fast path for predicate eval.
- filter_by_selector is now implemented on ColumnVector<T> and
ColumnDecimal<T> with the same selector-gather semantics
PredicateColumnType used.
- ColumnString does not expose a contiguous StringRef[] array, so per-row
access in predicate evaluators flows through ColumnElementView<Type>
(already in core/column/column_execute_util.h for the compute layer)
rather than a pointer subscript:
ColumnElementView<Type> view {column};
_base_loop_vec<...>(size, flags, null_map, view, _value);
ColumnElementView numeric specialization yields T via pointer
arithmetic; the string specialization yields StringRef via
get_data_at — same `view[i]` call shape, no if-constexpr at the
call site. The compute-layer ColumnElementView gains size() /
operator[] aliases and its TYPE_STRING specialization is generalized
to all is_string_type(PType) via a defaulted bool template param so
TYPE_CHAR / TYPE_VARCHAR / TYPE_JSONB all resolve correctly.
- For HybridSet::find paths that need const T* per row, a small
ColumnPointerCursor<Type> lives next to ColumnElementView. Numeric
specialization holds a const T* (zero copy); string specialization
stages each row into a member StringRef and returns its address
(HybridSet::find consumes synchronously, so the staged-cell reuse
is safe).
- BloomFilter / BitmapFilter find_fixed_len_olap_engine API is
redesigned to take `const IColumn&` instead of a `const char*` that
was reinterpreted as `const T[]`. CommonFindOp / StringFindOp each
specialize per-row access:
CommonFindOp: reads column.get_raw_data() as `const T*`, passes
`[data](int i){return data[i];}` as the accessor.
StringFindOp: `assert_cast<const ColumnString&>(column)`, passes
`[&col](int i){return col.get_data_at(i);}`.
Storage-side BF/Bitmap callers collapse to one line and the previous
workaround (materializing a temporary `vector<StringRef>` to satisfy
the legacy char* API) goes away — saves one jemalloc + N×16-byte
store/load per evaluate on string columns.
- Schema-template CHAR predicate columns previously got trailing-zero
padding stripped on every PredicateColumnType<TYPE_CHAR>::get_data_at
call. ColumnString no longer does that; the strip is handled by
Block::shrink_char_type_column_suffix_zero on the output side (and
page-decoder-level for the read path per upstream PR apache#63291), so no
extra pass is needed here.
- ColumnDictI32::convert_to_predicate_column_if_dictionary now produces
ColumnString for mid-batch dict->plain fallback.
- predicate_column.h / predicate_column_test.cpp deleted; the
PredicateColumnHolderType<T> transition alias and all
core/column/predicate_column.h includes are removed.
On the _base_loop_vec signature: upstream used
`const TArray* __restrict data_array` which worked because
PredicateColumnType<TYPE_STRING> physically held a contiguous StringRef[].
After this change the string side is ColumnElementView<TYPE_STRING> (a
struct value), and `__restrict` is a pointer-only qualifier — so we pass
TArray by value without it. Verified on a Release build (objdump) that
this does NOT regress SIMD: vectorizable types (INT / BIGINT /
dict-encoded string) still emit fully-vectorized loops (vpcmpeqd /
vpcmpeqq, 4× unrolled, 16 elements/iter). The compiler's loop versioning
emits one runtime alias check at function entry (~5 cycles, <0.5% of a
1024-row batch's total cost); the main loop body is identical to the
__restrict version. Non-vectorizable types (LARGEINT / DOUBLE / DECIMAL /
ColumnString memcmp) were scalar regardless of __restrict. See the
comment above _base_loop_vec for full rationale.
Storage compiles clean in both Release and ASAN trees. Targeted UTs:
319 tests across 13 suites (Column* / Predicate* / Segment* /
BloomFilterFunc / BitmapFilterPredicate /
ColumnExecuteUtil): PASS, 0 FAIL.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
csun5285
force-pushed
the
refactor/drop-predicate-column-type
branch
from
671bc54 to
4601254
Compare
Contributor
Author
|
run buildall |
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.
Review completed for the PredicateColumnType removal refactor. I did not find a blocking correctness issue in the changed paths.
Critical checkpoint conclusions:
- Goal and tests: The PR replaces storage predicate columns with canonical ColumnVector/ColumnDecimal/ColumnString types and updates predicate/runtime-filter call sites accordingly. The changed BE unit tests cover representative predicate, bloom filter, bitmap filter, nullable, and selector-copy paths.
- Scope: The code is focused on removing PredicateColumnType and updating direct dependents; no unrelated functional expansion is apparent in the GitHub PR diff.
- Concurrency: No new shared mutable state, thread entry, or lock ordering change was introduced. Existing dictionary predicate caches remain on the same structures as before.
- Lifecycle: No new static/global initialization or ownership cycle risk was found. New per-row view helpers are stack-local and used synchronously.
- Configuration and compatibility: No config, wire protocol, storage format, or persisted metadata change is introduced.
- Parallel paths: The storage comparison, in-list, like, bloom-filter, bitmap-filter, dictionary conversion, nullable, and selector copy paths were all updated consistently for the new column representations.
- Conditional checks: New asserts/checks are consistent with the prior predicate-column invariants; I did not find a silent-continuation path added by this PR.
- Test coverage: Unit tests were updated for the new APIs. I did not run the test suite in this review runner.
- Observability: No new observability appears necessary for this internal representation refactor.
- Transactions/data visibility: The PR does not change transaction, publish, rowset visibility, or delete-bitmap logic.
- Performance: The replacement avoids the extra predicate-column representation. I checked the hot predicate loops for obvious redundant copies and did not find a blocking regression.
User focus: No additional user-provided review focus was present.
Contributor
TPC-H: Total hot run time: 29821 ms |
Contributor
TPC-DS: Total hot run time: 176302 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
ClickBench: Total hot run time: 25.24 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
1 similar comment
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?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)