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.
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
Improve tests for Annoy index #51268
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This is an automated comment for commit be0e18f with description of existing statuses. It's updated for the latest CI running
|
This comment was marked as outdated.
This comment was marked as outdated.
@@ -53,30 +55,34 @@ INSERT INTO tab VALUES (1, [0.0, 0.0, 10.0]), (2, [0.0, 0.0, 10.5]), (3, [0.0, 0 | |||
|
|||
-- See (*) why commented out | |||
-- SELECT 'WHERE type, L2Distance'; | |||
-- SELECT * | |||
-- WITH [0.0, 0.0, 10.0] AS reference_point |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What the point in comment which just duplicates the subsequent query? There is a number of such comments
/// Seed random generator used for building the trees in Annoy. All other parameters being equal (granularity trees, search_k), this | ||
/// theoretically makes search results deterministic as well. In practice, it is hard to achieve that on SQL level because the index | ||
/// structure is also affected by implicit things like the row order or how the column data is split into parts. | ||
index->set_seed(42); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(1) I didn't see that any tests were added which check stable results when using Annoy index? If adding seed still doesn't make it possible - why the do it at all?
(2) If setting seed still make sense then it'd be preferable to add a setting for it
No further need for this PR. |
Addresses some suggestions in #50661
Changelog category (leave one):