add fields_metadata to SegmentReader, add columnar docs #2222
Conversation
PSeitz
force-pushed
the
list_fields
branch
2 times, most recently
from
6d9d17a to
35da586
Compare
src/core/segment_reader.rs
Outdated
| let mut out = Vec::new(); | ||
| for (_key, mut group) in &input | ||
| .into_iter() | ||
| .kmerge_by(|a, b| a < b) |
There was a problem hiding this comment.
left / right
| .kmerge_by(|a, b| a < b) | |
| .kmerge_by(|a, b| a < b) |
There was a problem hiding this comment.
Can you. remove the Ord implementation of FieldMetadata, create a function extracting the key you want (returning the pair field name field type ).
Put that in that file, and call it here instead?
There was a problem hiding this comment.
I prefer to have the Ord implementation, so it will behave consistent with sort_by etc.
src/core/segment_reader.rs
Outdated
| impl Ord for FieldMetadata { | ||
| fn cmp(&self, other: &Self) -> Ordering { | ||
| match self.field_name.cmp(&other.field_name) { | ||
| Ordering::Equal => self.typ.cmp(&other.typ), |
There was a problem hiding this comment.
Let's avoid inconsistent Ord / PartialOrd / Eq/ PartialEq / Hash implementation. This is a very common footgun.
(Here your Ord impl is inconsistent with Eq)
There was a problem hiding this comment.
I'll switch to a derive with a comment to not reorder
src/core/segment_reader.rs
Outdated
| let mut out = Vec::new(); | ||
| for (_key, mut group) in &input | ||
| .into_iter() | ||
| .kmerge_by(|a, b| a < b) |
There was a problem hiding this comment.
Can you. remove the Ord implementation of FieldMetadata, create a function extracting the key you want (returning the pair field name field type ).
Put that in that file, and call it here instead?
Co-authored-by: Paul Masurel <[email protected]>
src/core/segment_reader.rs
Outdated
| schema: &Schema, | ||
| ) -> Vec<FieldMetadata> { | ||
| // Maybe too slow for the high cardinality case | ||
| fn is_field_stored(field_name: &str, schema: &Schema) -> bool { |
There was a problem hiding this comment.
nitpick: Can we have the function outside of this?
I personally find it less readable.
I assume your point is that you want to namespace the function as much as possible.
One trouble with these is that it makes it non trivial to know whether they capture their environment or not.
So answering the question, "what does thjis function does" actually needs to look at its impl, as opposed to looking at the prototype of the function. It could hide any surprises.
There was a problem hiding this comment.
I think fn can't capture anything?
I moved it outside
* add fields_metadata to SegmentReader, add columnar docs * use schema to resolve field, add test * normalize paths * merge for FieldsMetadata, add fields_metadata on Index * Update src/core/segment_reader.rs Co-authored-by: Paul Masurel <[email protected]> * merge code paths * add Hash * move function oustide --------- Co-authored-by: Paul Masurel <[email protected]>
No description provided.