Skip to content

[branch-4.1](be) Move storage field type conversion to core (#65717)#65755

Merged
yiguolei merged 1 commit into
apache:branch-4.1from
csun5285:backport-65717-branch-4.1
Jul 18, 2026
Merged

[branch-4.1](be) Move storage field type conversion to core (#65717)#65755
yiguolei merged 1 commit into
apache:branch-4.1from
csun5285:backport-65717-branch-4.1

Conversation

@csun5285

Copy link
Copy Markdown
Contributor

pick from master #65717

### What problem does this PR solve?

Issue Number: None

Related PR: apache#65717

Problem Summary: PrimitiveType and FieldType conversion helpers lived in TabletColumn even though core serialization and data type code depend on them. Move the exhaustive two-way mapping into core/data_type, migrate all branch-4.1 callers, preserve persisted FieldType ordinals, and fail explicitly for unsupported values. The master-only ConstantColumnReader caller has no counterpart in branch-4.1, so no column_reader change is needed in this backport.

### Release note

None

### Check List (For Author)

- Test: Unit Test
    - BUILD_TYPE=ASAN DISABLE_BE_JAVA_EXTENSIONS=ON bash build.sh --be --clean -j 32
    - BUILD_TYPE_UT=ASAN DISABLE_BUILD_AZURE=ON bash run-be-ut.sh --run --filter=StorageFieldTypeTest.* -j 32 (4 tests passed)
    - build-support/clang-format.sh and build-support/check-format.sh
    - clang-tidy on changed production translation units and storage_field_type_test.cpp; no changed-line warnings
- Behavior changed: Yes (unsupported conversions now throw explicitly; valid mappings and persisted FieldType ordinals are unchanged)
- Does this need documentation: No
@csun5285
csun5285 requested a review from yiguolei as a code owner July 17, 2026 07:10
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@csun5285

Copy link
Copy Markdown
Contributor Author

run buildall

@csun5285

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review outcome: no blocking code issues found. Approval opinion: approve.

Critical checkpoint conclusions:

  • Goal and scope: the PR moves the two-way PrimitiveType/FieldType conversion out of TabletColumn, migrates every in-repository caller, documents the enum-maintenance contract, and adds a registered table-driven unit test. The authoritative 14-file bundle matches its diff, and repository-wide searches found no stale declaration or call to either removed helper.
  • Correctness and error handling: the new switches cover every named enum value. Valid callers keep their mappings; unsupported and out-of-range values now fail deterministically instead of propagating UNKNOWN/INVALID sentinels or indexing beyond the old reverse-mapping array. No ignored Status, exception-in-destructor, ownership, null/COW, allocation, or memory-tracking behavior changes.
  • Compatibility, persistence, and parallel paths: every previously supported sparse-VARIANT tag retains the same persisted byte. Dense-to-sparse writers, recursive arrays/nulls, field/column dispatch, least-common-type handling, and sparse merge/compaction were traced. DecimalV2 changes from UNKNOWN/21 to DECIMAL/16 in its direct binary writer, but both the old and new payloads are rejected by the already-unsupported DecimalV2 sparse reader path; UINT32/UINT64 are offset-only and have no logical SerDe producer. No previously readable rowset or supported rolling-upgrade path regresses.
  • Concurrency, lifecycle, configuration, observability, and security: not applicable. The helpers are pure stateless switches; no shared state, lock, thread, cleanup, configuration, logging/metrics/tracing, or trust boundary changes.
  • Performance: the migrated hot paths replace one enum conversion with another and add no copy, scan, allocation, materialization, or lock.
  • Tests and validation: the new test covers all supported mappings, unsupported/invalid values, two-way round trips, and all persisted FieldType ordinals; recursive CMake globs include both the implementation and test. git diff --check, CheckStyle, and Clang Formatter passed. Per the automation contract I did not build locally; live BE UT, macOS BE UT, and COMPILE checks were still pending at the final sweep. Separately, the PR title check is failing because the configured prefix regex does not accept the dot in [branch-4.1]; the code-review opinion remains approve, but that merge check needs resolution.
  • Review-environment limitation: the expected repository .claude/skills/code-review/SKILL.md file is absent from this checkout, so the review followed the bundle contract and the available Doris review workflow directly.

User focus: no additional focus points were provided; the complete PR was reviewed.

Review completion: one full cyclic review round plus an unchanged-set convergence pass; both normal full-review subagents and the separate risk-focused subagent returned NO_NEW_VALUABLE_FINDINGS, every initial risk has an evidence-backed conclusion, and the final changed-file/unresolved-candidate sweep is clean.

@yiguolei
yiguolei merged commit d2926d1 into apache:branch-4.1 Jul 18, 2026
29 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants