[refine](column) enforce nullable nested types for array and map#62491
[refine](column) enforce nullable nested types for array and map#62491Mryange wants to merge 2 commits intoapache:masterfrom
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
I found one blocking issue.
be/src/core/data_type/data_type_array.cpp: forcing every array child throughmake_nullable()changes the FE-visible type metadata that BE exports.DataTypeArray::to_protobuf()now always reportscontains_null=true, and FE'sFoldConstantRuleOnBE.convertToNereidsType()reconstructs NereidsArrayTypefrom that flag. That means BE constant folding can no longer round-tripARRAY<... NOT NULL>precisely. A concrete case isCAST([1] AS ARRAY<INT NOT NULL>): after folding, FE will seeARRAY<INT>instead of the original non-null-child type. FE still treatsArrayType.containsNullas semantically significant in exact matching (fe/fe-type/.../ArrayType.java,Type.matchExactType()), so this is a real behavior regression, not just an internal refactor.
Critical checkpoint conclusions:
- Goal of the task: standardize BE array/map nested types as nullable. The code mostly does that, but it does not preserve the existing FE-visible array child nullability contract on the BE->FE constant-folding path, so the goal is not achieved safely end-to-end.
- Minimality/focus: the patch is small and focused, but it misses one required follow-through path (
PTypeDescexport / FE reconstruction). - Concurrency: no concurrency concerns in the touched code.
- Lifecycle/static init: no special lifecycle concerns.
- Config changes: none.
- Compatibility: this changes transmitted complex-type metadata behavior for arrays and is not compatibility-safe as written.
- Parallel code paths: I checked the related SerDe/cast/type-factory paths. The array protobuf path is the one that remains inconsistent with FE expectations.
- Special condition checks: the new
DORIS_CHECKassertions are reasonable for invariant enforcement. - Test coverage: insufficient. There is no regression test covering FE<->BE round-trip of
ARRAY<... NOT NULL>through BE constant folding or protobuf type export. - Test result changes: none.
- Observability: not needed for this refactor.
- Transaction/persistence/data-write concerns: none.
- FE/BE variable passing: no new fields, but existing transmitted type metadata is affected and not preserved correctly.
- Performance: no material issue identified.
- Other issues: none beyond the blocking metadata regression above.
Because of the array type-metadata regression, I am requesting changes rather than approving.
|
|
||
| DataTypeArray::DataTypeArray(const DataTypePtr& nested_) : nested {nested_} {} | ||
| DataTypeArray::DataTypeArray(const DataTypePtr& nested_) { | ||
| DataTypePtr nullable_nested = make_nullable(nested_); |
There was a problem hiding this comment.
Wrapping every array child in make_nullable() changes the type metadata you export later. DataTypeArray::to_protobuf() uses nested->is_nullable(), and FE's FoldConstantRuleOnBE.convertToNereidsType() reconstructs ArrayType from that flag. After this change, a folded expression like CAST([1] AS ARRAY<INT NOT NULL>) will round-trip back to FE as ARRAY<INT> because BE now always reports contains_null=true. FE still treats ArrayType.containsNull as part of exact type matching, so this is a real regression in the constant-folding path, not just an internal invariant cleanup. Please preserve the original child-nullability flag on the exported type metadata path or add a compatibility fix/test for it.
|
run buildall |
What problem does this PR solve?
Problem Summary:
This PR makes the nested types inside
ArrayandMapexplicitly nullable in BE type implementations, instead of relying on implicit caller-side conventions.DataTypeArraynow always stores nullable nested element typeDataTypeMapnow always stores nullable key/value typesDataTypeArraySerDeandDataTypeMapSerDeare updated to follow the same invariantRelease note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)