Skip to content

Commit d6238e5

Browse files
committed
emulator: filters: ColumnRange: Don't return cells from every column family.
- There is a passing unit test that tests for this (which is fixed and shows up in this diff). - The CI test (bigtable_filters_integration_test) which revealed this bug now passes.
1 parent 3735316 commit d6238e5

2 files changed

Lines changed: 34 additions & 9 deletions

File tree

google/cloud/bigtable/emulator/table.cc

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -395,26 +395,51 @@ StatusOr<CellStream> Table::CreateCellStream(
395395
}
396396

397397
bool FilteredTableStream::ApplyFilter(InternalFilter const& internal_filter) {
398-
if (!absl::holds_alternative<FamilyNameRegex>(internal_filter)) {
398+
if (!absl::holds_alternative<FamilyNameRegex>(internal_filter) &&
399+
!absl::holds_alternative<ColumnRange>(internal_filter)) {
399400
return MergeCellStreams::ApplyFilter(internal_filter);
400401
}
402+
// internal_filter is either FamilyNameRegex or ColumnRange
401403
for (auto stream_it = unfinished_streams_.begin();
402-
stream_it != unfinished_streams_.end(); ++stream_it) {
404+
stream_it != unfinished_streams_.end(); ) {
403405
auto const* cf_stream =
404406
dynamic_cast<FilteredColumnFamilyStream const*>(&(*stream_it)->impl());
405407
assert(cf_stream);
406-
if (!re2::RE2::PartialMatch(
407-
cf_stream->column_family_name(),
408-
*absl::get<FamilyNameRegex>(internal_filter).regex)) {
408+
409+
// We need to call ApplyFilter on the column family stream. But
410+
// ApplyFilter changes the data of the calling object so it cannot
411+
// be const.
412+
auto* cf_stream_mutable =
413+
const_cast<FilteredColumnFamilyStream*>(cf_stream);
414+
assert(cf_stream_mutable);
415+
416+
if ((absl::holds_alternative<FamilyNameRegex>(internal_filter) &&
417+
!re2::RE2::PartialMatch(
418+
cf_stream_mutable->column_family_name(),
419+
*absl::get<FamilyNameRegex>(internal_filter).regex)) ||
420+
(absl::holds_alternative<ColumnRange>(internal_filter) &&
421+
absl::get<ColumnRange>(internal_filter).column_family !=
422+
cf_stream_mutable->column_family_name())) {
409423
auto last_it = std::prev(unfinished_streams_.end());
410424
if (stream_it == last_it) {
411425
unfinished_streams_.pop_back();
412426
break;
413427
}
414-
stream_it->swap(unfinished_streams_.back());
415-
unfinished_streams_.pop_back();
428+
429+
stream_it = unfinished_streams_.erase(stream_it);
430+
continue;
431+
}
432+
433+
if (absl::holds_alternative<ColumnRange>(internal_filter) &&
434+
absl::get<ColumnRange>(internal_filter).column_family ==
435+
cf_stream_mutable->column_family_name()) {
436+
cf_stream_mutable->ApplyFilter(internal_filter);
416437
}
438+
439+
stream_it++;
440+
417441
}
442+
418443
return true;
419444
}
420445

google/cloud/bigtable/emulator/table_test.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,8 @@ TEST(FilteredTableStream, OnlyRightFamilyColumnsAreFiltered) {
9191
FilteredTableStream stream(std::move(fams));
9292

9393
stream.ApplyFilter(
94-
ColumnRange{"fam2", StringRangeSet::Range("a", false, "b", false)});
95-
EXPECT_EQ("row0 fam1:col0 @10ms: foo\n", DumpStream(stream));
94+
ColumnRange{"fam2", StringRangeSet::Range("col0", false, "col1", true)});
95+
EXPECT_EQ("row0 fam2:col0 @10ms: foo\n", DumpStream(stream));
9696
}
9797

9898
TEST(FilteredTableStream, OtherFiltersArePropagated) {

0 commit comments

Comments
 (0)