DRIVERS-3123 Require ignored bits in BSON binary vector PACKED_BITS to be zero.#1783
Conversation
…he given padding (those ignored) must be zero.
|
mongo-python-driver pull-request #2261 |
caseyclements
requested review from
JamesKovacs,
blink1073,
jyemin and
kevinAlbs
|
|
||
| - Padding MUST be 0 for all dtypes where padding doesn’t apply, and MUST be within \[0, 7\] for PACKED_BIT. | ||
| - A PACKED_BIT vector MUST NOT be empty if padding is in the range \[1, 7\]. | ||
| - For a PACKED_BIT vector, bits lower than the given padding (those ignored) must be zero. |
There was a problem hiding this comment.
For what it's worth, it could be helpful adding some specificity about bit order here since the description is correct only if bits are interpreted in the opposite order used for packed_bit encoding. (packed_bit is otherwise MSB first, but this is referring to the LSB as 0)
There was a problem hiding this comment.
Happy to. Would you please suggest a short alternative? Any longer description that you'd like to make should be done in a new PR.
There was a problem hiding this comment.
Suggest rewording to avoid "lower":
| - For a PACKED_BIT vector, bits lower than the given padding (those ignored) must be zero. | |
| - For a PACKED_BIT vector, ignored bits must be zero. |
I expect the phrase "The least-significant bits are ignored." above clarifies which bits are ignored.
ghost
mentioned this pull request
kevinAlbs
left a comment
kevinAlbs
left a comment
There was a problem hiding this comment.
Thank you for the change.
|
|
||
| - Padding MUST be 0 for all dtypes where padding doesn’t apply, and MUST be within \[0, 7\] for PACKED_BIT. | ||
| - A PACKED_BIT vector MUST NOT be empty if padding is in the range \[1, 7\]. | ||
| - For a PACKED_BIT vector, bits lower than the given padding (those ignored) must be zero. |
There was a problem hiding this comment.
Is this for Encoding and Decoding?
There was a problem hiding this comment.
Yes. That's what we came to, and exceptions are thrown in the Python implementation in both cases.
There was a problem hiding this comment.
Right, it's specified a few lines after:
Drivers MUST perform this validation when...
kevinAlbs
left a comment
kevinAlbs
left a comment
There was a problem hiding this comment.
LGTM with a test expectation fix and suggested rewording.
|
|
||
| - Padding MUST be 0 for all dtypes where padding doesn’t apply, and MUST be within \[0, 7\] for PACKED_BIT. | ||
| - A PACKED_BIT vector MUST NOT be empty if padding is in the range \[1, 7\]. | ||
| - For a PACKED_BIT vector, bits lower than the given padding (those ignored) must be zero. |
There was a problem hiding this comment.
Suggest rewording to avoid "lower":
| - For a PACKED_BIT vector, bits lower than the given padding (those ignored) must be zero. | |
| - For a PACKED_BIT vector, ignored bits must be zero. |
I expect the phrase "The least-significant bits are ignored." above clarifies which bits are ignored.
| "dtype_alias": "PACKED_BIT", | ||
| "padding": 0, | ||
| "canonical_bson": "1600000005766563746F7200040000000910007F0700" | ||
| "canonical_bson": "1600000005766563746F7200040000000910007F0800" |
There was a problem hiding this comment.
| "canonical_bson": "1600000005766563746F7200040000000910007F0800" | |
| "canonical_bson": "1600000005766563746F7200040000000910007F0700" |
5 participants
Please complete the following before merging:
clusters, and serverless).