fix(bigtable): resolve O(n^2) bottleneck in ReadRowsQuery.add_range

[parthea]

This PR resolves a severe client-side O(n^2) CPU performance bottleneck inside ReadRowsQuery.add_range. This regression was introduced when deduplication checks were added to prevent duplicate row ranges in read requests.

Fixes b/573522284
Fixes #18708

[gemini-code-assist]

Code Review

This pull request optimizes the performance of ReadRowsQuery.add_range from O(N) to O(1) by making RowRange hashable and maintaining a _range_signatures set. It also fixes a typo from slots to __slots__ to ensure proper memory optimization, and adds corresponding unit tests. However, the proposed self-healing logic in add_range compares the lengths of the _range_signatures set and the _row_set.row_ranges list. If the query is initialized with duplicate ranges, these lengths will never match, causing the cache to rebuild on every call and introducing an O(N^2) bottleneck. It is recommended to remove this self-healing check entirely.

[ohmayr]

Wall-clock assertions tend to flake under coverage and when they fail they don't say why. Could we assert the behaviour instead? For example, check that add_range never builds row_ranges:

def test_add_range_does_not_materialize_row_ranges(self):
    from unittest import mock
    from google.cloud.bigtable.data.read_rows_query import RowRange
    query = self._make_one()
    with mock.patch.object(
        type(query), "row_ranges", new_callable=mock.PropertyMock
    ) as row_ranges:
        for i in range(100):
            query.add_range(RowRange(f"k{i:03d}", f"k{i + 1:03d}"))
    row_ranges.assert_not_called()
    assert len(query._row_set.row_ranges) == 100

[parthea]

fixed in d08cc34

[ohmayr]

Thanks for picking this up. Using a set is the right fix. One blocking issue (inline): __hash__ and __eq__ disagree for the legacy row_set.RowRange, so mixed legacy/base ranges no longer dedup. The rest are small.

Separately, add_key and ReadRowsQuery.__eq__ have the same O(n²) pattern: 10k add_key calls take ~11.6s, and __eq__ on two 2k-range queries takes ~50s. These can go in a follow-up PR, but please don't close b/573522284 until they're tracked, since key-heavy workloads are still slow.

[ohmayr]

__eq__ compares _pb, but __hash__ hashes self._key(). The legacy subclass google.cloud.bigtable.row_set.RowRange overrides _key() with a different field order, so legacy and base ranges compare equal but hash differently. That breaks set membership for mixed types, and dedup regresses vs main:

from google.cloud.bigtable.row_set import RowRange as LegacyRowRange

b, l = RowRange(b"a", b"b"), LegacyRowRange(b"a", b"b")
b == l               # True
hash(b) == hash(l)   # False

q = ReadRowsQuery(row_ranges=[b])
q.add_range(l)
len(q.row_ranges)    # 2 on this branch, 1 on main

Suggest hashing the same data __eq__ compares:

def __hash__(self) -> int:
    pb = self._pb
    return hash(
        (pb.start_key_closed, pb.start_key_open, pb.end_key_closed, pb.end_key_open)
    )

and dropping __hash__ from row_set.RowRange so it inherits this one (_key() then isn't needed here). I tried this locally: mixed legacy/base dedup works, the bigtable unit tests pass, and 10k add_range calls still take ~0.23s. Could you add a test along these lines?

def test_row_range_hash_matches_legacy(self):
    from google.cloud.bigtable.data.read_rows_query import RowRange
    from google.cloud.bigtable.row_set import RowRange as LegacyRowRange

    b, l = RowRange(b"a", b"b"), LegacyRowRange(b"a", b"b")
    assert b == l
    assert hash(b) == hash(l)
    query = self._make_one(row_ranges=[b])
    query.add_range(l)
    assert len(query.row_ranges) == 1

[ohmayr]

row_ranges is already a list of RowRange here, so set(row_ranges) should be enough and avoids re-wrapping from the proto. Naming nit: this holds RowRange objects, not signatures. Maybe _row_range_set?

Heads-up: ReadRowsQuery declares slots = (...) (not __slots__), which is the only reason adding a new attribute works. If that typo gets fixed later, this attribute needs to be added there.

[ohmayr]

Thanks for adding this. One gap: the new set seeding in __init__ isn't covered. Could we add a case where a range is passed to the constructor and an equal one is then added?

query = self._make_one(row_ranges=[RowRange("a", "b")])
query.add_range(RowRange("a", "b"))
assert len(query.row_ranges) == 1

[ohmayr]

nit: I'd drop this comment. The code already says what it does.

[ohmayr]

nit: cls.__new__(cls) is a good change, but the docstring now describes implementation. I'd keep the original wording and add a # comment on why __init__ is skipped if needed.