-
Notifications
You must be signed in to change notification settings - Fork 853
Fix vector index missing in telemetrics #21788
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
base: devel
Are you sure you want to change the base?
Conversation
"edge", "geo", "hash", "fulltext", "inverted", | ||
"persistent", "skiplist", "ttl", "mdi", "mdi-prefixed", | ||
"iresearch", "primary", "unknown"}; | ||
"iresearch", "primary", "vector", "unknown"}; |
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.
For telemetry schema consistency, we need to always report the vector index here, regardless of whether it is used or not.
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.
LGTM, just some small comments.
return { | ||
setUpAll: function () { | ||
db._create(cn); | ||
let coll = db._createEdgeCollection(cn3, {numberOfShards: 2}); |
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.
Is there a reason why the edge collection has 2 shards and the document collection has one?
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.
Nope I forgot about it. I will remove the uneeded stuff from the setup
|
||
function validateObjectAgainstSchema(obj, fieldDefinitions, currentPath) { | ||
let allErrors = []; | ||
const dataKeys = new Set(Object.keys(obj)); |
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.
I don't see why we need this variable
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.
allErrors
reports all found violations agains schema and dataKeys
severs to check if some fields are duplicates
if (mode !== "NULLABLE" && mode !== "REPEATED") { | ||
// This case is for fields that are implicitly "REQUIRED". | ||
// Given the schema's explicit modes, this path usually means the field is optional. | ||
// errors.push(`Error at "${path}": Field is required but missing.`); |
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.
why is this commented out? if it stays like this, we don't need this case.
Scope & Purpose
Vector index
n_vector
was missing in telemetrics is now added.Checklist
Related Information
(Please reference tickets / specification / other PRs etc)