Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

Remove TraceReader trait. #203

Merged
merged 1 commit into from
Oct 10, 2022
Merged

Remove TraceReader trait. #203

merged 1 commit into from
Oct 10, 2022

Conversation

ryzhyk
Copy link
Collaborator

@ryzhyk ryzhyk commented Oct 10, 2022

TraceReader only defined a single method, map_batches. Work on persistent traces showed that this is not a good design, as it exposes the internal structure of the trace. We only used this API in one place in distinct.rs, and that code would have to be re-written anyway as we generalize distinct to work in arbitrary nested scopes.

We eliminate the map_batches method from the public API and downgrade it to a private method of Spine. We also remove the no longer useful trait TraceReader.

Addresses a TODO in PR #124.

`TraceReader` only defined a single method, `map_batches`.  Work on
persistent traces showed that this is not a good design, as it exposes
the internal structure of the trace.  We only used this API in one place
in `distinct.rs`, and that code would have to be re-written anyway as we
generalize `distinct` to work in arbitrary nested scopes.

We eliminate the `map_batches` method from the public API and downgrade
it to a private method of `Spine`.  We also remove the no longer useful
trait `TraceReader`.
@ryzhyk ryzhyk requested review from gz and Kixiron October 10, 2022 20:09
Copy link
Contributor

@gz gz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@codecov
Copy link

codecov bot commented Oct 10, 2022

Codecov Report

Merging #203 (3abcde8) into main (874b979) will decrease coverage by 0.00%.
The diff coverage is 90.90%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #203      +/-   ##
==========================================
- Coverage   83.40%   83.40%   -0.01%     
==========================================
  Files         126      126              
  Lines       23482    23468      -14     
==========================================
- Hits        19585    19573      -12     
+ Misses       3897     3895       -2     
Impacted Files Coverage Δ
src/operator/join.rs 86.68% <ø> (ø)
src/operator/trace.rs 76.57% <ø> (ø)
src/operator/upsert.rs 95.00% <ø> (ø)
src/trace/mod.rs 46.00% <ø> (ø)
src/operator/distinct.rs 94.63% <87.50%> (-0.14%) ⬇️
src/trace/spine_fueled.rs 83.61% <100.00%> (+0.18%) ⬆️
src/time/nested_ts32.rs 72.60% <0.00%> (-1.37%) ⬇️
src/circuit/circuit_builder.rs 84.37% <0.00%> (+0.05%) ⬆️
src/nexmark/generator/bids.rs 99.20% <0.00%> (+0.79%) ⬆️

@ryzhyk ryzhyk merged commit e50067c into vmware-archive:main Oct 10, 2022
@ryzhyk ryzhyk deleted the hide_map_batches branch October 10, 2022 20:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants