remove JsonTermWriter #2238
remove JsonTermWriter #2238
Conversation
PSeitz
force-pushed
the
path_collector
branch
from
143cd70 to
95343c1
Compare
PSeitz
force-pushed
the
path_collector
branch
from
95343c1 to
584d19f
Compare
| json_term_writer, | ||
| DateTime::from_utc(dt_utc), | ||
| )); | ||
| term.append_type_and_fast_value(DateTime::from_utc(dt_utc)); |
There was a problem hiding this comment.
I suspect this is a bug or a bug in waiting.
For this to work, we need term to not contain any value at this point.
PSeitz
force-pushed
the
path_collector
branch
4 times, most recently
from
2afa480 to
ce04b98
Compare
src/core/json_utils.rs
Outdated
| phrase: &str, | ||
| ) -> Option<Term> { | ||
| /// Tries to infer a JSON type from a string and append it to the term. | ||
| pub(crate) fn convert_to_fast_value_and_append(term: &Term, phrase: &str) -> Option<Term> { |
There was a problem hiding this comment.
the prototype and/or the impl is a bit silly / misleading.
We pass a reference but always start by cloning it... even though we sometime return None in the end.
There was a problem hiding this comment.
I cloned it, because it's not performance critical and simplifies the code
PSeitz
force-pushed
the
path_collector
branch
from
9a1b756 to
8b21d88
Compare
| /// Append a type marker + fast value to a term. | ||
| /// This is used in JSON type to append a fast value after the path. | ||
| /// | ||
| /// It will not clear existing bytes. | ||
| pub(crate) fn append_type_and_fast_value<T: FastValue>(&mut self, val: T) { | ||
| self.0.push(T::to_type().to_code()); |
There was a problem hiding this comment.
can we have debug assert here too?
There was a problem hiding this comment.
In indexing we don't have a path, but a path id, so the debug_assert doesn't work here. It's also atypical for the Term methods, they are just building blocks, and the caller has to know the Term state
There was a problem hiding this comment.
the comment made me think it was only for json. So it is not?
There was a problem hiding this comment.
It is, but there are two JSON currently
Indexing: [FieldId][PathId][Value]
Query: [FieldId][JsonPath][JsonPathEnd][Value]
When we switch to [PathId][Value] for all data during Indexing, we can split Term into Term and IndexingTerm with a clearer API.
PSeitz
force-pushed
the
path_collector
branch
3 times, most recently
from
982bc22 to
b5d07c1
Compare
remove JsonTermWriter remove path truncation logic, add assertion
* remove JsonTermWriter remove JsonTermWriter remove path truncation logic, add assertion * fix json_path_writer add sep logic
No description provided.