-
Notifications
You must be signed in to change notification settings - Fork 13
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
[VO-874] feat: Improve index naming #1495
Conversation
424115d
to
700db23
Compare
0af08c5
to
80b3052
Compare
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.
That's a really great and long-expected work, thank you @cballevre!
My main concern is about the explicit operators, I feel like at least one of your test is not working at it should on couchdb. We might need to push tests a bit further, directly on couchdb
$or: ['konnector', 'worker'] | ||
} | ||
} | ||
expect(makeOperatorsExplicit(query)).toEqual(expected) |
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.
Have you tested this one on couchdb? I don't think that would work like this, I would expect this from couchdb:
"$or": [
{
"type": {
"$eq": "konnector"
}
},
{
"type": {
"$eq": "worker"
}
}
]
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.
Well found it was a failure. I've corrected the function to take this case into account 🙂
80b3052
to
f90dbe2
Compare
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.
Everything looks good, but it's probably worth to make some real partial filter testing on couchdb, to ensure we didn't miss anything like the one with the $or
Related to #1374 |
3fee032
to
1d0a959
Compare
The partialFilter values are static. As this does not change, we decided to integrate them into the index naming. Before this change, the index was not recalculated when a value changed. So the query always returned the same result.
When CouchDB creates an index with a partialFilter, it adds explicit operators when they are implicit, typically the $and and $eq operators. This causes a mismatch when comparing the partialFilter from the request definition with the partialFilter from the index. To address this, we added a step to make the operators in the partialFilter from the request definition explicit. This makes it possible to migrate indexes after they have been renamed rather than re-creating them, which has a non-negligible cost.
1d0a959
to
9aea8f4
Compare
So much cleaner than what I did before 👏 👏
In fact you're just going to update the name of the index, it should be ok. |
return `${indexName}_filter_(${makeKeyFromPartialFilter(partialFilter)})` | ||
} | ||
|
||
return indexName |
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.
You didn't change the method for the Pouch Link. Is it wanted? Is it out of scope ?
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.
Since @Ldoppea is currently heavily working on pouch link and notably on this part, we didn't feel the need to put it in this PR scope. But this is indeed identified 😉
It also raises a question about the need the mutualize such parts between stack-client and pouch-link... Maybe a new CC package?
Since we migrate the existing index through a copy, we should avoid a too much consequent load (i.e. we don't recompute index) |
Yep, I updated my comment just before yours ;). Just a last thing: I know that we had conflicts between the sack and cozy-client about index name. A few users had blank page because of this. I think that stack use partialFilters on some place, I'll double check that ;). |
If you find any info about this, that would be precious! |
In #1495 we improved index naming for CozyStackClient This commit applies the same algorithm to CozyPouchLink
In #1495 we improved index naming for CozyStackClient This commit applies the same algorithm to CozyPouchLink
In #1495 we improved index naming for CozyStackClient This commit applies the same algorithm to CozyPouchLink
In #1495 we improved index naming for CozyStackClient This commit applies the same algorithm to CozyPouchLink
In #1495 we improved index naming for CozyStackClient This commit applies the same algorithm to CozyPouchLink
In #1495 we improved index naming for CozyStackClient This commit applies the same algorithm to CozyPouchLink
In #1495 we improved index naming for CozyStackClient This commit applies the same algorithm to CozyPouchLink
In #1495 we improved index naming for CozyStackClient This commit applies the same algorithm to CozyPouchLink
In #1495 we improved index naming for CozyStackClient This commit applies the same algorithm to CozyPouchLink
In #1495 we improved index naming for CozyStackClient This commit applies the same algorithm to CozyPouchLink
In #1495 we improved index naming for CozyStackClient This commit applies the same algorithm to CozyPouchLink
In #1495 we improved index naming for CozyStackClient This commit applies the same algorithm to CozyPouchLink
In #1495 we improved index naming for CozyStackClient This commit applies the same algorithm to CozyPouchLink
Index naming has two weaknesses:
This can cause differences between the expected result and the result when modifying queries.
An example of the new index name:
Query definition :
Old index name :
_design/by_type_and_name_filter_class_and_trashed
New index name :
_design/by_type_and_name_filter_(class_(image_$or_pdf))_and_(trashed_$eq_false)