-
Notifications
You must be signed in to change notification settings - Fork 46
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
Implement schema conversion to returned docs and general improvements #69
Conversation
pub static PRIMARY_KEY: &str = "_id"; | ||
|
||
fn default_to_true() -> bool { | ||
true |
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'm curious what the purpose of this is.
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.
This is a rather unfortunate result of serde not allowing default values directly and instead requiring a factory.
This is basically doing "if it's not given default to true" as hacky as it is. :(
/// These values need to either be a fast field (ints) or TEXT. | ||
search_fields: Vec<String>, | ||
|
||
/// A set of fields to boost by a given factor. |
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.
Could you comment a bit more the boost factor? For example, what is the range?
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.
Could probably mirror the docs from Tantivy's boost query to be honest, considering this is just a multiplier.
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.
Ah, thanks for the clarification. Putting a link to tantiviy's doc page would be helpful.
fn validate(&self) -> Result<()> { | ||
if self.search_fields.is_empty() { | ||
return Err(Error::msg( | ||
"at least one indexed field must be given to search.", |
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.
Does it make sense to make search_fields optional and by default use all indexed fields for search?
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.
Thats a good idea! Shall add that.
let existing_fields_set: HashSet<&str> = | ||
HashSet::from_iter(existing_fields.into_iter()); | ||
|
||
let union: Vec<&str> = defined_fields_set |
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.
Shouldn't this variable be called diff
or something?
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.
it does indeed, left over from testing 😓 Has been changed in my local copy.
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.
Not sure if it's appropriate to write unsolicited review, please just take it as generic comments from someone who doesn't know much about search engines.
for (field, details) in self.fields.iter() { | ||
if field == PRIMARY_KEY { | ||
warn!( | ||
"{} is a reserved field name due to being a primary key", |
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.
Note that _id
is the primary key identifier for mongodb, too. This might cause pain if someone wants to build a bridge between mongo and lnx. Maybe make it a bit more explicit, something like _lnx_id
?
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 think it's quite reasonable to do that, although probably want to make that change in a separate PR as this one's already pretty big.
Now that this PR is pretty much done I'll do a brief summary of the changes as there are quite a few:
|
Closes #67
This adds the ability for a user to explicitly state in the schema if a field is multi-value or not.
If the field isn't multi-value the returned field will be unwrapped from it's array wrapper.