[refine](Field) rename Tuple to Struct in BE core types#63093
Open
Mryange wants to merge 1 commit intoapache:masterfrom
Open
[refine](Field) rename Tuple to Struct in BE core types#63093Mryange wants to merge 1 commit intoapache:masterfrom
Mryange wants to merge 1 commit intoapache:masterfrom
Conversation
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.
I found one blocking issue: the PR introduces a formatting regression in a BE core header comment. The type rename itself is mechanically consistent in the changed code, and I did not find a behavior, serialization, concurrency, memory tracking, lifecycle, or FE/BE protocol issue.
Critical checkpoint conclusions:
- Goal/test: The goal is to rename the BE Field storage type from Tuple to Struct; the changed code appears to accomplish this. Existing affected unit tests were updated, but no test run is reported here.
- Scope: The change is small and focused.
- Concurrency/lifecycle/config/compatibility: Not applicable; this is a local type/comment rename with no new runtime state, config, wire format, or storage format change.
- Parallel paths: The main TYPE_STRUCT Field construction/use paths touched by this rename are updated; remaining Tuple references found are unrelated runtime tuple descriptors, std::tuple usage, or existing nested-type comments outside the changed symbol.
- Tests/results: Existing tests were updated only for compilation naming; no generated outputs are involved.
- Observability/transactions/data writes: Not applicable.
- Performance: No performance-impacting logic changes found.
User focus: no additional user-provided focus was present.
| * Example: simple columns like UInt64 or FixedString store their values contiguously in single memory buffer. | ||
| * | ||
| * Example: Tuple store values of each component in separate subcolumn, so the values of Tuples with at least two components are not contiguous. | ||
| * Example: Struct stores values of each component in separate subcolumn, so the values of Structs with at least two components are not contiguous. |
Contributor
There was a problem hiding this comment.
This line lost the indentation used by the surrounding Doxygen block. BE C++ formatting is enforced by build-support/check-format.sh, and this changed line is now visibly misaligned with the adjacent * prefixes. Please keep the original comment indentation while applying the rename.
Suggested change
| * Example: Struct stores values of each component in separate subcolumn, so the values of Structs with at least two components are not contiguous. | |
| * Example: Struct stores values of each component in separate subcolumn, so the values of Structs with at least two components are not contiguous. |
Contributor
Author
|
run buildall |
Mryange
force-pushed
the
rename-tuple-to-struct
branch
from
f810a69 to
77d1414
Compare
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result: no additional blocking issues found in the current head diff.
Critical checkpoint conclusions:
- Goal and tests: The PR renames the BE Field C++ storage wrapper for TYPE_STRUCT from Tuple to Struct and updates directly affected call sites/tests. The current diff is consistent with that goal; existing struct-related tests are updated, though I did not run them in this review environment.
- Scope: The change is small and focused on BE core type naming plus related test call sites.
- Concurrency/lifecycle: No new concurrency, locks, thread lifecycle, or static initialization behavior is introduced.
- Configuration/compatibility: No new configuration, serialized format, thrift protocol, storage format, or user-visible SQL behavior change was found. TYPE_STRUCT semantics and serialization paths remain unchanged.
- Parallel paths: I checked remaining Tuple references. Remaining production references are comments in unrelated runtime/config tuple-row contexts or existing non-code comments, not missed uses of the renamed Field storage type in the changed path.
- Error handling: No new Status/Exception propagation issue was introduced by the rename. The changed exception messages remain consistent with ColumnStruct semantics.
- Tests/results: No new result files are introduced. The touched unit tests only update construction from Tuple to Struct.
- Observability/performance/memory: No new logging/metrics need was identified; no new allocations, hot-path copies, COW-column mutation patterns, or memory tracking changes are introduced.
User focus: .opencode-review.KIRx2t/review_focus.txt contains no additional focus points, so I performed the normal full PR review.
Existing review context: I did not duplicate the already-known inline thread about column.h Doxygen indentation; that file is not part of the current PR files returned by GitHub for head 77d1414.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 30060 ms |
Contributor
TPC-DS: Total hot run time: 170561 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run Vault_P0 |
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
2 participants
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:
The C++ struct Tuple in be/src/core/field.h is a type alias for FieldVector used to represent STRUCT-typed field values. However, the name "Tuple"
is misleading — this type corresponds to the STRUCT data type, not a tuple. This rename aligns the C++ type name with the actual semantic: Tuple
→ Struct, along with all references in comments, error messages, and variable names.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)