Repository navigation
[refactor](be) Strengthen enum type safety in execution and expressions - #69174
Merged
Merged
Conversation
Mryange
requested review from
Gabriel39,
airborne12,
csun5285,
eldenmoon,
gavinchou and
yiguolei
as code owners
October 16, 2026 06:28
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review of PR #69174 at f98de0c covered all 104 changed paths (66 source files, 34 tests, 4 benchmarks), callers and parallel paths. No actionable defect was found. No existing inline threads or additional user focus were supplied.
Critical checkpoint conclusions:
- Goal and proof: scoped enums, typed join/runtime-filter/WKB interfaces, and constexpr scalar indices implement the stated type-safety refactor. Existing tests were adapted; runtime equivalence has not been established by execution.
- Scope and clarity: changes remain focused on enum types and consumers; benchmark formatting does not change logic.
- Concurrency: fragment sharing, thread-local state, operator scheduling, locks, and callbacks retain their existing behavior; no new concurrency mechanism.
- Lifecycle/static initialization: original and cloned expression contexts retain open/close scope pairing and shared fragment ownership. New constants are constexpr; no new dynamic global initialization or ownership cycle.
- Configuration: none added or modified.
- Compatibility: geometry tags, WKB byte order/type codes, and parse-status numbers are unchanged, with numeric conversion at existing encoding boundaries. Changed C++ interfaces and template declarations/instantiations agree. No new libc dependency or serialized layout.
- Thrift/protobuf: no definitions or field IDs changed; existing TJoinOp/PFilterType enums are reused.
- Parallel paths: checked source/test/benchmark callers, normal and ASOF join dispatch/instantiations, tablet modes, and v1/v2 expression paths; no missed migration found.
- Conditions/errors: qualified comparisons preserve their predicates. The new minMemSize default rejects types absent from its two supported line/polygon callers; WKT parser success remains integer zero and unknown runtime-filter handling remains unchanged.
- Test coverage: existing function, geometry/endian/malformed-input, scan, and routing cases retain their expected behavior. BE compilation, unit/regression tests, and clang-tidy were not run.
- Test results: no golden files changed; test expectations change only enum qualification.
- Observability: no new execution behavior needs counters; existing profiles and error paths remain, including explicit integer formatting of an unknown function scope.
- Transactions/persistence: no transaction, EditLog, visibility/version, rowset, or delete-bitmap protocol changes.
- Writes/atomicity: tablet routing enumerator values and selection behavior are preserved; no new write/recovery or atomicity behavior.
- FE/BE variables: no added transmitted variable or sending path.
- Performance/memory: no changed runtime algorithm, per-row allocation, column copy, nullable shape, or ownership behavior. The PlanNodes header supplies the nested enum needed by the typed friend declaration; static header hygiene passes. No runtime benchmark or completed-ELF compatibility claim.
- Other risks: independent review covers numeric/serialization boundaries and residual callers; final risk dispositions and convergence are recorded in the shared ledger.
Validation: build-support/check-build-hygiene.sh passed as a pure-text static gate (header layering/budgets/PCH, extern-template pairing, unity-skip coverage). No builds or runtime tests were attempted. The review's testing limitation is the absence of compilation and execution evidence.
Review converged in round 1. All three reviewers returned NO_NEW_VALUABLE_FINDINGS; both initial risk focuses were dismissed with concrete evidence and independently checked. No unresolved candidate or uncovered changed-file scope remains.
Review converged after 1 round(s).
### What problem does this PR solve? Issue Number: N/A Problem Summary: Unscoped enums and integer interfaces weaken compile-time type checking. Use scoped enums for tablet routing, function state scope and geometry, type Hash Join template parameters and runtime-filter/WKB interfaces, and update callers. Preserve encoded values and leave DateTime value types unchanged. ### Release note None ### Check List (For Author) - Test: No need to test (type-safety refactoring covered by existing tests); clang-format 16, git diff --check and build hygiene passed. Compilation, unit tests, regression tests and clang-tidy were not run. - Behavior changed: No - Does this need documentation: No
Mryange
force-pushed
the
refine-be-enum-types
branch
from
October 16, 2026 06:53
f98de0c to
8342f80
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29593 ms |
Contributor
TPC-DS: Total hot run time: 152308 ms |
Contributor
ClickBench: Total hot run time: 24.71 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
yiguolei
approved these changes
Oct 16, 2026
csun5285
approved these changes
Oct 18, 2026
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all 104 changed paths at 8342f80. No actionable defect introduced by this refactor was identified. Existing inline threads are empty, so there are no duplicate findings or existing blocking comment IDs. The user supplied no additional focus points.
Critical checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and proof | Scoped enums and typed interfaces strengthen source checking. Static comparison preserves supported enum values and behavior; existing tests are migrated, but no new behavior test is added. |
| Scope and clarity | Changes are focused on enum typing and callers, constant indexes, and removing an unused JSON enum. All 104 paths were read; 84 contain only expected qualifications/template changes or whitespace, with 20 boundary files checked separately. |
| Concurrency and locks | No threads, lock acquisition/order, shared-state mutations, atomics, or critical-section work change. Fragment state sharing and thread state isolation remain intact. |
| Lifecycle and initialization | Original contexts still open fragment and thread scopes; clones inherit fragment state and open thread scope. Close symmetry and ownership are preserved. No cross-translation-unit initialization dependency is introduced. |
| Configuration | No configuration items or dynamic configuration behavior change. |
| Compatibility | Geometry type/status numbers, internal encoded headers, WKB type/endian bytes, and DateTime value types are preserved. Typed join/filter APIs are internal source changes; no persisted layout or mixed-version protocol change was found. |
| Thrift and protobuf | No gensrc definitions, field IDs, enum wire values, or optionality change. |
| Parallel paths | Checked hash build/probe/ASOF/mark joins, declarations/friends/instantiations, both tablet writers and partitioner, function/UDF and benchmark callers, WKT/WKB, format-v2 cast/delete and storage LIKE users. No stale consumers found. |
| Conditions and errors | Existing success/error comparisons retain their values. WKT checks the parser's integer success value 0. WKB minMemSize's added default cannot affect its only two call types, line and polygon. Existing numeric malformed-input sentinels are unchanged. |
| Test coverage | Unit tests and benchmark callers retain their original inputs and expectations; geometry tests include both endiannesses, invalid order/types, SRID and truncation. No tests or builds were run for this review. |
| Expected results | No regression output files or expected SQL results change. Updated unit-test expectations refer to the same enum values. |
| Observability | Existing timers, metrics and error propagation remain; the function-scope diagnostic explicitly converts the enum to int. No new execution path requires additional instrumentation. |
| Transactions and persistence | No EditLog, commit/publish, failover, rowset version, or delete-bitmap protocol change. |
| Writes and atomicity | Tablet routing choices and write operations are unchanged; no transaction or crash-recovery behavior changes. |
| FE/BE propagation | No new transmitted variables or FE/BE sending paths are introduced. Existing TJoinOp and PFilterType values are preserved. |
| Performance and memory | No row-loop work, allocation, copying, COW/null handling, I/O, or algorithm complexity changes. The new PlanNodes dependency in column_vector.h passed the static header gates; no runtime performance claim was made. |
| Other checks | Header layering/budgets, PCH whitelist, extern-template pairing and test source/unity coverage pass the repository's pure-text build-hygiene script. No further introduced issue was found. |
Validation limits: this was a static review. No compilation, unit/regression execution, clang-tidy, final ELF/glibc audit, or performance measurement was performed. GitHub statuses on this SHA report COMPILE, BE UT and regressions successful, with the coverage-merge check failing; those logs were not inspected and these statuses are not independent runtime validation by this review.
Convergence: converged in round 1. Both normal reviewers and the separate risk reviewer returned NO_NEW_VALUABLE_FINDINGS on the same empty inline set. All changed paths, candidates and risk foci are accounted for; no unresolved changed-code candidate or coverage gap remains.
Review converged after 1 round(s).
OIiveirra
added a commit
to OIiveirra/doris
that referenced
this pull request
Oct 19, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69032 Problem Summary: Official upstream/master 01e735b contains the scoped-enum migration from apache#69174. Its geometry/status/WKB changes conflict with this branch's typed constructors, byte-oriented parser and regression coverage. Merge the official branch while preserving both parents' behavior. Qualify the spatial enum values, explicitly convert wire-byte and WKB integer boundaries, retain the new metadata error status, and use an equivalent boolean assertion for a shape enum without an ostream overload. Preserve upstream Iceberg table-property operations and the spatial schema checks; Connector API remains 19.0. ### Release note None ### Check List (For Author) - Test: Unit Test / Manual test - Standard ASAN/clang21 BE UT: WkbParseTest, VGeoFunctionsTest and GeoTypesTest. - Standard FE UT: Iceberg DDL/catalog operations, column evolution, spatial write validation and the complete Connector SPI suite. - clang-format16, header hygiene, source whitespace check and Standards/Spec self-review. - Full-file clang-tidy reports 45 existing-line diagnostics; all map to unchanged HEAD lines and no modified-line diagnostics are present. No unrelated refactoring or NOLINT additions. - Behavior changed: No manual feature changes; inherit official master and preserve typed/legacy parsing semantics. - Does this need documentation: No; the two new review requests are analyzed separately and remain unimplemented.
5 of 13 tasks
3 of 16 tasks
morningman
added a commit
that referenced
this pull request
Oct 19, 2026
…enum class (#69297) ### What problem does this PR solve? Issue Number: None Related PR: #69018, #69174 Problem Summary: **In short.** BE UT fails with `BE BUILD FAIL` for every PR against master: `be/test/exprs/function/function_h3_test.cpp`, added by #69018, no longer compiles since #69174 made `FunctionContext::FunctionStateScope` an `enum class`. This PR spells the 8 affected calls with the scoped enumerators, so the test compiles again. Nothing else changes. **Background.** #69174 (merged 2026-10-18) turned `FunctionContext::FunctionStateScope` into an `enum class` and rewrote every caller from `FunctionContext::FRAGMENT_LOCAL` to `FunctionContext::FunctionStateScope::FRAGMENT_LOCAL`, and the same for `THREAD_LOCAL`. #69018 (merged 2026-10-19) added an H3 function test written against the old, unscoped names. The two PRs were merged independently; together they leave a test on master that does not compile. The BE UT pipeline merges each PR into the latest master before it builds `doris_be_test`, so the break hits every PR, whatever it changes. **The problem, and what it cost.** Since #69261 fixed the other test break of the day (left by #68447), every BE UT build for a PR against master stops at this file, for example [build 1073803](http://43.132.222.7:8111/viewLog.html?buildId=1073803) (#69257, merged into master `41a58afba2e`): ``` FAILED: [code=1] test/CMakeFiles/doris_be_test.dir/exprs/function/function_h3_test.cpp.o ../test/exprs/function/function_h3_test.cpp:66:62: error: no member named 'FRAGMENT_LOCAL' in 'doris::FunctionContext' ../test/exprs/function/function_h3_test.cpp:67:62: error: no member named 'THREAD_LOCAL' in 'doris::FunctionContext' ... 8 errors generated. ninja: build stopped: subcommand failed. ``` No BE unit test runs for those PRs, so their BE UT result says nothing about the PR until this is fixed. **How this PR fixes it.** The 8 `open` / `close` calls in `function_h3_test.cpp` now use `FunctionContext::FunctionStateScope::FRAGMENT_LOCAL` / `THREAD_LOCAL`, as #69174 did for the other function tests. The test logic is unchanged. **Results.** `-fsyntax-only` with the flags of each file's own entry in `be/ut_build_ASAN/compile_commands.json` (clang 22, `-DBE_TEST`): | File | master `41a58afba2e` | this PR | |---|---|---| | `function_h3_test.cpp` | the 8 errors CI reports | compiles | | the 4 test files #69261 fixed (`data_type_serde_arrow_test.cpp`, `data_type_variant_v2_serde_jsonb_test.cpp`, `variant_v2_execution_test.cpp`, `vparquet_writer_test.cpp`) | compile | compile (not touched) | | `function_array_zip_test.cpp` (baseline, not touched) | compiles | compiles |
Mryange
added a commit
to Mryange/doris
that referenced
this pull request
Oct 21, 2026
Issue Number: N/A Related PR: apache#69174 Problem Summary: Use scoped enums and typed fields for storage states, character categories, compaction triggers, and binlog operations to prevent implicit integer conversions and mixing unrelated values. Preserve persisted values and Cloud source compatibility; adapt callers and existing tests. None - Test: ASAN BE build, header hygiene, and diff whitespace checks passed. Existing unit tests were adapted but not run. - Behavior changed: No - Does this need documentation: No
yiguolei
pushed a commit
that referenced
this pull request
Oct 22, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: #69174 Problem Summary: Unscoped enums and integer fields used for storage states, character categories, compaction triggers, and binlog operations allow unintended implicit conversions and mixing unrelated values. Use scoped enums throughout these paths, store enum values in typed fields and containers, and convert explicitly at serialization, indexing, and logging boundaries. Replace anonymous size enums with constexpr constants and the IK dictionary's binary node state with a bool. Persisted numeric values and storage formats remain unchanged. Keep compatibility constants and compaction-type stream output so Cloud source and tests do not need changes. ### Release note None ### Check List (For Author) - Test - [ ] Regression test - [ ] Unit Test - [x] Manual test: `BUILD_TYPE=ASAN bash ./build.sh --be -j48` passed. Header hygiene and diff whitespace checks passed. Existing unit tests were adapted but not run. - [x] No need to test or manual test. Explain why: - [x] This is a refactor/code format and no logic has been changed. - Behavior changed: - [x] No. - [ ] Yes. - Does this need documentation? - [x] No. - [ ] Yes. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
csun5285
added a commit
that referenced
this pull request
Oct 23, 2026
…ash (#69487) ### What problem does this PR solve? Issue Number: None Related PR: #69223, #69174, #69327, #69183 Problem Summary: BE UT of every PR based on master fails since 2026-10-22. Two test-only breakages were merged by PRs whose own CI ran on an older master. 1. doris_be_test does not compile (TeamCity: "BE BUILD FAIL"): be/test/exprs/function/function_string_test.cpp:2971: error: no matching conversion for functional-style cast from 'std::string' to 'Field' be/test/exprs/function/function_string_test.cpp:2982: error: no member named 'FRAGMENT_LOCAL' in 'doris::FunctionContext' function_translate_mixed_const_map_test, added by #69223, builds a Field directly from std::string and uses FunctionContext::FRAGMENT_LOCAL. Field must be created with Field::create_field<TYPE_STRING>() now, and #69174 made FunctionStateScope an enum class. Use both current spellings, as the rest of be/test does. 2. Once it compiles, doris_be_test aborts (TeamCity: "CORE DUMPED") in DefaultAndConfiguredLimits/ExternalTableWriterPartitionLimitTest. IcebergRejectsNextPartitionBeforeOpeningAFile/0: shared_ptr_base.h:1344: [_Tp = doris::iceberg::Schema]: Assertion '__p != nullptr' failed. #69327 made VIcebergTableWriter validate every block against the schema pinned by open(): `iceberg::validate_write_block(*_schema, ...)`. The test added by #69183 skips open() and calls write_prepared_block() directly, so _schema is null. open() always sets _schema in production, so the writer stays unchanged. The test now pins a one-field schema that matches its block (optional INT "p", the source field of its identity partition), as viceberg_table_writer_test.cpp does. Co-authored-by: csun5285 <pianxian2@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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.
What problem does this PR solve?
Issue Number: N/A
Problem Summary:
Root cause: unscoped enums implicitly convert to integers, and integer parameters accept unrelated enum values, weakening compile-time type checking in BE execution and expressions.
Use scoped enums for tablet routing, function state scope, and geometry types/statuses. Replace integer Hash Join template parameters and runtime-filter/WKB interfaces with their actual enum types, and update source, test, grammar, and benchmark callers. Use constexpr constants for scalar-group indexes and remove an unused JSON enum. Existing encoded values and DateTime value types remain unchanged.
Release note
None
Check List (For Author)
Check List (For Reviewer who merge this PR)