Repository navigation
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69995 Problem Summary: Compaction promotes each physical Variant path independently, but BinaryColumnExtractIterator assembles query subtrees. A sparse row containing only a.b then becomes a non-null a as well. With three rowsets, promotion raises the physical a count from 6 to 9 and fails the path-statistics check before replacing input rowsets. Reuse the existing exact sparse-path binary search and V1 storage-cell decoder so only a persisted value at the requested path is promoted. Preserve missing, Variant-null and exact object payload semantics and reject invalid offsets before accessing cells. ### Release note Fix compaction failures when a Variant ancestor is promoted from sparse storage alongside its descendants. ### Check List (For Author) - Test: ASAN BE build; focused BE UT (169 passed, 2 skipped); three targeted regression suites passed; original SQL reproduces 6-to-9 statistics failure and the fixed retry merges four rowsets into one with identical row values, including after an FE/BE restart; clang-format 16, build hygiene and clang-tidy passed. - Behavior changed: Yes. Physical sparse extraction returns only exact path values. - Does this need documentation: No.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69995 Problem Summary: Add a discriminating sparse-ancestor promotion test. Three rowsets select different hot leaves at load and promote a common sparse ancestor during compaction. Check final compaction status as well as whole Variant and subpath results, because failed compactions retain readable original rowsets. Cover DUP/MOW tables, V2/V3 segment formats, one/three sparse shards and a second compaction with another sparse source. Add unit coverage for exact cells, descendant-only rows, neighboring keys, exact object payloads, missing and Variant-null values, nested paths, sequential reads and reordered rowid reads. Regression expectations were generated by the repository runner and independently checked row by row. ### Release note None ### Check List (For Author) - Test: New ASAN unit test fails on the original code in all four combinations; final focused BE UT has 169 passed and 2 skipped. New regression output generation and comparison passed, including 16 successful FULL compactions; sparse_key_byte_order and regression_test_query_subcolumns also passed. clang-format 16, build hygiene and clang-tidy passed. - Behavior changed: No. - Does this need documentation: No.
eldenmoon
requested review from
airborne12,
csun5285,
gavinchou,
liaoxin01 and
yiguolei
as code owners
November 4, 2026 10:35
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28729 ms |
Contributor
TPC-DS: Total hot run time: 151609 ms |
Contributor
ClickBench: Total hot run time: 24.34 s |
### What problem does this PR solve? Related PR: apache#70212 Problem Summary: Existing tests cover basic ancestor promotion, but not promotion/demotion/promotion of deep heterogeneous paths, encoded path neighbors, shared-cache read-mode transitions, or updates and deletes after promotion. Add these cases without repeating the existing format/shard/key-model matrix, and verify that compaction replaces the input rowsets. ### Release note None ### Check List (For Author) - Test: Focused ASAN BE unit tests; four related regression suites; independently checked 138 projected rows, five compactions and three layouts - Behavior changed: No - Does this need documentation: No
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28305 ms |
Contributor
TPC-DS: Total hot run time: 151679 ms |
Contributor
ClickBench: Total hot run time: 24.32 s |
Member
Author
|
/review |
Contributor
|
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. |
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
TPC-H: Total hot run time: 28722 ms |
Contributor
TPC-DS: Total hot run time: 151468 ms |
Contributor
ClickBench: Total hot run time: 24.37 s |
Contributor
|
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. |
Member
Author
|
/review |
3 of 7 tasks
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review of head 7de78269c53e33b103d781f955905ee9a35ce040: one new P2 test portability finding, reported inline. No production correctness regression was substantiated. The review converged after two rounds: all three reviewers returned NO_NEW_VALUABLE_FINDINGS in round 2, all six changed files were covered, and every candidate/risk has a verified disposition.
The reviewed invocation flow is:
VariantColumnReader
compaction/checksum flat leaves -> BinaryColumnExtractIterator -> exact cell -> decode_v1_storage_cells -> writer
query requiring subtree -> HierarchicalDataIterator -> VariantAssembler
Critical checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and evidence | Exact-path extraction prevents descendant-only rows from increasing a promoted ancestor's physical non-null count. Added unit/regression cases target that failure. |
| Scope, clarity and reuse | One production implementation changes; it reuses the existing byte-order lower_bound and bounded storage-cell decoder. Other changes are tests/results. |
| Concurrency and locks | No new threads, atomics, locks or lock ordering; scratch state is local and sibling cache consumption remains synchronous. |
| Lifecycle and static initialization | Borrowed cells are decoded into owning columns before a later cache reset. No new static initialization or ownership cycle. |
| Configuration | No new production setting. Temporary regression BE overrides restore saved values in finally. |
| Compatibility | No persisted cell format, metadata layout, function signature, dependency or libc API changes; existing legacy-cell decoding is reused for both segment formats. |
| Thrift/protobuf | No definition, field-id, enum or release-branch compatibility change. |
| Parallel paths | Query prefixes, document subtrees and truncated-stat cases retain hierarchical planning. Compaction/checksum use physical cells; sparse merge retains descendants; local/cloud share the extraction code. M1 concerns the new test's cloud backend selection. |
| Conditions and error handling | Exact equality excludes prefix neighbors. Bad offsets fail before search/access; decode/append statuses propagate. The padded negative offset sentinel is valid. |
| Test coverage | Inspected exact/deep/escaped paths, payload/null/missing cases, reordered/repeated rowids, shared cache transitions, corruption, format/shard/key models, promotion/demotion/repromotion and MOW updates/delete. |
| Test results | Independently reconstructed 560 base and 138 extended projected rows; checked status, row counts and layout transitions. Expectations agree for the intended local layout. M1 makes the tablet-only aggregate nondeterministic when cloud backends cache multiple copies. |
| Observability | Existing read-byte/timing counters and corruption context remain. This local decoding change needs no new background-operation metric. |
| Transactions, persistence and failover | No EditLog, transaction, visibility/version-selection or failover protocol changes. |
| Writes, atomicity and crash cleanup | Decode publishes only a successful owning result; existing compaction rowset replacement, delete bitmaps and cleanup remain downstream. |
| FE/BE variable passing | No new transmitted value or sender path. |
| Performance and memory | Per-row logarithmic lookup avoids descendant decoding/subtree assembly; O(rows) scratch vectors use Doris allocation accounting and shared batch reuse is retained. |
| Other concerns and user focus | No additional user focus was supplied. R1 (caller/path/shard semantics) and R2 (decode/null/ownership/cache semantics) were dismissed with concrete upstream/downstream evidence; no unresolved static review question remains. |
M1 is conditional on a supported multi-BE cloud invocation, such as one using enable_cloud_multi_replica; standard cloud_p0 skips nonConcurrent suites, so this is not a claim that its ordinary pipeline fails. Pin the backend across trigger/status/aggregate or explicitly restrict the physical-layout assertion to local mode.
No existing inline threads were present at the final live refresh; there are no existing P0/P1 findings to carry forward. Validation was static only: no builds, tests, formatters, clang-tidy or runtime/ELF checks were executed, and test-output generation provenance was not independently established. No repository source was modified.
Review converged after 2 round(s).
| assert after.rowsets.size() == 1 : after | ||
| assert after.rowsets.intersect(before.rowsets).isEmpty() : after | ||
| qt_output_rows """SELECT count(*), sum(NUM_SEGMENTS), sum(ROWSET_NUM_ROWS) | ||
| FROM information_schema.rowsets WHERE TABLET_ID=${tablet.TabletId} AND NUM_SEGMENTS>0""" |
Contributor
There was a problem hiding this comment.
[P2] Scope the rowset aggregate to the checked backend
In a multi-BE cloud compute group with enable_cloud_multi_replica, qt_before can cache the input rowsets on BE B while the full compaction and status checks succeed on BE A. information_schema.rowsets scans every BE's cached tablet map, so this tablet-only aggregate also counts B's stale inputs (or a duplicate refreshed output). The expected 1 / 1 / N result then fails despite successful compaction. Filter by BACKEND_ID for the verified backend and keep that backend pinned across trigger/status checks, or explicitly guard this physical-layout assertion to local mode.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This was referenced Nov 6, 2026
eldenmoon
added a commit
to eldenmoon/incubator-doris
that referenced
this pull request
Nov 6, 2026
…pe-table Resolve the sparse extract iterator conflict with apache#70212 by keeping master's exact sparse cell lookup; that path no longer assembles subtrees, so drop the iterator's path_encoding parameter. Keep only master's sparse plan sort, which orders the same encoded storage paths after statistics selection. Co-Authored-By: Claude Opus 5.5 <[email protected]>
eldenmoon
added a commit
to eldenmoon/incubator-doris
that referenced
this pull request
Nov 6, 2026
Check escaped-table schemas with one PropertyAnalyzer function (no NestedGroup; predefined templates and index field_pattern must be valid storage paths), called where the OlapTable is in hand: InternalCatalog.createOlapTable for CREATE TABLE, CTAS, async MV and LIKE, SchemaChangeHandler for ADD/MODIFY COLUMN and ADD INDEX, and MaterializedViewHandler for sync MVs. The DDL info classes are back to their master versions. ALTER and external CREATE TABLE reject the property through shared helpers. An async MV must use the same value as its OLAP base tables that have VARIANT columns: query rewrite replaces base scans with MV scans, and v['a.b'] maps to different storage paths on legacy and escaped tables. CTAS keeps inheriting from any escaped source. Port dotted_key_escape_sparse_promotion, which needed apache#70212, to the table property. Pin the doc mode and subcolumn defaults in dotted_key_escape_score: a path that is not materialized scores 0 on both encodings. Co-Authored-By: Claude Opus 5.5 <[email protected]>
eldenmoon
added a commit
to eldenmoon/incubator-doris
that referenced
this pull request
Nov 6, 2026
…tests On branch-4.x, casting Variant(max_subcolumns=0) from parse_to_variant into a column typed Variant(max_subcolumns=1) is rejected. Insert JSON strings instead so the cast uses the target column type, keeping the compaction sparse-ancestor promotion coverage from apache#70212.
eldenmoon
added a commit
to eldenmoon/incubator-doris
that referenced
this pull request
Nov 6, 2026
…tests On branch-4.x, casting Variant(max_subcolumns=0) from parse_to_variant into a column typed Variant(max_subcolumns=1) is rejected. Insert JSON strings instead so the cast uses the target column type, keeping the compaction sparse-ancestor promotion coverage from apache#70212.
eldenmoon
added a commit
to eldenmoon/incubator-doris
that referenced
this pull request
Nov 7, 2026
… inserts 1) schema_change .out: nested commit.distinct is true/false on branch-4.x (public remains 1). 2) var_index_snii: avoid parse_to_variant → typed Variant cast which branch-4.x rejects (Variant(0)→Variant(9)); insert JSON strings instead, same pattern as the apache#70212 sparse_ancestor pick fix.
xylaaaaa
pushed a commit
to xylaaaaa/doris
that referenced
this pull request
Nov 9, 2026
### What problem does this PR solve? Related PR: apache#69082 Problem Summary: Public URL generation assumed an S3 client whenever an endpoint matched the OSS private endpoint suffix. A custom Azure endpoint with that suffix would fail before signing. Rewrite the endpoint in a copy of the configured backend so its own client signs the public URL. ### Release note None ### Check List (For Author) - Test: FE and ASAN BE builds passed. The file system and regression test compiled. The macOS BE UT target stopped at the pre-existing std::min type mismatch in binary_column_extract_iterator_test.cpp:74 from apache#70212. Header hygiene, clang-format 16 and clang-tidy passed. - Behavior changed: No (restores pre-refactor public URL generation). - Does this need documentation: No
xylaaaaa
pushed a commit
to xylaaaaa/doris
that referenced
this pull request
Nov 9, 2026
### What problem does this PR solve? Related PR: apache#69082 Problem Summary: When credentials were refreshed while a writer waited for its parts, CompleteMultipartUpload used the replacement client. Remember the holder state before waiting. After all upload callbacks finish, reject an invalidated holder and keep the original client when only authentication changed. Other configuration updates and resource backend rebinds still use the current client. ### Release note None ### Check List (For Author) - Test: ASAN BE and FE builds passed. The new writer test compiled. The macOS BE UT target still stops in the upstream segment test binary_column_extract_iterator_test.cpp:74 (apache#70212), before test execution. Header hygiene, clang-format 16 and clang-tidy passed. - Behavior changed: No (restores pre-refactor credential rotation). - Does this need documentation: No
yiguolei
pushed a commit
that referenced
this pull request
Nov 10, 2026
…ion (#70355) cherry-pick #70212 Conflicts resolved: kept branch-4.1 exact-match _fill_variant_v2_path (already equivalent to the master fix; release branch still has dual V1/V2 API) and added intent comment. Adapted new BE UTs to ColumnVariant::create_binary_column_fn / StorageReadOptions.stats / use_variant_v2=true. BinaryColumnExtractIteratorV2Test: 13/13 passed locally.
yiguolei
pushed a commit
that referenced
this pull request
Nov 10, 2026
…ion (#70356) cherry-pick #70212 Conflicts resolved: kept branch-4.2 exact-match _fill_variant_v2_path (already equivalent; dual V1/V2 API) and added intent comment. Adapted new BE UTs to branch APIs (ColumnVariant::create_binary_column_fn / StorageReadOptions.stats / use_variant_v2=true). BE UT left to CI (branch-4.2 full UT build known lance_reader issue); same adaptations validated via branch-4.1 UT run.
eldenmoon
added a commit
to eldenmoon/incubator-doris
that referenced
this pull request
Nov 12, 2026
… inserts 1) schema_change .out: nested commit.distinct is true/false on branch-4.x (public remains 1). 2) var_index_snii: avoid parse_to_variant → typed Variant cast which branch-4.x rejects (Variant(0)→Variant(9)); insert JSON strings instead, same pattern as the apache#70212 sparse_ancestor pick fix.
yiguolei
pushed a commit
to eldenmoon/incubator-doris
that referenced
this pull request
Nov 13, 2026
… inserts 1) schema_change .out: nested commit.distinct is true/false on branch-4.x (public remains 1). 2) var_index_snii: avoid parse_to_variant → typed Variant cast which branch-4.x rejects (Variant(0)→Variant(9)); insert JSON strings instead, same pattern as the apache#70212 sparse_ancestor pick fix.
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
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: N/A
Related PR: #69995 (found during review; this defect also exists on master), #70060 (independent internal-column-name classification fix)
Problem Summary:
Compaction can promote an ordinary Variant path from sparse storage while its descendants stay sparse.
BinaryColumnExtractIteratorcurrently uses the hierarchicalVariantAssemblerfor this physical extraction. A row containing onlya.bthen becomes a non-null value in the promotedacolumn as well.With
variant_max_subcolumns_count=1, three input rowsets each materialize a different hot key and leave two scalaravalues plus onea.bvalue sparse. Across the rowsets,ais selected for promotion. Its physical non-null count grows from 6 to 9 and compaction fails withPath stats not match. The failure happens before replacing the input rowsets, so the old data remains readable, but retries with the same selection fail again. This occurs with ordinary keys and does not require dotted-key escaping.Extract only the exact persisted path using the existing sparse-path binary search and
decode_v1_storage_cells. Keep the existing missing/SQL-NULL and present Variant-null semantics, validate offsets before accessing cells, and retain full object payloads stored at the exact path. Hierarchical query assembly continues to useVariantAssembler.Self-review: the change stays within physical sparse extraction, shared by compaction/checksum readers. Query subtree assembly retains its own assembler. Scratch vectors use Doris memory accounting; borrowed cells are decoded before appending the owning result, and invalid offsets return Corruption before access. No stored-format, protocol, schema, lock, publication or delete-bitmap changes are required. Runtime validation uses a local shared-nothing cluster; cloud callers were checked statically.
Release note
Fix compaction failures when a Variant ancestor path is promoted from sparse storage alongside its descendants.
Check List (For Author)
6 -> 9count mismatch and confirmed that failed compaction leaves the input rowsets readable.clang-format16, build hygiene and changed-line clang-tidy passed.sparse_ancestor_promotion,sparse_ancestor_promotion_extended,sparse_key_byte_orderandregression_test_query_subcolumnspassed in comparison mode. Expectations were generated by the repository runner and independently checked.