Repository navigation
[feature](storage) support change variant_doc_materialization_min_rows and vertical_compaction_num_columns_per_group - #61100
Merged
Conversation
csun5285
requested review from
CalvinKirs,
dataroaring,
gavinchou,
morningman,
w41ter and
yiguolei
as code owners
March 6, 2026 05:03
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 27863 ms |
TPC-DS: Total hot run time: 153442 ms |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
csun5285
force-pushed
the
feature/new-feature
branch
from
March 9, 2026 04:32
7b3cdb1 to
45467b6
Compare
Contributor
Author
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 27841 ms |
TPC-DS: Total hot run time: 153645 ms |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
eldenmoon
changed the title
variant_doc_materialization_min_rows and vertical_compaction_num_columns_per_group
Member
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR adds support for dynamically changing variant_doc_materialization_min_rows (via ALTER TABLE MODIFY COLUMN) and vertical_compaction_num_columns_per_group (via ALTER TABLE SET(...)) as per-table properties. The implementation is thorough and covers all necessary code paths across FE, BE, Cloud, protobuf, and thrift layers.
Critical Checkpoint Conclusions
1. Goal and Test Coverage: The stated goal is achieved. Both features have regression tests. The variant_doc_materialization_min_rows test verifies the end-to-end flow including compaction behavior. The vertical_compaction_num_columns_per_group test uses debug points to verify the value reaches BE compaction logic. Test coverage is good overall.
2. Modification scope: The change is focused and touches only the necessary files. All CreateReplicaTask call sites (6 in non-cloud, 4 in cloud mode) and all vertical_split_columns call sites (2) have been updated. The protobuf/thrift/cloud.proto definitions are correctly added.
3. Concurrency: The new _vertical_compaction_num_columns_per_group field in TabletMeta follows the same unlocked getter/setter pattern as existing compaction config fields (_time_series_compaction_*). This is consistent with the class design where concurrency control is handled at the Tablet level. No new concurrency concerns.
4. Lifecycle management: No special lifecycle concerns. The field is a simple scalar.
5. Configuration items: The property is dynamically changeable via ALTER TABLE SET(...) and correctly propagated to BE via UpdateTabletMetaInfoTask. Cloud mode sync in CloudTablet::sync_meta() is also handled.
6. Incompatible changes / Upgrade compatibility: The protobuf fields have [default = 5] which matches the existing global config default. Thrift fields also have default = 5. During rolling upgrades, old BEs that don't understand the new thrift field will use the protobuf default of 5, which matches the previous config::vertical_compaction_num_columns_per_group behavior. No incompatibility.
7. Parallel code paths: Both cloud and non-cloud paths are updated. The config::vertical_compaction_num_columns_per_group global config is effectively superseded by the per-tablet property (config declaration retained but no longer referenced in code paths).
8. Observability: No new metrics or logging needed for this configuration change.
9. EditLog / Persistence: The property is persisted through TableProperty (Gson serialized), rebuilt during gsonPostProcess(), and included in modifyTableProperties chain. Correct.
10. Data writes / Transactionality: Not applicable - this is a configuration change, not a data write path modification.
11. FE-BE variable passing: All paths verified: CreateReplicaTask.toThrift(), UpdateTabletMetaInfoTask.toThrift(), cloud TabletMetaInfoPB, and TCreateTabletReq all correctly pass the new field.
12. Performance: No performance concerns. The per-tablet property is read once into a local variable at compaction time.
Issues Found
See inline comments for details. Main findings:
- Minor: Stale/misleading comments in variant test file (references "min_rows=5" when it should say "min_rows=10")
- Minor: Indentation inconsistency in
CloudSchemaChangeJobV2.javaandCloudInternalCatalog.java - Minor: Variant test drops table at end (violates regression test standards)
- Nitpick: Both test files use
def tableNamepattern
| ); | ||
| """ | ||
|
|
||
| // Verify SHOW CREATE TABLE shows min_rows=5 |
Contributor
There was a problem hiding this comment.
Stale comments: Lines 37, 43, and 56 reference min_rows=5 but the table is created with min_rows=10. These are copy-paste remnants. The actual code assertions are correct (checking for 10), but the comments are misleading.
Suggested fixes:
- Line 37:
// Verify SHOW CREATE TABLE shows min_rows=10 - Line 43:
// Verify tablet meta shows min_rows=10 - Line 56:
// Expect: only k and v columns, NO sub-columns extracted (3 rows < min_rows=10)
|
|
||
| // Cleanup | ||
| sql """ DROP TABLE IF EXISTS ${tableName} """ | ||
| } |
Contributor
There was a problem hiding this comment.
Per regression test standards: tables should not be dropped at the end of tests (to preserve the environment for debugging). The DROP TABLE IF EXISTS at the beginning of the test (line 21) is correct, but this cleanup drop at the end should be removed.
| columnSeqMapping); | ||
| columnSeqMapping, | ||
| tbl.getVerticalCompactionNumColumnsPerGroup()); | ||
| requestBuilder.addTabletMetas(builder); |
Contributor
There was a problem hiding this comment.
Nit: Indentation inconsistency. This line has extra leading spaces compared to the columnSeqMapping, argument above it. Should align with the other arguments at the same indentation level.
…up table property
csun5285
force-pushed
the
feature/new-feature
branch
from
March 12, 2026 06:13
45467b6 to
9365844
Compare
Contributor
Author
|
run buildall |
github-actions
Bot
removed
the
approved
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 28070 ms |
TPC-DS: Total hot run time: 153978 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
csun5285
added a commit
to csun5285/doris
that referenced
this pull request
Mar 17, 2026
…ws` and `vertical_compaction_num_columns_per_group` (apache#61100) cherry-pick from upstream master 4eba7f9
csun5285
added a commit
to csun5285/doris
that referenced
this pull request
Mar 17, 2026
…ws` and `vertical_compaction_num_columns_per_group` (apache#61100)
csun5285
added a commit
to csun5285/doris
that referenced
this pull request
Mar 17, 2026
…ws` and `vertical_compaction_num_columns_per_group` (apache#61100)
csun5285
added a commit
to csun5285/doris
that referenced
this pull request
Nov 9, 2026
… let MODIFY COLUMN change the row threshold ### What problem does this PR solve? Issue Number: None Related PR: apache#69812, apache#61100 Problem Summary: Two choices of the earlier commits went further than renaming the properties: - `variant_sparse_shard_count` was written into both stored shard fields, `variantSparseHashShardCount` and `variantDocShardCount`, and MODIFY COLUMN compared the field of the column's mode instead of both fields. The property only has a new name: with doc mode the value belongs to the doc shard field and without it to the sparse shard field, as each mode had its own property before. The parser now fills that field only and leaves the other one 0, which is what every released version stores, so `Column.checkSchemaChangeAllowed` compares the two fields again as master does. The BE and SHOW CREATE TABLE already read the field of the mode. - MODIFY COLUMN refused any change of `variant_subcolumn_extraction_min_rows`, while apache#61100 lets it change `variant_doc_materialization_min_rows`. It can change again, with and without doc mode, except on a column without doc mode created before this change (a V1 column): its BE ignores the threshold, because its count-0 compaction branch cannot extract paths that only the sparse column holds, so a value stored there would do nothing. test_variant_modify_doc_materialization_min_rows has its steps from master again (an ALTER lowers the threshold and the next compaction extracts the old paths), and test_variant_tuning_properties raises the threshold with an ALTER again. The BE comment and the UT description of a compaction below the threshold name the ALTER again; the BE code does not change. ### Release note MODIFY COLUMN can change `variant_subcolumn_extraction_min_rows`, but not on a column without doc mode created before the upgrade. ### Check List (For Author) - Test: Unit Test, Regression test - FE UT: the 11 classes that the VARIANT properties touch (PropertyAnalyzerTest, VariantSerializeTypeDdlTest, TypeTest, ColumnTest, SchemaChangeHandlerTest, DataTypeTest, NereidsParserTest, VariantLoadParseInjectionTest, CreateTableCommandTest, ShowCreateTableCommandTest, SessionVariablesTest), 286 tests, 0 failures. - Regression: test_variant_tuning_properties and test_variant_modify_doc_materialization_min_rows, regenerated by running them (the latter extracts the same subcolumns as on master); every enabled suite of variant_p0 (187), where only outfile_csv_variant_doc_value fails because it needs S3 credentials; six suites outside variant_p0 that create VARIANT tables; check_before_quit, which recreates every table from its SHOW CREATE TABLE and compares. - Behavior changed: Yes. MODIFY COLUMN can change the row threshold again. - Does this need documentation: Yes. Co-Authored-By: Claude Opus 5.5 <[email protected]>
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.
1.
variant_doc_materialization_min_rowsALTER TABLE MODIFY COLUMN.2.
vertical_compaction_num_columns_per_groupALTER TABLE SET(...). This controls how many columns are merged in each group during vertical compaction.memory usage during compaction.
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)