Move split_full_path to Schema
#1692
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1692 +/- ##
==========================================
+ Coverage 93.96% 94.05% +0.09%
==========================================
Files 256 256
Lines 48986 49291 +305
==========================================
+ Hits 46031 46363 +332
+ Misses 2955 2928 -27
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
|
||
| /// Searches for a full_path, returning the field name and a json path. | ||
| pub fn find_field<'a>(&self, full_path: &'a str) -> Option<(Field, &'a str)> { | ||
| if let Some(field) = self.0.fields_map.get(full_path) { |
There was a problem hiding this comment.
No unit tests for find_field!? Can we add some?
There was a problem hiding this comment.
I mostly copied the same unit test on QueryParser. I can remove the test from QueryParser or add more tests to Schema if that's what we want.
There was a problem hiding this comment.
yes please that would be awesome!
| /// If it's not, it splits the full_path at non-escaped '.' chars and tries to match the | ||
| /// prefix with the field names, favoring the longest field names. | ||
| /// | ||
| /// This does not check if field is a JSON field. It is possible for this functions to |
There was a problem hiding this comment.
Would it make sense to improve the spec and avoid this?
There was a problem hiding this comment.
It might be. I changed the function to continue when the prefix is not a JSON field, and all of the tests passed (probably does not mean it is completely ok to change), but I think it might be better to open a separate PR for it.
There was a problem hiding this comment.
Can you open a ticket?
Having this method on the
Schemalevel will help simplify the code on Quickwit for the issue quickwit-oss/quickwit#2260.Also, this comes with a bonus of removing the
field_nameshashmap fromQueryParser.