You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "csun5285 (via GitHub)" <gi...@apache.org> on 2026/04/14 13:40:03 UTC
[PR] [improvement](segment) Speed up BinaryDictPageBuilder::add and remove dead first/last-value [doris]
csun5285 opened a new pull request, #62493:
URL: https://github.com/apache/doris/pull/62493
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [ ] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [ ] No.
- [ ] Yes. <!-- Add document PR link here. eg: https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [improvement](segment) Speed up BinaryDictPageBuilder::add and remove dead first/last-value [doris]
Posted by "csun5285 (via GitHub)" <gi...@apache.org>.
csun5285 commented on PR #62493:
URL: https://github.com/apache/doris/pull/62493#issuecomment-4249053141
/review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [improvement](segment) Speed up BinaryDictPageBuilder::add and remove dead first/last-value [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62493:
URL: https://github.com/apache/doris/pull/62493#issuecomment-4245527000
# BE Regression && UT Coverage Report
Increment line coverage `80.77% (42/52)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62493_8ebe2e2910eccf4ad4b72e063cceb188de0bd17d_merge/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62493_8ebe2e2910eccf4ad4b72e063cceb188de0bd17d_merge/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 73.57% (27359/37186) |
| Line Coverage | 57.27% (295708/516333) |
| Region Coverage | 54.64% (246927/451882) |
| Branch Coverage | 56.17% (106778/190101) |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [improvement](segment) Speed up BinaryDictPageBuilder::add and remove dead first/last-value [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #62493:
URL: https://github.com/apache/doris/pull/62493#discussion_r3083935712
##########
be/benchmark/benchmark_binary_dict_page.hpp:
##########
@@ -0,0 +1,143 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <benchmark/benchmark.h>
+
+#include <algorithm>
+#include <memory>
+#include <random>
+#include <string>
+#include <vector>
+
+#include "common/status.h"
+#include "storage/segment/binary_dict_page.h"
+#include "storage/segment/options.h"
+#include "storage/segment/page_builder.h"
+#include "util/slice.h"
+
+namespace doris::segment_v2 {
+
+inline std::vector<std::string> bdp_gen_strings(size_t num_unique, size_t avg_len,
+ uint32_t seed) {
+ std::mt19937 rng(seed);
+ size_t lo = std::max<size_t>(1, avg_len / 2);
+ size_t hi = avg_len + avg_len / 2;
+ std::uniform_int_distribution<size_t> len_dist(lo, hi);
+ std::uniform_int_distribution<int> ch_dist('a', 'z');
+ std::vector<std::string> out;
+ out.reserve(num_unique);
+ for (size_t i = 0; i < num_unique; ++i) {
+ size_t len = len_dist(rng);
+ std::string s;
+ s.reserve(len);
+ for (size_t j = 0; j < len; ++j) {
+ s.push_back(static_cast<char>(ch_dist(rng)));
+ }
+ out.push_back(std::move(s));
+ }
+ return out;
+}
+
+// Benchmark BinaryDictPageBuilder::add with an input stream that has an
+// exact target hit rate.
+//
+// range(0) = target hit rate in percent (0..100)
+// range(1) = average string length (bytes)
+// range(2) = batch size per add() call
+//
+// The input is built as row[i] = dict[i % num_unique], where
+// num_unique = kTotalRows * (100 - hit_rate_pct) / 100
+// giving exactly num_unique misses followed by (kTotalRows - num_unique)
+// hits for a deterministic hit rate.
+static void BM_BinaryDictPageBuilder_Add(benchmark::State& state) {
+ const size_t hit_rate_pct = static_cast<size_t>(state.range(0));
+ const size_t avg_len = static_cast<size_t>(state.range(1));
+ const size_t batch = static_cast<size_t>(state.range(2));
+ constexpr size_t kTotalRows = 4096;
+
+ size_t num_unique = kTotalRows - (kTotalRows * hit_rate_pct / 100);
+ if (num_unique == 0) {
+ num_unique = 1;
+ }
+
+ auto dict = bdp_gen_strings(num_unique, avg_len, /*seed=*/12345);
+
+ std::vector<Slice> slices;
+ slices.reserve(kTotalRows);
+ for (size_t i = 0; i < kTotalRows; ++i) {
+ const std::string& s = dict[i % num_unique];
+ slices.emplace_back(s.data(), s.size());
+ }
+
+ PageBuilderOptions opts;
+
+ int64_t items = 0;
+ for (auto _ : state) {
+ state.PauseTiming();
+ PageBuilder* raw = nullptr;
+ Status st = BinaryDictPageBuilder::create(&raw, opts);
+ if (!st.ok()) {
+ state.SkipWithError(st.to_string().c_str());
+ break;
+ }
+ std::unique_ptr<PageBuilder> builder(raw);
+ state.ResumeTiming();
+
+ size_t off = 0;
+ while (off < slices.size()) {
+ size_t cnt = std::min(batch, slices.size() - off);
+ size_t added = cnt;
+ Status add_st = builder->add(
+ reinterpret_cast<const uint8_t*>(slices.data() + off), &added);
+ if (!add_st.ok()) {
+ state.SkipWithError(add_st.to_string().c_str());
+ break;
+ }
+ if (added == 0) {
+ break;
+ }
+ off += added;
+ }
+ benchmark::DoNotOptimize(builder);
+
+ state.PauseTiming();
+ OwnedSlice tmp;
+ (void)builder->finish(&tmp);
Review Comment:
`builder->finish(&tmp)` returns a `Status`, but this new benchmark drops it with `(void)`. That violates the BE common review rule that every `Status` must be checked, and it also means a failed finish path would be silently reported as a successful benchmark iteration. Please handle it like the surrounding `create()` / `add()` calls, e.g. `if (Status st = builder->finish(&tmp); !st.ok()) { state.SkipWithError(st.to_string().c_str()); break; }`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [cleanup](segment)Remove dead get_first_value/get_last_value APIs from PageBuilder [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62493:
URL: https://github.com/apache/doris/pull/62493#issuecomment-4257886508
# BE UT Coverage Report
Increment line coverage `0.00% (0/1)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/de340862224ea60ee1426d60c65e4cc3585fc933_de340862224ea60ee1426d60c65e4cc3585fc933/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/de340862224ea60ee1426d60c65e4cc3585fc933_de340862224ea60ee1426d60c65e4cc3585fc933/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 53.11% (20167/37973) |
| Line Coverage | 36.67% (189989/518056) |
| Region Coverage | 32.93% (147469/447862) |
| Branch Coverage | 34.06% (64575/189598) |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [cleanup](segment)Remove dead get_first_value/get_last_value APIs from PageBuilder [doris]
Posted by "eldenmoon (via GitHub)" <gi...@apache.org>.
eldenmoon closed pull request #62493: [cleanup](segment)Remove dead get_first_value/get_last_value APIs from PageBuilder
URL: https://github.com/apache/doris/pull/62493
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [improvement](segment) Speed up BinaryDictPageBuilder::add and remove dead first/last-value [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62493:
URL: https://github.com/apache/doris/pull/62493#issuecomment-4244328079
Thank you for your contribution to Apache Doris.
Don't know what should be done next? See [How to process your PR](https://cwiki.apache.org/confluence/display/DORIS/How+to+process+your+PR).
Please clearly describe your PR:
1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [cleanup](segment)Remove dead get_first_value/get_last_value APIs from PageBuilder [doris]
Posted by "csun5285 (via GitHub)" <gi...@apache.org>.
csun5285 commented on PR #62493:
URL: https://github.com/apache/doris/pull/62493#issuecomment-4257195172
/review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [cleanup](segment)Remove dead get_first_value/get_last_value APIs from PageBuilder [doris]
Posted by "eldenmoon (via GitHub)" <gi...@apache.org>.
eldenmoon merged PR #62493:
URL: https://github.com/apache/doris/pull/62493
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [cleanup](segment)Remove dead get_first_value/get_last_value APIs from PageBuilder [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #62493:
URL: https://github.com/apache/doris/pull/62493#issuecomment-4277481005
PR approved by at least one committer and no changes requested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [cleanup](segment)Remove dead get_first_value/get_last_value APIs from PageBuilder [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #62493:
URL: https://github.com/apache/doris/pull/62493#issuecomment-4277514136
PR approved by anyone and no changes requested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [cleanup](segment)Remove dead get_first_value/get_last_value APIs from PageBuilder [doris]
Posted by "csun5285 (via GitHub)" <gi...@apache.org>.
csun5285 commented on PR #62493:
URL: https://github.com/apache/doris/pull/62493#issuecomment-4257195507
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [cleanup](segment)Remove dead get_first_value/get_last_value APIs from PageBuilder [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62493:
URL: https://github.com/apache/doris/pull/62493#issuecomment-4257914927
# BE Regression && UT Coverage Report
Increment line coverage `100.00% (1/1)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62493_de340862224ea60ee1426d60c65e4cc3585fc933_merge/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62493_de340862224ea60ee1426d60c65e4cc3585fc933_merge/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 73.65% (27386/37182) |
| Line Coverage | 57.29% (295878/516438) |
| Region Coverage | 54.58% (246691/451972) |
| Branch Coverage | 56.20% (106876/190166) |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [improvement](segment) Speed up BinaryDictPageBuilder::add and remove dead first/last-value [doris]
Posted by "csun5285 (via GitHub)" <gi...@apache.org>.
csun5285 commented on PR #62493:
URL: https://github.com/apache/doris/pull/62493#issuecomment-4244328577
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org