You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "mrhhsg (via GitHub)" <gi...@apache.org> on 2023/10/23 10:04:02 UTC
[PR] [fix](bitmap) incorrect result of operator == [doris]
mrhhsg opened a new pull request, #25788:
URL: https://github.com/apache/doris/pull/25788
## Proposed changes
If both the right and left Roaring64Map contain only one empty map, the operator ‘==’ should return true but currently returns false.
## Further comments
If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
--
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] [fix](bitmap) incorrect result of operator == [doris]
Posted by "mrhhsg (via GitHub)" <gi...@apache.org>.
mrhhsg commented on PR #25788:
URL: https://github.com/apache/doris/pull/25788#issuecomment-1776387223
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] [fix](bitmap) incorrect result of operator == [doris]
Posted by "mrhhsg (via GitHub)" <gi...@apache.org>.
mrhhsg commented on PR #25788:
URL: https://github.com/apache/doris/pull/25788#issuecomment-1776317693
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] [fix](bitmap) incorrect result of operator == [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25788:
URL: https://github.com/apache/doris/pull/25788#issuecomment-1774957743
TeamCity be ut coverage result:
Function Coverage: 37.04% (8300/22410)
Line Coverage: 29.17% (66638/228409)
Region Coverage: 27.76% (34615/124683)
Branch Coverage: 24.42% (17583/72012)
Coverage Report: http://coverage.selectdb-in.cc/coverage/f43b452ccdbe60d56b1f839798c4ea32a83833ea_f43b452ccdbe60d56b1f839798c4ea32a83833ea/report/index.html
--
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] [fix](bitmap) incorrect result of operator == [doris]
Posted by "mrhhsg (via GitHub)" <gi...@apache.org>.
mrhhsg closed pull request #25788: [fix](bitmap) incorrect result of operator ==
URL: https://github.com/apache/doris/pull/25788
--
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] [fix](bitmap) incorrect result of operator == [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25788:
URL: https://github.com/apache/doris/pull/25788#issuecomment-1776504574
(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 45.79 seconds
stream load tsv: 552 seconds loaded 74807831229 Bytes, about 129 MB/s
stream load json: 20 seconds loaded 2358488459 Bytes, about 112 MB/s
stream load orc: 65 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 32 seconds loaded 861443392 Bytes, about 25 MB/s
insert into select: 29.0 seconds inserted 10000000 Rows, about 344K ops/s
storage size: 17162008390 Bytes
--
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] [fix](bitmap) incorrect result of operator == [doris]
Posted by "mrhhsg (via GitHub)" <gi...@apache.org>.
mrhhsg commented on PR #25788:
URL: https://github.com/apache/doris/pull/25788#issuecomment-1774849413
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] [fix](bitmap) incorrect result of operator == [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25788:
URL: https://github.com/apache/doris/pull/25788#issuecomment-1780357538
(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 46.44 seconds
stream load tsv: 572 seconds loaded 74807831229 Bytes, about 124 MB/s
stream load json: 20 seconds loaded 2358488459 Bytes, about 112 MB/s
stream load orc: 65 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 32 seconds loaded 861443392 Bytes, about 25 MB/s
insert into select: 29.0 seconds inserted 10000000 Rows, about 344K ops/s
storage size: 17162299282 Bytes
--
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] [fix](bitmap) incorrect result of operator == [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #25788:
URL: https://github.com/apache/doris/pull/25788#discussion_r1368435166
##########
be/src/util/bitmap_value.h:
##########
@@ -519,49 +519,42 @@ class Roaring64Map {
// we cannot use operator == on the map because either side may contain
// empty Roaring Bitmaps
auto lhs_iter = roarings.cbegin();
+ auto lhs_cend = roarings.cend();
auto rhs_iter = r.roarings.cbegin();
- do {
- // if the left map has reached its end, ensure that the right map
- // contains only empty Bitmaps
- if (lhs_iter == roarings.cend()) {
- while (rhs_iter != r.roarings.cend()) {
- if (rhs_iter->second.isEmpty()) {
- ++rhs_iter;
- continue;
- }
- return false;
- }
- return true;
- }
- // if the left map has an empty bitmap, skip it
- if (lhs_iter->second.isEmpty()) {
+ auto rhs_cend = r.roarings.cend();
+ while (lhs_iter != lhs_cend && rhs_iter != rhs_cend) {
+ auto lhs_key = lhs_iter->first, rhs_key = rhs_iter->first;
Review Comment:
warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]
```suggestion
auto lhs_key = lhs_iter->first;
auto rhs_key = rhs_iter->first;
```
##########
be/test/util/bitmap_value_test.cpp:
##########
@@ -29,6 +29,29 @@
namespace doris {
using roaring::Roaring;
+TEST(BitmapValueTest, Roaring64Map_operator_eq) {
+ detail::Roaring64Map roaring64_map1, roaring64_map2;
+ EXPECT_TRUE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map2.add(uint32_t(100));
+ EXPECT_FALSE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map2.remove(uint32_t(100));
+ EXPECT_TRUE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map1.add(uint32_t(100));
Review Comment:
warning: 100 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
```cpp
roaring64_map1.add(uint32_t(100));
^
```
##########
be/src/util/bitmap_value.h:
##########
@@ -519,49 +519,42 @@ class Roaring64Map {
// we cannot use operator == on the map because either side may contain
// empty Roaring Bitmaps
auto lhs_iter = roarings.cbegin();
+ auto lhs_cend = roarings.cend();
auto rhs_iter = r.roarings.cbegin();
- do {
- // if the left map has reached its end, ensure that the right map
- // contains only empty Bitmaps
- if (lhs_iter == roarings.cend()) {
- while (rhs_iter != r.roarings.cend()) {
- if (rhs_iter->second.isEmpty()) {
- ++rhs_iter;
- continue;
- }
- return false;
- }
- return true;
- }
- // if the left map has an empty bitmap, skip it
- if (lhs_iter->second.isEmpty()) {
+ auto rhs_cend = r.roarings.cend();
+ while (lhs_iter != lhs_cend && rhs_iter != rhs_cend) {
+ auto lhs_key = lhs_iter->first, rhs_key = rhs_iter->first;
+ const auto &lhs_map = lhs_iter->second, &rhs_map = rhs_iter->second;
Review Comment:
warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]
```suggestion
const auto &lhs_map = lhs_iter->second;
const auto &rhs_map = rhs_iter->second;
```
##########
be/test/util/bitmap_value_test.cpp:
##########
@@ -29,6 +29,29 @@
namespace doris {
using roaring::Roaring;
+TEST(BitmapValueTest, Roaring64Map_operator_eq) {
Review Comment:
warning: all parameters should be named in a function [readability-named-parameter]
```suggestion
TEST(BitmapValueTest /*unused*/, Roaring64Map_operator_eq /*unused*/) {
```
##########
be/test/util/bitmap_value_test.cpp:
##########
@@ -29,6 +29,29 @@
namespace doris {
using roaring::Roaring;
+TEST(BitmapValueTest, Roaring64Map_operator_eq) {
+ detail::Roaring64Map roaring64_map1, roaring64_map2;
+ EXPECT_TRUE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map2.add(uint32_t(100));
+ EXPECT_FALSE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map2.remove(uint32_t(100));
+ EXPECT_TRUE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map1.add(uint32_t(100));
+ EXPECT_FALSE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map2.add(uint32_t(100));
+ EXPECT_TRUE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map2.remove(uint32_t(100));
+ EXPECT_FALSE(roaring64_map1 == roaring64_map2);
+
+ roaring64_map1.remove(uint32_t(100));
Review Comment:
warning: 100 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
```cpp
roaring64_map1.remove(uint32_t(100));
^
```
--
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] [fix](bitmap) incorrect result of operator == [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #25788:
URL: https://github.com/apache/doris/pull/25788#issuecomment-1776399418
TeamCity be ut coverage result:
Function Coverage: 37.02% (8296/22410)
Line Coverage: 29.17% (66623/228420)
Region Coverage: 27.75% (34604/124681)
Branch Coverage: 24.41% (17574/72008)
Coverage Report: http://coverage.selectdb-in.cc/coverage/8210e767b6f9c119d22aba6b512479138fae903c_8210e767b6f9c119d22aba6b512479138fae903c/report/index.html
--
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] [fix](bitmap) incorrect result of operator == [doris]
Posted by "yiguolei (via GitHub)" <gi...@apache.org>.
yiguolei commented on PR #25788:
URL: https://github.com/apache/doris/pull/25788#issuecomment-1780284531
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