Conversation
📝 WalkthroughWalkthroughThe changes introduce a new public method Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (13)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| { | ||
| return Err(CollectionError::strict_mode( | ||
| format!("Index required but not found for \"{}\"", self.group_by,), | ||
| "Create an index for this key.", |
There was a problem hiding this comment.
not every index should work, we need to make sure that it is one which supports Match conditions, like integer, keyword, uuid or bool.
I wonder if we can reuse the unindexed field extractor logic for this.
There was a problem hiding this comment.
I was not able to reuse the unindexed field extractor because I don't have a filter to start with.
Instead I made something manually looking up the schema.
agourlay
force-pushed
the
strict-mode-group-by-unindexed
branch
from
f501a8d to
26b6b37
Compare
There was a problem hiding this comment.
I was not able to reuse the unindexed field extractor because I don't have a filter to start with.
Instead I made something manually looking up the schema.
One solution to reuse the unindexed field infra is to create a synthetic match filter to pass to it :)
| pub fn support_match(&self) -> bool { | ||
| match self { | ||
| Self::Keyword => true, | ||
| Self::Integer => true, | ||
| Self::Bool => true, | ||
| Self::Uuid => true, | ||
| Self::Float => false, | ||
| Self::Geo => false, | ||
| Self::Text => false, | ||
| Self::Datetime => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
integer and uuid can be parametrized without map index, which would make them not support match conditions
There was a problem hiding this comment.
I handled the integer the index here d6fbc6d
But I do not think the UUID lookup can be disabled.
I thought about it but decided against it because I felt that coming up with a synthetic match value was too hacky. EDIT: I can't do this trick, the |
agourlay
force-pushed
the
strict-mode-group-by-unindexed
branch
from
21421a6 to
5fe2ae0
Compare
agourlay
force-pushed
the
strict-mode-group-by-unindexed
branch
from
5fe2ae0 to
f5b3360
Compare
Then we should fix the unindexed extractor, regular filter checking for strict mode depends on this IIRC. |
I'd rather look at this in a different PR given that is is orthogonal to my current work. |
|
In a follow up PR I will fix the existing unindexed logic used by strict mode by leveraging the new match support detection. |
|
Here is the follow up #6496 |
* Fix strict mode unindexed group_by path * check index schema for matching support * Handle disabled lookup on integer index
2 participants
Fix strict mode for grouping APIs to make sure
unindexed_filtering_retrieveapplies to the field grouped on.