Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Findings:
-
be/test/exec/exchange/exchange_sink_test.h: removing the direct includes makes this header non-self-contained and breaks the only inclusion site.exchange_sink_test.cppincludes this header before<gtest/gtest.h>andexec/operator/exchange_sink_buffer.h, so names used directly in the header such astesting::Test,std::map,std::queue,Status,ExchangeSinkBuffer,AutoReleaseClosure, andPBackendService_Stubare no longer declared when the compiler parses this file. That turns this cleanup into an immediate BE test compile failure. -
be/test/exec/exchange/exchange_sink_test.h: removing the openingnamespace doris {also leaves the helper declarations at global scope and makes the existing}; // namespace dorisneartransmit_blockv2()an unmatched close. This file no longer parses even before considering the missing includes.
Critical checkpoints:
- Goal of current task: The apparent goal is to simplify
exchange_sink_test.h. The current patch does not accomplish that safely because it breaks compilation of the test target. No test evidence is provided. - Scope and minimality: The patch is small, but not correct; it removes required declarations rather than only removing dead code.
- Concurrency: No new concurrency behavior is introduced by this patch.
- Lifecycle / static initialization: Not applicable for this change.
- Configuration changes: None.
- Compatibility changes: None.
- Parallel code paths: Not applicable.
- Special conditional checks: None added.
- Test coverage: No new or updated tests. More importantly, the existing test appears unable to compile after this change.
- Test results modified: None.
- Observability: Not applicable.
- Transaction / persistence: Not applicable.
- Data writes / modifications: Not applicable.
- FE-BE variable passing: Not applicable.
- Performance: No meaningful performance impact; correctness blocks merge first.
- Other issues: The PR title/body and commit message are placeholders, but the compile break is the blocking issue.
Overall opinion: request changes until the header remains self-contained and syntactically valid.
| // specific language governing permissions and limitations | ||
| // under the License. | ||
|
|
||
| #include <gtest/gtest.h> |
There was a problem hiding this comment.
Removing these includes breaks the only inclusion site of this header. exchange_sink_test.cpp includes exchange_sink_test.h before it includes <gtest/gtest.h> or exec/operator/exchange_sink_buffer.h, but this header uses testing::Test, std::map, std::queue, Status, ExchangeSinkBuffer, AutoReleaseClosure, and PBackendService_Stub directly. After this deletion the test no longer compiles, so the header still needs to include the declarations it depends on instead of relying on transitive include order.
| #include "runtime/runtime_state.h" | ||
| #include "testutil/mock/mock_runtime_state.h" | ||
|
|
||
| namespace doris { |
There was a problem hiding this comment.
Removing the opening namespace doris { makes the helper declarations above ExchangeSinkTest leave the namespace and also turns the existing }; // namespace doris after transmit_blockv2() into an unmatched close. This is a syntax error even before the missing includes are considered.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)