[chore](variant) deprecate flatten nested property#61466
[chore](variant) deprecate flatten nested property#61466eldenmoon merged 2 commits intoapache:masterfrom
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
eldenmoon
force-pushed
the
deprecate-flatten-nested
branch
from
aa1486c to
0822ba8
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates Doris’ legacy VARIANT “flatten nested” switch to be explicitly labeled as deprecated across FE/BE, and updates regression tests/expected outputs to use the new property name.
Changes:
- Renames the table property from
variant_enable_flatten_nestedtodeprecated_variant_enable_flatten_nestedand marks related FE session/table APIs as@Deprecated. - Renames BE parse/schema plumbing fields/methods to
deprecated_*flatten_nested*while still persisting via the existing protobuf field. - Updates multiple VARIANT regression suites and
SHOW TABLE PROPERTIESgolden output to reflect the renamed property.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| regression-test/suites/variant_p1/predefine/load.groovy | Update table property key in VARIANT predefine p1 regression |
| regression-test/suites/variant_p1/doc_snapshot/load.groovy | Update table property key in doc snapshot p1 regression |
| regression-test/suites/variant_p0/test_types_in_variant.groovy | Update table property key in types regression |
| regression-test/suites/variant_p0/schema_change/test_double_write_when_schema_change.groovy | Update table property key for schema-change regression |
| regression-test/suites/variant_p0/predefine/load.groovy | Update table property key in predefine p0 regression |
| regression-test/suites/variant_p0/predefine/delete_update.groovy | Update table property key in delete/update regression |
| regression-test/suites/variant_p0/nested2.groovy | Update (commented) example property key |
| regression-test/suites/variant_p0/nested/nested_in_top_array.groovy | Update property key in nested/top-array regression |
| regression-test/suites/variant_p0/nested/load.groovy | Update (commented) property key usages |
| regression-test/suites/variant_p0/nested.groovy | Update property key in nested regression tables |
| regression-test/suites/variant_p0/doc_mode/schema_change/test_double_write_when_schema_change.groovy | Update property key in doc-mode schema-change regression |
| regression-test/suites/variant_p0/doc_mode/predefine/load.groovy | Update property key in doc-mode predefine regression |
| regression-test/suites/variant_p0/doc_mode/delete_update.groovy | Update property key in doc-mode delete/update regression |
| regression-test/suites/variant_p0/delete_update.groovy | Update property key in delete/update regression |
| regression-test/suites/variant_github_events_nonConcurrent_upgrade_p2/load.groovy | Update property key in github events upgrade regression |
| regression-test/suites/variant_github_events_nonConcurrent_p2/load.groovy | Update property key in github events regression |
| regression-test/suites/variant_doc_mode_p2/load.groovy | Update property key in doc-mode p2 regression |
| regression-test/data/query_p0/system/test_table_properties.out | Update expected SHOW TABLE PROPERTIES output key |
| fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java | Mark legacy session variable enable_variant_flatten_nested as deprecated and clarify docs |
| fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java | Adjust create-table path comments for deprecated legacy switches |
| fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java | Rename legacy table property key constant and deprecate analyzer |
| fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java | Mark legacy table property setter/getter as deprecated |
| be/src/util/json/json_parser.h | Rename parse config/context field to deprecated_enable_flatten_nested |
| be/src/util/json/json_parser.cpp | Propagate renamed parse config field in traversal logic |
| be/src/storage/tablet/tablet_schema.h | Rename schema setter/getter + backing field to deprecated_* |
| be/src/storage/tablet/tablet_schema.cpp | Map deprecated field to existing protobuf field + update comparisons |
| be/src/storage/tablet/tablet_meta.cpp | Map thrift schema field into renamed tablet schema API |
| be/src/exec/common/variant_util.cpp | Use deprecated schema flag when building parse configs / ambiguity checks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Deprecated legacy switch for flatten-nested variant behavior. | ||
| // It is distinct from variant_enable_nested_group. | ||
| @Deprecated | ||
| public static final String PROPERTIES_VARIANT_ENABLE_FLATTEN_NESTED = "deprecated_variant_enable_flatten_nested"; |
| // Deprecated legacy flatten-nested switches. | ||
| // Only if session variable: enable_variant_flatten_nested = true and | ||
| // table property: variant_enable_flatten_nested = true | ||
| // we can enable variant flatten nested otherwise throw error |
| element_ctx.is_top_array = ctx.is_top_array; | ||
| traverse(element, element_ctx); | ||
| auto& [_, paths, values, flatten_nested, __, is_top_array] = element_ctx; | ||
| auto& [_, paths, values, deprecated_flatten_nested, __, is_top_array] = element_ctx; |
| // enable variant flatten nested as session variable, default is false, | ||
| // which means do not flatten nested when create table | ||
| // Deprecated legacy switch for flatten-nested variant behavior. | ||
| // It is distinct from variant_enable_nested_group. |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary for PR #61466: chore deprecate flatten nested property
This PR renames the variant_enable_flatten_nested property and related internal identifiers to carry a deprecated_ prefix, signaling the feature is deprecated. The rename is applied to the FE table property string, BE C++ member variables and accessors, and regression test SQL. Wire-protocol field names (protobuf/thrift) are correctly left unchanged.
Critical Checkpoints
1. Goal / Completeness:
The stated goal is to deprecate the variant_enable_flatten_nested property. The code changes rename the FE property string and BE internal names, and add @Deprecated annotations. However, there are two significant omissions (see issues below).
2. Modification scope / Focused:
The changes are focused and mechanical — a rename across FE, BE, and tests. Appropriate in scope.
3. Concurrency:
Not applicable — no concurrency changes.
4. Lifecycle / Static initialization:
Not applicable.
5. Configuration changes:
Yes — the table property name visible to users changed from variant_enable_flatten_nested to deprecated_variant_enable_flatten_nested. This is an incompatible change with no backward compatibility handling (see Issue #1).
6. Incompatible changes / Rolling upgrade compatibility:
The property string stored in FE metadata is changed without migration code. Existing tables that have this property stored will silently lose the value on FE restart (see Issue #1).
7. Parallel code paths:
All FE Java code referencing this property uses the PropertyAnalyzer.PROPERTIES_VARIANT_ENABLE_FLATTEN_NESTED constant, so the rename propagates correctly to all FE paths.
8. Test coverage:
BE unit tests are broken — 29 references across 6 test files still use the old field name enable_flatten_nested which was renamed to deprecated_enable_flatten_nested in ParseConfig (see Issue #2).
9. Observability / Transactions / Data writes / FE-BE variable passing:
Not applicable.
10. Performance:
No performance concerns — this is a pure rename.
Issues Found
Issue #1 (Critical): Missing backward compatibility for persisted metadata
The FE property string constant PROPERTIES_VARIANT_ENABLE_FLATTEN_NESTED is changed from "variant_enable_flatten_nested" to "deprecated_variant_enable_flatten_nested". This string is the key used in the persisted properties map of TableProperty. Existing tables created before this change have "variant_enable_flatten_nested" -> "true" stored in their metadata (BDBJE/editlog). After this PR, buildVariantEnableFlattenNested() in TableProperty.java:329-332 will look up the new key "deprecated_variant_enable_flatten_nested", which won't exist in old metadata, causing getOrDefault to return "false". This silently resets the property for all existing tables on FE restart.
Fix needed: Add migration logic in buildVariantEnableFlattenNested() to check for the old key and migrate it to the new key, e.g.:
public TableProperty buildVariantEnableFlattenNested() {
// Backward compatibility: migrate old property key to new one
String oldKey = "variant_enable_flatten_nested";
if (properties.containsKey(oldKey)) {
String oldValue = properties.remove(oldKey);
properties.put(PropertyAnalyzer.PROPERTIES_VARIANT_ENABLE_FLATTEN_NESTED, oldValue);
}
variantEnableFlattenNested = Boolean.parseBoolean(
properties.getOrDefault(PropertyAnalyzer.PROPERTIES_VARIANT_ENABLE_FLATTEN_NESTED, "false"));
return this;
}Issue #2 (Critical): BE unit tests will not compile
The ParseConfig struct field was renamed from enable_flatten_nested to deprecated_enable_flatten_nested in be/src/util/json/json_parser.h, but 29 references across 6 BE test files still use the old field name and were not updated:
be/test/testutil/variant_util.h(3 refs)be/test/storage/segment/variant_column_writer_reader_test.cpp(7 refs)be/test/storage/segment/variant_util_test.cpp(6 refs)be/test/exec/common/schema_util_test.cpp(1 ref)be/test/exec/common/schema_util_rowset_test.cpp(1 ref)be/test/core/jsonb/json_parser_test.cpp(11 refs)
These all use config.enable_flatten_nested which should now be config.deprecated_enable_flatten_nested.
Both issues must be addressed before merging.
| // Deprecated legacy switch for flatten-nested variant behavior. | ||
| // It is distinct from variant_enable_nested_group. | ||
| @Deprecated | ||
| public static final String PROPERTIES_VARIANT_ENABLE_FLATTEN_NESTED = "deprecated_variant_enable_flatten_nested"; |
There was a problem hiding this comment.
Critical: Missing backward compatibility for persisted metadata.
Renaming this string constant from "variant_enable_flatten_nested" to "deprecated_variant_enable_flatten_nested" changes the key used to look up the property in the persisted properties map of TableProperty. Existing tables created before this change will have "variant_enable_flatten_nested" as the key in their stored metadata. After this change, buildVariantEnableFlattenNested() in TableProperty.java will fail to find the old key and silently default to false, losing the property value for all existing tables.
Fix: Add migration logic in TableProperty.buildVariantEnableFlattenNested() to check for the old key name and migrate it to the new one:
public TableProperty buildVariantEnableFlattenNested() {
String oldKey = "variant_enable_flatten_nested";
if (properties.containsKey(oldKey)) {
String oldValue = properties.remove(oldKey);
properties.put(PropertyAnalyzer.PROPERTIES_VARIANT_ENABLE_FLATTEN_NESTED, oldValue);
}
variantEnableFlattenNested = Boolean.parseBoolean(
properties.getOrDefault(PropertyAnalyzer.PROPERTIES_VARIANT_ENABLE_FLATTEN_NESTED, "false"));
return this;
}
eldenmoon
force-pushed
the
deprecate-flatten-nested
branch
from
0822ba8 to
8273cc1
Compare
|
/review |
|
run buildall |
There was a problem hiding this comment.
Code Review Summary: chore deprecate flatten nested property
This PR renames the variant_enable_flatten_nested table property to deprecated_variant_enable_flatten_nested across BE (C++), FE (Java), and regression tests, marking the feature as deprecated. The internal rename is consistent and thorough across all modules. However, there is a backward compatibility issue in the CREATE TABLE path.
Critical Checkpoint Conclusions
1. Goal / Correctness:
The goal is to deprecate the variant_enable_flatten_nested property by renaming it with a deprecated_ prefix. The BE-side rename is complete and correct. The FE-side has a deserialization migration path (migrateDeprecatedVariantEnableFlattenNestedProperty) that correctly handles already-persisted metadata. However, the CREATE TABLE path does NOT accept the old property name, which is a backward compatibility gap (see inline comment).
2. Minimal / Focused:
Yes — the change is a pure rename with deprecation annotations. No behavioral changes.
3. Concurrency:
No new concurrency concerns introduced. The rename is purely cosmetic.
4. Lifecycle / Static Init:
No lifecycle concerns. No static initialization order issues.
5. Configuration items:
The table property name visible to users changes from variant_enable_flatten_nested to deprecated_variant_enable_flatten_nested. The session variable name enable_variant_flatten_nested is unchanged. This asymmetry is acceptable since the session variable is a separate concept.
6. Incompatible changes / Rolling upgrade:
This is the key concern. The Thrift field name (variant_enable_flatten_nested in AgentService.thrift) and protobuf field name (enable_variant_flatten_nested in olap_file.proto) are unchanged — wire format compatibility is preserved. However, the user-facing SQL property name changes, which breaks existing DDL scripts that use variant_enable_flatten_nested in CREATE TABLE. The analyzeVariantFlattenNested method only looks for the new key deprecated_variant_enable_flatten_nested; the old key becomes an unrecognized property causing DdlException("Unknown properties"). See inline comment for details.
7. Parallel code paths:
The deserialization path (gsonPostProcess → buildVariantEnableFlattenNested → migrateDeprecatedVariantEnableFlattenNestedProperty) correctly migrates the old persisted key. The CREATE TABLE analysis path does NOT have equivalent migration.
8. Test coverage:
- Unit test
testBuildVariantEnableFlattenNestedWithLegacyPropertyKeycovers the deserialization migration path — good. - No test covers CREATE TABLE with the old property name
variant_enable_flatten_nestedto verify it still works (or to verify the expected error). This gap aligns with the backward compatibility issue.
9. Observability:
No new observability needed for a rename.
10. Transaction / Persistence:
The EditLog serialization uses gsonPostProcess which calls the migration method — existing persisted metadata with the old key will be correctly migrated on FE restart. This is correct.
11. FE-BE variable passing:
Thrift field names are unchanged. CreateReplicaTask still uses setVariantEnableFlattenNested (Thrift-generated method). No issues.
12. Performance:
No performance concerns — this is a pure rename.
Summary
The BE rename and the FE deserialization migration are clean and correct. The primary issue is the missing backward compatibility handling in analyzeVariantFlattenNested for users who pass the old property name variant_enable_flatten_nested during CREATE TABLE. This should either be handled by also checking LEGACY_PROPERTIES_VARIANT_ENABLE_FLATTEN_NESTED in the analyze method, or by adding a migration step early in the CREATE TABLE path.
fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java
Show resolved
Hide resolved
| // Deprecated legacy flatten-nested switches. | ||
| // Only if session variable: enable_variant_flatten_nested = true and | ||
| // table property: variant_enable_flatten_nested = true | ||
| // we can enable variant flatten nested otherwise throw error |
There was a problem hiding this comment.
Stale comment: The comment on this line still says table property: variant_enable_flatten_nested = true, but the actual property key is now deprecated_variant_enable_flatten_nested. Consider updating to match.
|
run buildall |
Preserve flatten nested table property on replay
eldenmoon
force-pushed
the
deprecate-flatten-nested
branch
from
8273cc1 to
afdf5ae
Compare
|
run buildall |
TPC-H: Total hot run time: 27224 ms |
TPC-DS: Total hot run time: 168519 ms |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 27161 ms |
TPC-DS: Total hot run time: 168458 ms |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
1 similar comment
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
PR approved by anyone and no changes requested. |
|
PR approved by at least one committer and no changes requested. |
github-actions
bot
added
approved
No description provided.