Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the handling of row ranges and keys in the Bigtable client, specifically improving how empty keys and inclusiveness flags are parsed and represented. It replaces truthiness checks on protobuf fields with explicit membership tests to prevent incorrect evaluation of empty byte keys. Additionally, add_row_range_with_prefix has been updated to support both string and byte prefixes, correctly handling UTF-8 boundaries and empty inputs. The review feedback correctly identifies an issue in read_rows_query.py where comparing end_key to an empty string fails to match empty byte keys, and suggests using 'not end_key' for a more robust and idiomatic check.
| # but preserve explicit empty-key bounds when inclusiveness is specified. | ||
| if not start_key and start_is_inclusive is not False: | ||
| start_key = None | ||
| if not end_key and end_is_inclusive is None: |
There was a problem hiding this comment.
why is this not end_is_inclusive is not False?
There was a problem hiding this comment.
Because b"" is the smallest possible key in lexicographical byte order, [b"", ...) (start_is_inclusive=True) is equivalent to an unbounded start (None), whereas (..., b""] (end_is_inclusive=True) matches only the empty key b"" (k <= b"") rather than all rows. Using end_is_inclusive is not False would turn RowRange(end_key=b"", end_is_inclusive=True) into end_key=None (unbounded), matching the entire table instead of just b"". Checking end_is_inclusive is None only normalizes "" / b"" when the caller omitted end_is_inclusive (such as RowRange("", "")), while preserving both explicit end_is_inclusive=True (end_key_closed=b"") and end_is_inclusive=False (end_key_open=b"").
| prefix_bytes = _to_bytes(row_key_prefix, encoding="utf-8") | ||
| if not prefix_bytes: | ||
| raise ValueError("row_key_prefix cannot be empty.") | ||
| stripped = prefix_bytes.rstrip(b"\xff") |
There was a problem hiding this comment.
why is \xff a special case?
There was a problem hiding this comment.
0xFF (255) is the maximum value of a single byte, so incrementing the last byte of a prefix ending in \xff would overflow range(0, 256) (ValueError: byte must be in range(0, 256)). To find the smallest byte string greater than all keys with that prefix, the increment must carry into the rightmost non-0xFF byte and drop the trailing 0xFF bytes (e.g., prefix b"abc\xff" has exclusive upper bound b"abd"). If every byte in the prefix is 0xFF (e.g., b"\xff\xff"), no larger byte prefix exists so end_key must be None (unbounded).
274d3dc to
8d23a80
Compare
8d23a80 to
6735ff7
Compare
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕