Skip to content

DRIVERS-3123 Require ignored bits in BSON binary vector PACKED_BITS to be zero. #1783

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
caseyclements merged 7 commits into from
Apr 9, 2025
Merged

DRIVERS-3123 Require ignored bits in BSON binary vector PACKED_BITS to be zero. #1783

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c13784b
Adds test that exception is thrown if bits ignored by padding are non…
caseyclements Apr 3, 2025
2d9565d
Add line to Validation section: For a PACKED_BIT vector, bits lower t…
caseyclements Apr 3, 2025
f270a25
Grammatical fix.
caseyclements Apr 4, 2025
d7ccb03
Updated changelog and FAQ
caseyclements Apr 8, 2025
ea265ea
Reverted change to Simple Vector PACKED_BIT test case
caseyclements Apr 8, 2025
87f75e1
canonical_bson sync for Simple Vector PACKED_BIT test case
caseyclements Apr 8, 2025
9461d62
Updated wording according to pull-request feedback
caseyclements Apr 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions source/bson-binary-vector/bson-binary-vector.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ Drivers MUST validate vector metadata and raise an error if any invariant is vio

- 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.
Copy link

@ghost ghost Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@caseyclements caseyclements Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@kevinAlbs kevinAlbs Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest rewording to avoid "lower":

Suggested change
- 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.

Copy link
Contributor Author

@caseyclements caseyclements Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kevinAlbs. Updated.

Copy link
Contributor

@BorisDog BorisDog Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this for Encoding and Decoding?

Copy link
Contributor Author

@caseyclements caseyclements Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. That's what we came to, and exceptions are thrown in the Python implementation in both cases.

Copy link
Contributor

@BorisDog BorisDog Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it's specified a few lines after:
Drivers MUST perform this validation when...

- When unpacking binary data into a FLOAT32 Vector structure, the length of the binary data following the dtype and
padding MUST be a multiple of 4 bytes.

Expand Down
25 changes: 17 additions & 8 deletions source/bson-binary-vector/tests/packed_bit.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,30 +14,39 @@
{
"description": "Simple Vector PACKED_BIT",
"valid": true,
"vector": [127, 7],
"vector": [127, 8],
"dtype_hex": "0x10",
"dtype_alias": "PACKED_BIT",
"padding": 0,
"canonical_bson": "1600000005766563746F7200040000000910007F0700"
"canonical_bson": "1600000005766563746F7200040000000910007F0800"
Copy link
Contributor

@kevinAlbs kevinAlbs Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"canonical_bson": "1600000005766563746F7200040000000910007F0800"
"canonical_bson": "1600000005766563746F7200040000000910007F0700"

},
{
"description": "Empty Vector PACKED_BIT",
"description": "PACKED_BIT with padding",
"valid": true,
"vector": [],
"vector": [127, 8],
"dtype_hex": "0x10",
"dtype_alias": "PACKED_BIT",
"padding": 0,
"canonical_bson": "1400000005766563746F72000200000009100000"
"padding": 3,
"canonical_bson": "1600000005766563746F7200040000000910037F0800"
},
{
"description": "PACKED_BIT with padding",
"valid": true,
"description": "PACKED_BIT with inconsistent padding",
"valid": false,
"vector": [127, 7],
"dtype_hex": "0x10",
"dtype_alias": "PACKED_BIT",
"padding": 3,
"canonical_bson": "1600000005766563746F7200040000000910037F0700"
},
{
"description": "Empty Vector PACKED_BIT",
"valid": true,
"vector": [],
"dtype_hex": "0x10",
"dtype_alias": "PACKED_BIT",
"padding": 0,
"canonical_bson": "1400000005766563746F72000200000009100000"
},
{
"description": "Overflow Vector PACKED_BIT",
"valid": false,
Expand Down
Loading