make inspect-table work again #190
Conversation
nicklan
commented
May 3, 2024
•
nicklan
commented
- redo inspect-table with new model
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #190 +/- ##
==========================================
+ Coverage 77.66% 77.68% +0.02%
==========================================
Files 49 49
Lines 10079 10089 +10
Branches 10079 10089 +10
==========================================
+ Hits 7828 7838 +10
Misses 1805 1805
Partials 446 446 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
lgtm only thing that feels worth double-checking is the pub actions/visitors for developer-visibility? seems fine since we basically say everything is 'unstable' in those APIs anyways and we could revoke it in the future (just saying it's a two-way door). also I think for write path I'll need to expand this to include commitInfo action when that goes in. lastly, for my future self: nothing here is a breaking change- correct?
| "developer-visibility", | ||
| "tokio", |
There was a problem hiding this comment.
is tokio a feature? doesn't default-engine feature turn on tokio?
There was a problem hiding this comment.
Ahh, I think you're right
Co-authored-by: Zach Schuermann <[email protected]>
Yeah, I think it's fine to put stuff behind developer-visibility and then change it, we're not promising those will stay the same
Yep, sounds correct
Nope, should not be |
|
|
||
| fn fields_in(field: &StructField) -> usize { | ||
| if let DataType::Struct(ref inner) = field.data_type { | ||
| let mut count = 0; |
There was a problem hiding this comment.
nit: Not sure if we have a preference for FP vs imperative, but we could do this
inner.fields().map(fields_in).sum()| impl DataVisitor for LogVisitor { | ||
| fn visit<'a>(&mut self, row_count: usize, getters: &[&'a dyn GetData<'a>]) -> DeltaResult<()> { | ||
| for i in 0..row_count { | ||
| if let Some(path) = getters[self.add_offset].get_opt(i, "add.path")? { |
There was a problem hiding this comment.
I noticed the DataVisitor::visit has a TODO for a better doc comment. This may be a good opportunity to add that. In particular, I want to know what getters are supposed to look like. Even a pointer to docs for EngineData::extract would be handy. I'm also interested in how TypedGetData::get/TypedGetData::get_opt work wrt the field name. Do we always have to start at the top level field (ie add.path), or could we have done path here since getters only stores leaves.
There was a problem hiding this comment.
Yep, added a better comment there.
Note that this is an internal trait that connector/engine authors don't need to worry about.
Re add.path vs. path, that's actually only for debugging purposes. The getters already are only for leaf nodes in the schema passed to extract. So we'll just return an Err with add.path in the message if we didn't find a String in the getter. It helps a lot when you're working on any code that touches this stuff.
There was a problem hiding this comment.
Aha, so just a label for debuggability, makes sense
This comment was marked as outdated.
This comment was marked as outdated.
