Repository navigation
fix(bigtable): encode str qualifier in DeleteRangeFromColumn and allow -2**63 in int mutations - #18881
fix(bigtable): encode str qualifier in DeleteRangeFromColumn and allow -2**63 in int mutations#18881mutianf wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates integer boundary checks across mutations and read-modify-write rules to correctly validate 64-bit signed integers using both minimum and maximum bounds, and adds support for string qualifiers in DeleteRangeFromColumn. The review feedback correctly points out that several error messages incorrectly state the upper limit as 263 instead of the exclusive 263 - 1, and suggests updating these messages to prevent user confusion.
| if not (_MIN_INCREMENT_VALUE <= new_value <= _MAX_INCREMENT_VALUE): | ||
| raise ValueError( | ||
| "int values must be between -2**63 and 2**63 (64-bit signed int)" | ||
| ) |
There was a problem hiding this comment.
The error message states that integer values must be between -2**63 and 2**63. However, the upper bound 2**63 is exclusive (the maximum valid 64-bit signed integer is 2**63 - 1). To avoid confusion for users hitting the upper boundary, the error message should be updated to specify 2**63 - 1 as the upper limit.
| if not (_MIN_INCREMENT_VALUE <= new_value <= _MAX_INCREMENT_VALUE): | |
| raise ValueError( | |
| "int values must be between -2**63 and 2**63 (64-bit signed int)" | |
| ) | |
| if not (_MIN_INCREMENT_VALUE <= new_value <= _MAX_INCREMENT_VALUE): | |
| raise ValueError( | |
| "int values must be between -2**63 and 2**63 - 1 (64-bit signed int)" | |
| ) |
| if not (_MIN_INCREMENT_VALUE <= value <= _MAX_INCREMENT_VALUE): | ||
| raise ValueError( | ||
| "int values must be between -2**63 and 2**63 (64-bit signed int)" | ||
| ) |
There was a problem hiding this comment.
The error message states that integer values must be between -2**63 and 2**63. However, the upper bound 2**63 is exclusive (the maximum valid 64-bit signed integer is 2**63 - 1). To avoid confusion for users hitting the upper boundary, the error message should be updated to specify 2**63 - 1 as the upper limit.
| if not (_MIN_INCREMENT_VALUE <= value <= _MAX_INCREMENT_VALUE): | |
| raise ValueError( | |
| "int values must be between -2**63 and 2**63 (64-bit signed int)" | |
| ) | |
| if not (_MIN_INCREMENT_VALUE <= value <= _MAX_INCREMENT_VALUE): | |
| raise ValueError( | |
| "int values must be between -2**63 and 2**63 - 1 (64-bit signed int)" | |
| ) |
| if not (_MIN_INCREMENT_VALUE <= increment_amount <= _MAX_INCREMENT_VALUE): | ||
| raise ValueError( | ||
| "increment_amount must be between -2**63 and 2**63 (64-bit signed int)" | ||
| ) |
There was a problem hiding this comment.
The error message states that the increment amount must be between -2**63 and 2**63. However, the upper bound 2**63 is exclusive (the maximum valid 64-bit signed integer is 2**63 - 1). To avoid confusion for users hitting the upper boundary, the error message should be updated to specify 2**63 - 1 as the upper limit.
| if not (_MIN_INCREMENT_VALUE <= increment_amount <= _MAX_INCREMENT_VALUE): | |
| raise ValueError( | |
| "increment_amount must be between -2**63 and 2**63 (64-bit signed int)" | |
| ) | |
| if not (_MIN_INCREMENT_VALUE <= increment_amount <= _MAX_INCREMENT_VALUE): | |
| raise ValueError( | |
| "increment_amount must be between -2**63 and 2**63 - 1 (64-bit signed int)" | |
| ) |
…w -2**63 in int mutations
…lumns, and fix wrapper algebra
e251888 to
a8364ea
Compare
Summary
DeleteRangeFromColumnqualifier validation and encoding:DeleteRangeFromColumndid not encodestrqualifiers tobytesin__post_init__, causingproto-plusto treat string qualifiers as base64 during_to_pb()serialization and either silently decode into the wrong column bytes or fail withbinascii.Error.__post_init__now encodesstrqualifiers to UTF-8bytesand raisesTypeErrorfor non-bytes/strinputs, matchingSetCellandAddToCell.-2**63(INT64_MIN) in integer mutations and increment rules:SetCell,AddToCell, andIncrementRulecheckedabs(val) > (1 << 63) - 1, which rejected the valid two's-complement minimum 64-bit signed integer-2**63(-9223372036854775808) withValueError. Replaced theabs()check with_MIN_INCREMENT_VALUE <= val <= _MAX_INCREMENT_VALUE.Test plan
tests/unit/data/test_mutations.pyandtests/unit/data/test_read_modify_write_rules.pycovering-2**63and2**63 - 1bounds andDeleteRangeFromColumnstr/ invalid qualifier handling.