[fix](be) Export byte-backed aggregate states as Parquet binary - #66305
Draft
zclllyybb wants to merge 1 commit into
Draft
[fix](be) Export byte-backed aggregate states as Parquet binary#66305zclllyybb wants to merge 1 commit into
zclllyybb wants to merge 1 commit 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.
Review completed for head 284c90e. Requesting changes for one performance regression.
Checkpoint conclusions:
-
Goal and correctness proof: Aggregate states are emitted as opaque Parquet binary, valid fixed and variable state bytes round-trip, and malformed fixed widths fail before unsafe insertion or partial output.
-
Scope, focus, and performance: The change is focused, but the early CAST dispatch bypasses the existing identity fast path and copies all String-backed variable states; see the inline finding.
-
Nullable, constant, and error handling: Standard wrappers preserve source null maps and materialize constants; fixed-width errors propagate explicitly without publishing partial output.
-
Concurrency, lifecycle, configuration, persistence, FE/BE propagation, transactions, and observability: No new shared state, lifecycle, configuration, durable Doris format, distributed variable, transaction behavior, or separate observability requirement is introduced.
-
Compatibility and parallel paths: The external Parquet representation intentionally changes to opaque binary as documented; both Parquet reader mappings reach the checked CAST, and FileScannerV2 cannot bypass load conversion. The direct FORMAT_ARROW validation weakness is pre-existing and not changed by this PR.
-
Tests and results: Unit coverage checks Arrow type selection, exact fixed-state bytes, and malformed width; the ordered regression checks fixed, variable, nullable-declared, and non-nullable-declared semantic round trips. Per the review bundle, no local build or test was run; existing style and macOS BE-UT checks are green.
No additional review focus was supplied, so the full PR was reviewed.
| // NOLINTNEXTLINE(readability-function-size) | ||
| WrapperType prepare_impl(FunctionContext* context, const DataTypePtr& origin_from_type, | ||
| const DataTypePtr& origin_to_type) { | ||
| if (check_and_get_data_type<DataTypeAggState>(origin_to_type.get()) != nullptr && |
Contributor
There was a problem hiding this comment.
[P2] Preserve the identity fast path for String-backed states
This branch runs before the serialized-type equality check below, so a String source cast to a variable AggState whose serialized column is already ColumnString (for example HLL or group_concat) now allocates a new column and copies every state payload row-by-row. Raw Parquet BYTE_ARRAY maps to DataTypeString by default, so the normal OUTFILE-to-stream-load path regresses from O(1) column reuse to O(total state bytes). Let the existing serialized-type equality/identity check run before this specialization; fixed-state and VARBINARY sources would still take the checked byte-copy path.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 30159 ms |
Contributor
TPC-DS: Total hot run time: 177703 ms |
Contributor
ClickBench: Total hot run time: 24.8 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Parquet OUTFILE previously derived Arrow schemas from an aggregate state serialized type. String-backed states were labeled as UTF-8, while fixed-length states could not be mapped to Arrow. The first fix overcorrected this by mapping every AggState to Binary. DataTypeAggState delegates its column and SerDe to the physical serialized type, so complex states such as array_agg still use ArraySerDe. Pairing that SerDe with BinaryBuilder caused the P0/Cloud P0 SIGABRT. Classify an AggState as raw bytes only when its physical serialized type is String or FixedLengthObject. Export those states as Arrow Binary and keep Array, Map, Bitmap, and other complex states on their native Arrow/SerDe path. On import, preserve serialized-type identity before the raw-byte cast, limit that cast to byte-backed states, and validate fixed-width payloads through nullable wrappers. Add unit coverage for raw-byte schema mapping, real Array AggState record-batch conversion, cast identity, valid fixed-size payloads, and malformed widths. Keep regression coverage for string/fixed states and validate the existing array and bitmap sibling suites on an ASAN local cluster. Verification: - GLIBC_COMPATIBILITY=OFF CMAKE_BUILD_DIR=/mnt/disk6/common/doris-27625-zhaochangle/ut_build_ASAN ./run-be-ut.sh -j 90 --run --filter=Params/DataTypeAggStateTest.* (30/30 passed) - GLIBC_COMPATIBILITY=OFF ./build.sh --be --fe --be-extension-ignore avro-scanner -j 90 - ./run-regression-test.sh --run -d query_p0/outfile/agg_state_array -s test_outfile_agg_state_array - ./run-regression-test.sh --run -d query_p0/outfile/agg_state -s test_outfile_agg_state - ./run-regression-test.sh --run -d query_p0/outfile/agg_state_bitmap -s test_outfile_agg_state_bitmap - build-support/check-format.sh with clang-format 16 - git diff --check
zclllyybb
force-pushed
the
agent/fix-agg-state-parquet-binary
branch
from
July 31, 2026 07:32
284c90e to
e3fcf58
Compare
zclllyybb
changed the title
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29105 ms |
Contributor
TPC-DS: Total hot run time: 168859 ms |
Contributor
ClickBench: Total hot run time: 23.88 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review summary
This change correctly maps byte-backed AggState payloads to Arrow Binary and keeps complex states on their native builders, but internally nullable fixed-state round trips still fail in both directions. Requesting changes for the two data-correctness defects below; the existing String identity-copy thread is resolved at the current head.
Checkpoint conclusions:
- Goal and tests: The non-null fixed and representative complex paths are covered, but
max_state(NULL)is omitted and fails both export and load as described inline. - Scope and conditions: The implementation is focused and the raw-byte predicate is narrow; shared Flight and Python consumers were inspected and their Binary builder/SerDe types agree.
- Concurrency, lifecycle, configuration, persistence, transactions, and FE-BE variables: No new mechanism in these categories is introduced. OUTFILE and stream-load transaction boundaries are unchanged.
- Compatibility and parallel paths: Parquet export/import and the shared Arrow schema consumers were traced. The Utf8-to-Binary schema change correctly describes opaque bytes; no separate incompatibility was demonstrated.
- Error handling and observability: Status propagation and malformed-width context are adequate; this local conversion path does not need new metrics or logs. Null-source handling remains incorrect as described inline.
- Memory and performance: Destination ownership, reservation, COW behavior, and allocator use are sound. Serialized-type identity precedes the copy path, resolving the prior performance comment.
- Data correctness and BE nullable handling: Two P1 defects remain: serialized nullability is absent from the Arrow field and the load cast loses the nullable source map.
- Test results: The deterministic expected output is consistent with the added non-null data. No builds or tests were run in this review because the authoritative task requires static review only; current PR checks are green.
- Other applicable checkpoints: No additional valuable finding survived the full upstream/downstream review.
| Status convert_to_arrow_type(const DataTypePtr& origin_type, | ||
| std::shared_ptr<arrow::DataType>* result, | ||
| const std::string& timezone) { | ||
| if (is_raw_byte_agg_state(origin_type)) { |
Contributor
There was a problem hiding this comment.
[P1] Preserve serialized AggState nullability in the Arrow field
A legal agg_state<max(int null)> stores max_state(NULL) as an internally nullable FixedLengthObject, even though the outer AggState is deliberately non-nullable. This branch emits that state through a Binary builder, but the Parquet schema still takes field nullability from root()->is_nullable() (false), making the byte column required and leaving no definition level for the internal null. Please derive Arrow field nullability from the AggState's serialized type and cover a max_state(NULL) Parquet round trip.
| } | ||
|
|
||
| const auto value = source.get_data_at(row); | ||
| if (fixed_result != nullptr && value.size != fixed_result->item_size()) { |
Contributor
There was a problem hiding this comment.
[P1] Propagate nullable binary sources before checking state width
FileScanner makes the Parquet byte column Nullable, but the target AggState's outer type is non-nullable, so prepare_remove_nullable() does not pass this source null map here. For a null row, ColumnNullable::get_data_at() therefore returns a zero-byte value and this check rejects it as malformed before the internally nullable fixed-state destination can preserve the null. Please unwrap/propagate source nulls for byte-to-AggState casts (while still rejecting nulls for a non-nullable serialized target) and add a nullable fixed-state load case.
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: DORIS-27625
Related PR: #50769
Problem Summary:
Parquet OUTFILE derived Arrow schemas from the physical serialized type of aggregate-state columns. String-backed states were labeled as UTF-8, while fixed-length states could not be mapped to Arrow. The import path also interpreted exported byte-backed state payloads as text and did not validate fixed-length payload widths.
Aggregate states do not all have the same physical representation. String and FixedLengthObject states are raw bytes, while Array, Map, Bitmap, and other complex states use their native columns and SerDes. Mapping every AggState to Arrow Binary mismatched complex SerDes with BinaryBuilder and caused a BE SIGABRT in the existing array_agg OUTFILE regression.
This PR exports only byte-backed aggregate states as Arrow Binary, preserves native Arrow types for complex states, restores raw payloads into byte-backed state columns, and rejects malformed fixed-size payloads.
Release note
Parquet OUTFILE now exports byte-backed aggregate-state columns as binary and supports round-trip import of fixed-length states. Complex aggregate states retain their native Parquet representation.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)