Clean up inspect-table and add Cdc action support #527
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #527 +/- ##
=======================================
Coverage 80.31% 80.31%
=======================================
Files 62 62
Lines 13541 13541
Branches 13541 13541
=======================================
Hits 10876 10876
Misses 2106 2106
Partials 559 559 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| &getters[cdc_start..cdc_end], | ||
| )?) | ||
| } else { | ||
| // TODO: Add CommitInfo support (tricky because all fields are optional) |
There was a problem hiding this comment.
made #528 for commit info support
| let mut protocol_offset = 0; | ||
| let mut metadata_offset = 0; | ||
| let mut set_transaction_offset = 0; | ||
| fn new(log_schema: &'static SchemaRef) -> LogVisitor { |
There was a problem hiding this comment.
Any reason for SchemaRef to be static?
There was a problem hiding this comment.
Because rust is a bit silly, and you can't probe a HashMap<String, _> with &str (there's a Borrow<str> for String but no Borrow<&str> for String). So I'm storing HashMap<&'static str, _> instead, to avoid allocating a string for every map probe and/or adding a lifetime to the enclosing struct. I hit a similar issue with ColumnName btw, but could solve that one by defining a suitable Borrow (can do that since it's a local trait).
There was a problem hiding this comment.
there's a Borrow for String but no Borrow<&str> for String
wow you're right that is silly. TIL the difference between str and &str 👍
to avoid allocating a string for every map probe and/or adding a lifetime to the enclosing struct
that makes sense.
can do that since it's a local trait
And we can't do this because Borrow is a foreign trait, and SchemaRef is a foreign struct (from inspect-table's perspective). While in the example, ColumnName was at least local.
Thanks @scovich!
There was a problem hiding this comment.
Update: Turns out if I pass e.g. offsets[ADD_NAME] instead of offsets[&ADD_NAME] then it works!
What changes are proposed in this pull request?
The inspect-table example was brittle -- sensitive to field order and requiring a lot of manual fiddlery to add a new action -- and did not have support for the Cdc action.
Rework the code to be simpler and more flexible, and take advantage of that to add Cdc support.
How was this change tested?
Ran it on a table.