add app txn visitors and struct #170
Conversation
Blajda
changed the title
nicklan
requested review from
ryan-johnson-databricks,
nicklan,
rtyler and
roeap
kernel/src/actions/visitors.rs
Outdated
| if let Some(app_id) = getters[0].get_opt(i, "txn.appId")? { | ||
| self.transactions.push(Self::visit_txn(i, app_id, getters)?); | ||
| } |
There was a problem hiding this comment.
If I'm understanding correctly, we're paying to collect an unbounded number of app ids in the hash map -- regardless of whether the caller requested a specific one -- and then we return the entire hash map unless it fails to contain the requested match.
This seems expensive in the common case that the caller only cares about one specific app id.
Could we add a "push-down filter" to the visitor? Still have a hash map, but the visitor ignores mismatching app ids if a specific app id was requested so the map size is bounded to 1?
| if let Some(app_id) = getters[0].get_opt(i, "txn.appId")? { | |
| self.transactions.push(Self::visit_txn(i, app_id, getters)?); | |
| } | |
| if let Some(app_id) = getters[0].get_opt(i, "txn.appId")? { | |
| // If caller requested a specific app id, only visit matches: | |
| // | |
| // !(is_some && (requested_app_id != app_id)) | |
| // ==> !is_some || !(requested_app_id != app_id) | |
| // ==> !is_some || (requested_app_id == app_id) | |
| if !requested_app_id.is_some_and(|requested| requested != app_id) { | |
| val txn = Self::visit_txn(i, app_id, getters)?; | |
| if !self.transactions.contains_key(&txn.app_id) { | |
| self.transactions.insert(txn.app_id.clone(), txn); | |
| } | |
| } | |
| } |
We could also implement the actual log replay here, so we don't need to do it later and don't need a second visitor for log replay.
(a follow-on optimization, probably not in this PR, could push down the requested app id as a filter to the underlying file reads, so we don't even materialize mismatching app ids in the first place)
Thanks so much. Given your suggestions I removed the irrelevant bits and simplified the logic. From an overall unit testing perspective it would be nice if I could assert which files were read so an assert for the minimum amount of read can be performed. |
| pub fn new(snapshot: Arc<Snapshot>) -> Self { | ||
| TransactionScanner { snapshot } | ||
| } |
There was a problem hiding this comment.
aside: @nicklan If I counted correctly, this brings us to six structs whose new is just a thin wrapper for convenience. Do you think it's worth bringing in the derive-new crate? Tho this reddit thread has some arguments for why trivial new might be an antipattern?
There was a problem hiding this comment.
Mmm, it's an interesting point. I think the only reason I'd want to keep it this way is that we could easily change things in the new method and/or add new fields with default values (which we could do using derive-new) and not require upstream changes.
I created #181 to remind us to think about this
Blajda
force-pushed
the
app-txn
branch
2 times, most recently
from
a463e47
to
e8c2a18
Compare
| pub fn new(snapshot: Arc<Snapshot>) -> Self { | ||
| TransactionScanner { snapshot } | ||
| } |
There was a problem hiding this comment.
Mmm, it's an interesting point. I think the only reason I'd want to keep it this way is that we could easily change things in the new method and/or add new fields with default values (which we could do using derive-new) and not require upstream changes.
I created #181 to remind us to think about this
Implements visitors and structures required for reading application transactions that exist on a snapshot.
A similar pattern to the
LogReplayScannerwas implemented where users can explicitly scan metadata for the latest transaction for all application or obtain the latest transaction for particular application.I decided against adding the
TransactionMaptoSnapshotsince snapshot only require protocol + metadata to be built and obtaining the latest transaction may require scanning past that point. Also the overall design ofSnapshotseems to be immutable and it would not make much sense to have methods that mutate it.