Skip to content

DRIVERS-3097 Update test cases for BSON Binary Vector #1750

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
qingyang-hu merged 6 commits into from
Feb 7, 2025
Merged

DRIVERS-3097 Update test cases for BSON Binary Vector #1750

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ef3219e
update test cases
qingyang-hu Feb 3, 2025
0da88e0
clarify testing invalid cases
qingyang-hu Feb 4, 2025
d542c2e
formatting
qingyang-hu Feb 4, 2025
38bea9e
formatting
qingyang-hu Feb 4, 2025
cb988a4
clarify the specs
qingyang-hu Feb 5, 2025
394111e
formatting
qingyang-hu Feb 5, 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
2 changes: 1 addition & 1 deletion source/bson-binary-vector/tests/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ Each JSON file contains three top-level keys.

- `description`: string describing the test.
- `valid`: boolean indicating if the vector, dtype, and padding should be considered a valid input.
- `vector`: list of numbers
- `vector`: (required if valid is true) list of numbers

@nbbeeken nbbeeken Feb 4, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's a section on validation in the specs that we should update too:

https://github.com/qingyang-hu/specifications/blob/drivers2926/source/bson-binary-vector/bson-binary-vector.md#validation

@vbabanin vbabanin Feb 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When the vector field is absent in an invalid case, is canonical_bson now a required field?

- `dtype_hex`: string defining the data type in hex (e.g. "0x10", "0x27")
- `dtype_alias`: (optional) string defining the data dtype, perhaps as Enum.
- `padding`: (optional) integer for byte padding. Defaults to 0.
Expand Down
11 changes: 9 additions & 2 deletions source/bson-binary-vector/tests/float32.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,15 @@
"vector": [127.0, 7.0],
"dtype_hex": "0x27",
"dtype_alias": "FLOAT32",
"padding": 3
"padding": 3,
"canonical_bson": "1C00000005766563746F72000A0000000927030000FE420000E04000"
},
{
"description": "Insufficient vector data FLOAT32",

@vbabanin vbabanin Feb 4, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks like this test introduces a use case where drivers should validate for the absence of the array, as when a null reference is passed.

If that's the case, we should extend this test to cover INT8 and PackedBit, as well as other relevant fields which could be absent.I also think, explicitly defining "vector": null or "dtype": null could make the test case more explicit.

@qingyang-hu qingyang-hu Feb 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The "canonical_bson" in "Insufficient vector data FLOAT32" is a corrupted BSON with insufficient bytes for 4-byte float32, so it lacks a corresponding float32 vector. It makes sense to use "vector": null to make the case more explicit and consistent with other cases.

@vbabanin vbabanin Feb 4, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for the clarification! At first glance, I thought the test was validating the absence of the entire vector array.

There is a section outlining what drivers should do in both valid and invalid cases. In valid cases, canonical_bson must be present since drivers are expected to encode and decode it to verify correctness.

Now that we have canonical_bson in invalid cases, it's unclear whether we should rely only on numerical values or also consider canonical_bson. The spec states:

To prove correctness in an invalid case (valid: false), one MUST:
Raise an exception when attempting to encode a document from the numeric values, dtype, and padding.

Should we add another line for clarity? For example:

When the vector field is absent, drivers MUST decode canonical_bson, as this contains corrupted vector data that can't be represented via the numerical vector field.

"valid": false,
"dtype_hex": "0x27",
"dtype_alias": "FLOAT32",
"canonical_bson": "1700000005766563746F7200050000000927002A2A2A00"
Comment thread
nbbeeken marked this conversation as resolved.
}
]
}

4 changes: 2 additions & 2 deletions source/bson-binary-vector/tests/int8.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@
"vector": [127, 7],
"dtype_hex": "0x03",
"dtype_alias": "INT8",
"padding": 3
"padding": 3,
"canonical_bson": "1600000005766563746F7200040000000903037F0700"
},
{
"description": "INT8 with float inputs",
Expand All @@ -54,4 +55,3 @@
}
]
}

23 changes: 4 additions & 19 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 @@ -8,7 +8,8 @@
"vector": [],
"dtype_hex": "0x10",
"dtype_alias": "PACKED_BIT",
"padding": 1
"padding": 1,
"canonical_bson": "1400000005766563746F72000200000009100100"
},
{
"description": "Simple Vector PACKED_BIT",
Expand Down Expand Up @@ -61,21 +62,14 @@
"dtype_alias": "PACKED_BIT",
"padding": 0
},
{
"description": "Padding specified with no vector data PACKED_BIT",
"valid": false,
"vector": [],
"dtype_hex": "0x10",
"dtype_alias": "PACKED_BIT",
"padding": 1
},

@caseyclements caseyclements Feb 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What was wrong with this test? An empty array but a non-zero padding?

@qingyang-hu qingyang-hu Feb 6, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just a duplicate of L5-L12

@caseyclements caseyclements Feb 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice spot! Thank you!

{
"description": "Exceeding maximum padding PACKED_BIT",
"valid": false,
"vector": [1],
"dtype_hex": "0x10",
"dtype_alias": "PACKED_BIT",
"padding": 8
"padding": 8,
"canonical_bson": "1500000005766563746F7200030000000910080100"
},
{
"description": "Negative padding PACKED_BIT",
Expand All @@ -84,15 +78,6 @@
"dtype_hex": "0x10",
"dtype_alias": "PACKED_BIT",
"padding": -1
},
{
"description": "Vector with float values PACKED_BIT",
"valid": false,
"vector": [127.5],
"dtype_hex": "0x10",
"dtype_alias": "PACKED_BIT",
"padding": 0
}
]
}

Comment thread
caseyclements marked this conversation as resolved.