Skip to content

Commit 0e6dfa7

Browse files
skyelvesfacebook-github-bot
skyelves
authored and
facebook-github-bot
committed
fix: Fix min >= max caused by duplicate values in hashTable_ (facebookincubator#13375)
Summary: Pull Request resolved: facebookincubator#13375 fix: Fix min >= max caused by duplicate values in hashTable_ Update error logs Reviewed By: xiaoxmeng, kewang1024 Differential Revision: D74780463 fbshipit-source-id: 29d8ec9d437cf4b1f03afe3f8bc27209fbee14ad
1 parent ba5da13 commit 0e6dfa7

File tree

2 files changed

+127
-10
lines changed

2 files changed

+127
-10
lines changed

‎velox/type/Filter.cpp‎

Lines changed: 39 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -762,8 +762,17 @@ BigintValuesUsingBitmask::BigintValuesUsingBitmask(
762762
: Filter(true, nullAllowed, FilterKind::kBigintValuesUsingBitmask),
763763
min_(min),
764764
max_(max) {
765-
VELOX_CHECK(min < max, "min must be less than max");
766-
VELOX_CHECK(values.size() > 1, "values must contain at least 2 entries");
765+
VELOX_CHECK_LT(
766+
min,
767+
max,
768+
"BigintValuesUsingBitmask min must be less than max. min: {}, max: {}",
769+
min,
770+
max);
771+
VELOX_CHECK_GT(
772+
values.size(),
773+
1,
774+
"values must contain at least 2 entries, current size is {}",
775+
values.size());
767776

768777
bitmask_.resize(max - min + 1);
769778

@@ -814,8 +823,17 @@ BigintValuesUsingHashTable::BigintValuesUsingHashTable(
814823
max_(max),
815824
values_(values) {
816825
constexpr int32_t kPaddingElements = 4;
817-
VELOX_CHECK(min < max, "min must be less than max");
818-
VELOX_CHECK(values.size() > 1, "values must contain at least 2 entries");
826+
VELOX_CHECK_LT(
827+
min,
828+
max,
829+
"BigintValuesUsingHashTable min must be less than max. min: {}, max: {}",
830+
min,
831+
max);
832+
VELOX_CHECK_GT(
833+
values.size(),
834+
1,
835+
"values must contain at least 2 entries, current size is {}",
836+
values.size());
819837

820838
// Size the hash table to be 2+x the entry count, e.g. 10 entries
821839
// gets 1 << log2 of 50 == 32. The filter is expected to fail often so we
@@ -999,7 +1017,12 @@ HugeintValuesUsingHashTable::HugeintValuesUsingHashTable(
9991017
min_(min),
10001018
max_(max) {
10011019
VELOX_CHECK(!values.empty(), "values must not be empty");
1002-
VELOX_CHECK_LE(min_, max_, "min must not be greater than max");
1020+
VELOX_CHECK_LE(
1021+
min_,
1022+
max_,
1023+
"HugeintValuesUsingHashTable min must not be greater than max. min: {}, max: {}",
1024+
min_,
1025+
max_);
10031026
for (auto value : values) {
10041027
values_.insert(value);
10051028
}
@@ -1036,7 +1059,12 @@ NegatedBigintValuesUsingBitmask::NegatedBigintValuesUsingBitmask(
10361059
: Filter(true, nullAllowed, FilterKind::kNegatedBigintValuesUsingBitmask),
10371060
min_(min),
10381061
max_(max) {
1039-
VELOX_CHECK(min <= max, "min must be no greater than max");
1062+
VELOX_CHECK_LE(
1063+
min,
1064+
max,
1065+
"NegatedBigintValuesUsingBitmask min must be no greater than max. min: {}, max: {}",
1066+
min,
1067+
max);
10401068

10411069
nonNegated_ = std::make_unique<BigintValuesUsingBitmask>(
10421070
min, max, values, !nullAllowed);
@@ -1205,13 +1233,14 @@ BigintMultiRange::BigintMultiRange(
12051233
: Filter(true, nullAllowed, FilterKind::kBigintMultiRange),
12061234
ranges_(std::move(ranges)) {
12071235
VELOX_CHECK(!ranges_.empty(), "ranges is empty");
1208-
VELOX_CHECK(ranges_.size() > 1, "should contain at least 2 ranges");
1236+
VELOX_CHECK_GT(ranges_.size(), 1, "should contain at least 2 ranges.");
12091237
for (const auto& range : ranges_) {
12101238
lowerBounds_.push_back(range->lower());
12111239
}
12121240
for (int i = 1; i < lowerBounds_.size(); i++) {
1213-
VELOX_CHECK(
1214-
lowerBounds_[i] >= ranges_[i - 1]->upper(),
1241+
VELOX_CHECK_GE(
1242+
lowerBounds_[i],
1243+
ranges_[i - 1]->upper(),
12151244
"bigint ranges must not overlap");
12161245
}
12171246
}
@@ -2070,7 +2099,7 @@ std::unique_ptr<Filter> BigintValuesUsingHashTable::mergeWith(
20702099
valuesToKeep.emplace_back(kEmptyMarker);
20712100
}
20722101

2073-
for (int64_t v : hashTable_) {
2102+
for (int64_t v : values_) {
20742103
if (v != kEmptyMarker && other->testInt64(v)) {
20752104
valuesToKeep.emplace_back(v);
20762105
}

‎velox/type/tests/FilterTest.cpp‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1419,6 +1419,94 @@ TEST(FilterTest, mergeWithBigint) {
14191419
filters.push_back(in({1, 5, 210, empty}, true));
14201420
filters.push_back(in({empty - 10, empty, empty + 5}, false));
14211421
filters.push_back(in({empty - 10, empty, empty + 5}, true));
1422+
filters.push_back(
1423+
in({303624755802858,
1424+
384136357751697,
1425+
423368943828438,
1426+
476859918288764,
1427+
515956188024717,
1428+
530476219784376,
1429+
540277308863418,
1430+
541942388637759,
1431+
547188151816561,
1432+
594891360374231,
1433+
2959637400868637,
1434+
3721199441487026,
1435+
5114096828649969,
1436+
5740659369327042,
1437+
7458347650862984,
1438+
7772595162800109,
1439+
9047066205367949,
1440+
9499197740094903,
1441+
10164541576263747},
1442+
false));
1443+
filters.push_back(
1444+
in({366156343044031, 419600327914330, 422718254267138,
1445+
424415250736060, 432660186580905, 433507116523339,
1446+
435212426352282, 438781105992603, 462668840270469,
1447+
469721892897704, 475424508904228, 483961588133744,
1448+
489952690871193, 490100434182734, 490819974117919,
1449+
497522143079784, 498117470059295, 507069815819932,
1450+
510252055500200, 511649005360431, 528948513624967,
1451+
530007393190773, 532093806643372, 532858776337696,
1452+
533698686153076, 534941456359069, 534952966357439,
1453+
535328579401504, 536514619192305, 538518528865308,
1454+
539629822309977, 541635018758516, 542645398681605,
1455+
546538218258352, 549075251184136, 552034220783356,
1456+
555830763849991, 556128663698410, 557441673901702,
1457+
557518250566058, 558506873800117, 560841773258853,
1458+
564440396502105, 564619926218582, 565301696087188,
1459+
566576865977204, 567327966039757, 571300905632024,
1460+
571507642484409, 576283914994222, 583838294159962,
1461+
584905160854028, 585126200832284, 585144667234794,
1462+
585512340796065, 589020370310849, 590619753637829,
1463+
590968883472318, 602908642207125, 607722928272194,
1464+
630688649629213, 885942503719703, 901036228842690,
1465+
962070099101869, 969139801894639, 969995364959528,
1466+
973627137921970, 1002311311705971, 1038877281341469,
1467+
1065903171959919, 1087370529605788, 1098287305028396,
1468+
1101645231614814, 1102068331524993, 1105447177903198,
1469+
1133861121777932, 1276407910222050, 1287904945548777,
1470+
1288419322354242, 1291701008519687, 1316499312639548,
1471+
1673720443174607, 1718049419052929, 1764589184342287,
1472+
1956371744842865, 1957088988143983, 2294783974193603,
1473+
2310611259279723, 2489988147857778, 2506680886188504,
1474+
2778644458941619, 2864989937002641, 3330532020422880,
1475+
3379144482229182, 3531656983798849, 3684799191810397,
1476+
3767646070216820, 3780026845640994, 3866482590263275,
1477+
3908237629460878, 3929286414014934, 3997016003866875,
1478+
6662924023746038, 8514927431889941, 8612971988750230,
1479+
8636020789790871, 8719877121393549, 8725132074239741,
1480+
8780651958686524, 8787110144711385, 8787330658025567,
1481+
8819046344852395, 8924909114261419, 8975532075831271,
1482+
9259979694035612, 9315469178463099, 9337669522923600,
1483+
9568982303132896, 9577455068986597, 9581431555203631,
1484+
9829903477024362, 10109338878839298, 10111913146675745,
1485+
10116454049060631, 10124906929770020, 10134576737527714,
1486+
10159848794251706, 10160080836411106, 10160121877061863,
1487+
10160278547826680, 10160634373033302, 10160634378133302,
1488+
10160634384343302, 10160690892401937, 10160813905552695,
1489+
10160947889699506, 10160952984619779, 10161103697043337,
1490+
10161366446441677, 10161764887950446, 10161987568435865,
1491+
10161990333441343, 10162023187830630, 10162078292985953,
1492+
10162218597897184, 10162437068533464, 10162437274543464,
1493+
10162778721371988, 10162785900361133, 10162858926244783,
1494+
10163123124869467, 10163146005786988, 10163215994438487,
1495+
10163381914257985, 10164541576263747, 10164917662438438,
1496+
10169186680190153, 10169728530660634, 10170078224595191,
1497+
10222519309531166, 10222619264790035, 10226169282240298,
1498+
10226533527356871, 10226809478652237, 10228886958906475,
1499+
10229587253999927, 10230037972188280, 10230384967105126,
1500+
10230604232962437, 10230895412882189, 10231056553451466,
1501+
10231187746449346, 10231194618102996, 10231349384532060,
1502+
10231749348709149, 10231816240283162, 10232659299412293,
1503+
10232936398942338, 10233350393082617, 10233775873439095,
1504+
10233821539425403, 10234016712428289, 10234070019035259,
1505+
10234328283366746, 10234421657513549, 10234695927556443,
1506+
10236169460675790, 10236184353810192, 10236719696391339,
1507+
10236734971453206, 10236817060225653, 28113146188270673,
1508+
28349528491304705},
1509+
false));
14221510

14231511
// NOT IN-list.
14241512
filters.push_back(notIn({1, 2, 3, 67'000'000'000, 134}));

0 commit comments

Comments
 (0)