Uh oh!
There was an error while loading. Please reload this page.
[feat](Variant) add variant_enable_nested_group property and fix variant nullable issue - #61047
Conversation
Thearas
commented
Mar 4, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
eldenmoon
commented
Mar 4, 2026
run buildall |
eldenmoon
commented
Mar 4, 2026
/review |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new VARIANT column property, variant_enable_nested_group, and wires it through FE/BE metadata serialization (Thrift/Proto) and BE write paths to enable/disable nested-group handling.
Changes:
- Add
variant_enable_nested_groupto Thrift (TColumn) and Proto (ColumnPB) descriptors. - Propagate the flag through FE type/model layers (session variable, property parsing, catalog/nereids VariantType conversion, Column serialization).
- Gate BE nested-group routing/prepare behavior in VARIANT column writers based on the tablet schema flag.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| gensrc/thrift/Descriptors.thrift | Adds variant_enable_nested_group to TColumn for Thrift serialization. |
| gensrc/proto/olap_file.proto | Adds variant_enable_nested_group to ColumnPB for on-disk schema serialization. |
| fe/fe-core/src/main/java/org/apache/doris/tablefunction/ExternalFileTableValuedFunction.java | Updates VARIANT type construction to pass the new nested-group flag. |
| fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java | Adds a session variable to control the default nested-group setting. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/types/VariantType.java | Extends Nereids VariantType to carry the nested-group flag. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/types/DataType.java | Includes nested-group flag when converting from catalog VariantType to Nereids VariantType. |
| fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java | Parses the new property and passes it into Nereids VariantType. |
| fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java | Adds property key + parsing + validation rules for variant_enable_nested_group. |
| fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java | Serializes/deserializes nested-group flag to Thrift/Proto and blocks schema-change toggling. |
| fe/fe-common/src/main/java/org/apache/doris/catalog/VariantType.java | Adds nested-group field to catalog VariantType metadata and equality logic. |
| fe/fe-common/src/main/java/org/apache/doris/catalog/ScalarType.java | Adds getVariantEnableNestedGroup() for backward-compatible access via ScalarType. |
| be/src/vec/functions/array/function_array_utils.cpp | Ensures VARIANT root uses a nullable nested type when wrapping array element columns. |
| be/src/olap/tablet_schema.h | Adds nested-group flag to tablet schema VARIANT metadata. |
| be/src/olap/tablet_schema.cpp | Reads/writes the nested-group flag from/to ColumnPB. |
| be/src/olap/tablet_meta.cpp | Transfers nested-group flag from Thrift TColumn into tablet metadata. |
| be/src/olap/rowset/segment_v2/variant/variant_column_writer_impl.cpp | Conditionally builds routing plan / invokes nested-group provider based on the flag. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Code Review Summary: feat add variant_enable_nested_group property
This PR adds a variant_enable_nested_group property plumbed through the entire FE->BE stack (session variable, property analysis, Nereids types, catalog types, Thrift/Protobuf serialization, BE tablet schema, and writer gating). The feature is explicitly gated with a "not supported now" exception. It also includes a correctness fix in function_array_utils.cpp.
Critical Checkpoint Conclusions
Goal / Does the code accomplish it? The goal is to add the plumbing for a
variant_enable_nested_groupproperty. The plumbing is complete across all layers. However, there is a critical usability bug: the session variable defaults totruewhile the feature throws "not supported now" when enabled, breaking allvariant<...>column definitions by default.Is this modification minimal, clear, and focused? Mostly yes. The
function_array_utils.cppfix is unrelated to the property addition and should ideally be a separate commit/PR for clarity, but is a legitimate bug fix.Concurrency? No new concurrency concerns. The new field is a simple boolean in immutable-after-construction structures.
Lifecycle management? No special lifecycle concerns.
Configuration items added? Yes — session variable
default_variant_enable_nested_group(defaulttrue). The default is problematic (see issue #1 below).Incompatible changes / rolling upgrade? New optional fields in Protobuf and Thrift with
falsedefaults — backward compatible. Older BEs will simply ignore the new field.Parallel code paths? The property is correctly gated in both
VariantColumnWriterImpl::finalize()andVariantSubcolumnWriter::finalize().Test coverage? No tests are added. At minimum, a regression test verifying the property can be set to
falseand a negative test for the "not supported now" error should be present.Observability? No observability concerns for a boolean property.
Transaction/persistence? The property is correctly persisted via Protobuf (ColumnPB) and serialized via Thrift (TColumn).
FE-BE variable passing? Correctly set in
Column.toThrift()andColumn.toPb(), and read inTabletMeta.init_column_from_tcolumn()andTabletColumn.init_from_pb().Performance? No performance concerns. The gating avoids calling nested group provider methods when disabled.
Issues Found
See inline comments for details.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
eldenmoon
commented
Mar 4, 2026
run buildall |
Uh oh!
There was an error while loading. Please reload this page.
b951af9 to
2e1e4aeCompareeldenmoon
commented
Mar 4, 2026
run buildall |
doris-robot
commented
Mar 4, 2026
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Uh oh!
There was an error while loading. Please reload this page.
2e1e4ae to
06a53c9Compareeldenmoon
commented
Mar 4, 2026
run buildall |
06a53c9 to
516cf7fCompareeldenmoon
commented
Mar 4, 2026
run buildall |
hello-stephen
commented
Mar 4, 2026
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Mar 4, 2026
FE UT Coverage ReportIncrement line coverage |
doris-robot
commented
Mar 4, 2026
TPC-H: Total hot run time: 29184 ms |
doris-robot
commented
Mar 4, 2026
TPC-DS: Total hot run time: 183747 ms |
doris-robot
commented
Mar 4, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Mar 4, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Mar 4, 2026
FE Regression Coverage ReportIncrement line coverage |
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
eldenmoon
commented
Mar 5, 2026
skip check_coverage |
hello-stephen
commented
Mar 5, 2026
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Mar 5, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
eldenmoon
commented
Mar 5, 2026
skip check_coverage |
Uh oh!
There was an error while loading. Please reload this page.
hello-stephen
commented
Mar 5, 2026
FE Regression Coverage ReportIncrement line coverage |
…ant nullable issue (apache#61047) (cherry picked from commit 405d8df)
No description provided.