Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#16022, apache#20461, apache#51418, apache#61092, apache#67171 Problem Summary: be/test/CMakeLists.txt keeps a "todo: need fix those ut" list of tests that are not built. Most of them stopped building years ago, so they have not run since and no longer match the code. This change fixes or removes each of them. Ported and built again: - plain, bitshuffle, binary plain, binary prefix and RLE page tests. They used ColumnBlock/ColumnBlockView, which apache#16022 removed. They now create builders with PageBuilderHelper::create() and read pages into a MutableColumnPtr. binary_prefix_page_test already had a copy of its test on the column API, so the ColumnBlock copy is removed. - column_reader_writer_test, ported to the column API. It also needed: - seek_to_ordinal(0) before the sequential read. apache#51418 removed seek_to_first() and dropped the calls without a replacement, so the read crashed on a null ordinal index iterator. - precision 27 and frac 9 in the DECIMAL column meta, because ColumnWriter now builds a data type from the meta. - valid DATETIME values, because reading checks them. - CHAR is now read back without its trailing '\0' padding. - The BITMAP default value case is removed. DefaultValueColumnIterator can not parse a BITMAP default value from a string any more. - segment_iterator_apply_index_expr_test, excluded since apache#61092 added it. apache#67171 later made _apply_index_expr() skip virtual columns when _row_bitmap is empty. The fixture never filled it, so 7 of 11 cases failed. The fixture now selects every row, like _lazy_init() does. - config_validator_test. Its config with an invalid default broke config::init() when the test binary starts, and the test reset every config. The defaults are valid now, and the validator is checked by loading a conf file with set_to_default=false, which only touches the configs in the file. - metrics_action_test. It redefined HttpChannel::send_reply(), which clashes with the real one. It now starts an EvHttpServer and reads the response with HttpClient. - large_int_value_test. Its suite name clashed with core/value/large_int_value_test.cpp. The two boundary cases move there, and the benchmark that loops 10M times is dropped. Duplicate cases are dropped while porting: - The Equal and Sequence plain page cases. PLAIN only copies memory, so the shape of the data does not matter and the Random cases cover them. - test_v_default_value. The ported test_default_value covers more types and also seeks. - MetricsActionTest.prometheus_no_prefix. It only checks the registry output format, which metrics_test already covers. Removed: - heartbeat_server_test, decimal_value_test, decompress_test and url_coding_test. The code they test is gone or never matched their API. - remote_file_system_test and remote_rowset_gc_test. They need a real S3/HDFS/broker, were DISABLED since they were added, and call APIs that no longer exist. - The runtime/jsonb_value_test.cpp entry. The file moved to core/value and already runs. frame_of_reference_page_test stays excluded. FrameOfReferencePageDecoder can not read pages with next_batch(), and seeking backward throws since apache#53675. Two headers did not compile on their own, and the page tests include them first. page_decoder.h now includes storage/olap_define.h for DISALLOW_COPY_AND_ASSIGN, and rle_page.h includes storage/types.h for TypeTraits. No behavior change. rle_page_test has one NOLINT for clang-analyzer-optin.cplusplus.VirtualCall. It is a false positive: the analyzer reports uninitialized fields in the RlePageDecoder constructor and then treats the decoder as still being built. ### Release note None ### Check List (For Author) - Test: Unit Test - PlainPageTest, BitShufflePageTest, BinaryPlainPageTest, BinaryPrefixPageTest, RlePageTest, ColumnReaderWriterTest, SegmentIteratorApplyIndexExprTest, ConfigValidatorTest, MetricsActionTest, LargeIntValueTest - Related suites: BinaryDictPageTest, BinaryPlainPageV2Test, BinaryPlainPageV3Test, ColumnReaderCacheTest, EncodingInfoTest, ColumnReaderTest, ConfigTest, JsonBinaryValueTest - 18 suites, 186 cases, all passed - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
csun5285
requested review from
Gabriel39,
airborne12,
eldenmoon,
gavinchou,
liaoxin01,
luwei16,
morningman and
yiguolei
as code owners
September 24, 2026 08:19
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: two issues are called out inline.
Critical checkpoint conclusions:
- Goal and test proof: the PR mostly reactivates or removes the excluded BE tests, but it deletes the only focused specification of the still-live remote-tablet-GC marker-to-directory cleanup lifecycle instead of moving it onto the existing hermetic remote-filesystem mock.
- Scope, build, and compatibility: the changes are focused on test migration/removal plus two header self-containment fixes. The CMake glob includes each surviving restored source once, no duplicate GoogleTest or externally linked helper collision was found, and there is no runtime configuration, FE/BE protocol, storage-format, transaction, or persistence change. The broad
olap_define.hdependency reaches only one additional TU through this edge and is not a blocking issue. - Data correctness and parallel paths: the migrated page-decoder and vectorized-column representations match their production contracts. Active schema-change regressions exercise the BITMAP-default and nullable-default production paths end to end, so those earlier direct-unit concerns were dismissed rather than reported.
- Concurrency and lifecycle: the changed fixtures restore shared flags, isolate filesystem state, use an ephemeral HTTP port, and destroy the server before its stack handler; no production concurrency issue was found. The remote-GC deletion remains a coverage defect because no remaining test observes remote directory deletion or success-only marker cleanup.
- Determinism and performance: the reactivated metrics endpoint test fixes output order from multiple
std::unordered_mapinstances, making a semantically correct response toolchain-dependent. No other material performance or determinism issue was substantiated. - Validation: this review was static only, as required; I did not run builds or tests. At submission time COMPILE, formatting, Checkstyle, license, and secret checks pass, while BE UT, regression, cloud regression, and performance jobs remain pending. The PR checklist does not record author-run validation. No user-specific focus points were supplied.
The review completed three rounds and converged at exact head 270f1d142f62d4bc11523ac23f516901200bf67e; all other candidates were deduplicated or dismissed with code/test evidence.
|
|
||
| st = k_engine->tablet_manager()->drop_tablet(10005, 0, true); | ||
| ASSERT_EQ(Status::OK(), st); | ||
| tablet->data_dir()->perform_remote_tablet_gc(); |
Contributor
There was a problem hiding this comment.
Preserve the live remote-tablet-GC lifecycle coverage.
Please port this lifecycle case to a hermetic backend instead of deleting it. Although the file was excluded before this PR, it is still the only specification of the live drop -> REMOTE_TABLET_GC_PREFIX marker -> perform_remote_tablet_gc() -> remote data/<tablet_id> deletion path, including success-only marker cleanup. No remaining test asserts either the marker or this GC entry point, and tablet_cooldown_test.cpp already provides a local-backed RemoteFileSystemMock with delete_directory_impl, so the scenario can be preserved without S3/HDFS/broker dependencies.
| put_requests_total->increment(2345); | ||
|
|
||
| s_expect_response = | ||
| EXPECT_EQ( |
Contributor
There was a problem hiding this comment.
Make the Prometheus comparison independent of unordered-map iteration.
MetricRegistry::to_prometheus() emits an EntityMetricsByType std::unordered_map, and labels_to_string() directly iterates Labels, another std::unordered_map; this string fixes both metric-group order and the path,type label order. A different standard-library bucket order can fail the reactivated test while producing equivalent Prometheus output. Please parse or normalize the response, or compare complete samples and labels order-insensitively while still checking that no unexpected lines are present.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 27602 ms |
Contributor
TPC-DS: Total hot run time: 152098 ms |
Contributor
ClickBench: Total hot run time: 24.01 s |
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: 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)