[improvement](be) Add release-enabled Doris check macros#63730
Open
mrhhsg wants to merge 1 commit into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
/review |
mrhhsg
force-pushed
the
codex/doris-check-comparison-macros
branch
2 times, most recently
from
ea4605c to
641d320
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review result: one blocking issue found.
Critical checkpoints:
- Goal/test: The PR adds release-enabled Doris check macros and tests basic success/failure/stream-laziness behavior, but it misses streamed messages containing fmt braces.
- Scope: The change is focused and small.
- Concurrency/lifecycle/config/compatibility/transactions/data writes/FE-BE variables: Not applicable to this macro refactor.
- Parallel paths: status.h re-exports the new header; direct common/check.h include path is covered structurally.
- Error handling: The new failure helper incorrectly forwards an already-rendered stream message as a fmt format string through Status::FatalError in debug builds.
- Tests: Unit coverage exists for simple streaming, but not for brace-containing streamed context, which exposes the issue.
- Observability/performance: No additional issue found; success paths avoid stream operand evaluation as intended.
User focus: No additional user-provided review focus was supplied.
Issue Number: None
Related PR: None
Problem Summary: Add a dedicated common/check.h header for Doris check macros. DORIS_CHECK accepts streamed context through the usual << syntax while avoiding evaluation of streamed operands on successful checks. DORIS_CHECK_EQ/NE/LT/LE/GT/GE are intended for invariants that should remain checked in Release builds: Debug builds map them to the corresponding DCHECK_* macros, while Release builds evaluate each operand once, compare with the requested operator, and throw through DORIS_CHECK-style fatal error handling with a message that includes both compared expressions and their actual values. Release comparison checks also accept streamed context. status.h re-exports common/check.h to keep existing includes compatible. The added tests cover successful comparisons, single-evaluation behavior, stream operand laziness, and failure behavior for Debug/Release builds.
None
- Test: Manual test
- DORIS_HOME=$PWD ninja -C be/ut_build_ASAN src/common/CMakeFiles/Common.dir/check.cpp.o test/CMakeFiles/doris_be_test.dir/common/status_test.cpp.o
- DORIS_HOME=$PWD ninja -C be/ut_build_ASAN Common
- Compiled a small -DNDEBUG translation unit that includes common/check.h directly and exercises the Release macro branch, streamed DORIS_CHECK context, streamed comparison-check context, and stream-operand laziness on success.
- Compiled a small -DNDEBUG translation unit that checks DORIS_CHECK/DORIS_CHECK_EQ statement behavior under if/else.
- git diff --check
- build-support/check-format.sh
- Attempted ./run-be-ut.sh --run --filter=StatusTest.*, but stopped it after it expanded into a broad BE UT rebuild.
- Attempted build-support/run-clang-tidy.sh on the changed files before this follow-up; it failed on pre-existing/toolchain header diagnostics outside this change.
- Behavior changed: No
- Does this need documentation: No
mrhhsg
force-pushed
the
codex/doris-check-comparison-macros
branch
from
641d320 to
4ae57b2
Compare
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
I found one blocking issue: one DCHECK-to-DORIS_CHECK conversion turns an existing user-facing InvalidArgument path into a fatal invariant failure, making the following validation branch unreachable in Release builds.
Critical checkpoint conclusions:
- Goal and tests: the goal is to add release-enabled Doris check macros; the check macro tests cover the new macro mechanics, but the json_remove invalid-arity path is not covered after this conversion.
- Scope/focus: the macro addition is focused, but converting argument validation-adjacent DCHECKs needs care.
- Concurrency/lifecycle/config: no new concurrency, lifecycle, or configuration concerns found.
- Compatibility: no storage/protocol compatibility issue found; one user-visible error-handling compatibility issue is raised inline.
- Parallel paths: I reviewed the other changed JSONB checks and did not find another changed line with an immediately following explicit InvalidArgument branch like this one.
- Error handling: blocking issue found because a recoverable InvalidArgument becomes FatalError/Exception before the intended Status return. The existing check.cpp fmt-format concern was already present in the supplied review context and the current patch shows it addressed.
- Memory/transactions/persistence/observability: no additional issue found.
- Tests: add or preserve coverage for invalid json_remove arity so this remains a normal error instead of a fatal check.
User focus: no additional user-provided review focus was supplied.
| uint32_t result, size_t input_rows_count) const override { | ||
| DCHECK_GE(arguments.size(), 2); | ||
| DORIS_CHECK_GE(arguments.size(), 2); | ||
|
|
Contributor
There was a problem hiding this comment.
This check now fires before the existing validation below, so json_remove with fewer than two arguments no longer returns Status::InvalidArgument("json_remove requires at least 2 arguments") in Release builds; it throws a fatal check exception instead. Since this function is variadic (get_number_of_arguments() == 0, is_variadic() == true) and already has a user-facing arity check immediately after this line, the arity failure is not an invariant for DORIS_CHECK_GE. Please leave this as a non-fatal validation path, for example by removing this DORIS_CHECK_GE and relying on the existing if (arguments.size() < 2) branch.
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31808 ms |
Contributor
TPC-DS: Total hot run time: 172282 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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: None
Related PR: None
Problem Summary: Add a dedicated
common/check.hheader for Doris check macros.DORIS_CHECKaccepts streamed context through the usual<<syntax while avoiding evaluation of streamed operands on successful checks.DORIS_CHECK_EQ/NE/LT/LE/GT/GEare intended for invariants that should remain checked in Release builds: Debug builds map them to the correspondingDCHECK_*macros, while Release builds evaluate each operand once, compare with the requested operator, and throw through the existingDORIS_CHECK-style fatal error path with a message that includes both compared expressions and their actual values. Release comparison checks also accept streamed context.status.hre-exportscommon/check.hto keep existing includes compatible.Release note
None
Check List (For Author)
DORIS_HOME=$PWD ninja -C be/ut_build_ASAN src/common/CMakeFiles/Common.dir/check.cpp.o test/CMakeFiles/doris_be_test.dir/common/status_test.cpp.oDORIS_HOME=$PWD ninja -C be/ut_build_ASAN Common-DNDEBUGtranslation unit that includescommon/check.hdirectly and exercises the Release macro branch, streamedDORIS_CHECKcontext, streamed comparison-check context, and stream-operand laziness on success.-DNDEBUGtranslation unit that checksDORIS_CHECK/DORIS_CHECK_EQstatement behavior underif/else.git diff --checkbuild-support/check-format.sh./run-be-ut.sh --run --filter=StatusTest.*, but stopped it after it expanded into a broad BE UT rebuild.build-support/run-clang-tidy.shon the changed files before this follow-up; it failed on pre-existing/toolchain header diagnostics outside this change.