feat: use data container wrapper for mutable metadata #559
Conversation
nathanielc
requested review from
stephhuynh18,
samika98 and
stbrody
and removed request for
a team
nathanielc
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
nathanielc
force-pushed
the
feat/should-index
branch
from
6d2f19c to
e4bcc38
Compare
nathanielc
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
nathanielc
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
event-svc/src/event/service.rs
Outdated
| Error::new_app(anyhow::anyhow!( | ||
| "Malformed event found in the database: {}", | ||
| e | ||
| )) | ||
| })?; | ||
|
|
||
| // Small wrapper container around the data field to hold other mutable metadata for the |
There was a problem hiding this comment.
any reason we're defining this type inside the body of the (already long) transform_raw_events_to_conclusion_events function? Can we pull it out so the function is easier to read?
There was a problem hiding this comment.
It's only used within this context so I placed it here inside the function. I can look into how to shorten this function to make it easier to read.
There was a problem hiding this comment.
can we just make it file-local instead of function-local?
|
@stbrody @samika98 Good call on making sure we are precise on the behavior when the metadata key is missing in the data header. The PR has been updated to transitively carry forward the metadata map and only if its present change update it. The aggregation reproduces the known state of the metadata for each new event so that clients do not have to do that work, which is aligned with the purpose of the aggregator. |
nathanielc
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
nathanielc
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
Existing MID events have a header on data payloads. This data is mutable metadata. We needed a mechanism to represent this kind of data within the conclusion events and doc states. We added a simple wrapper type with metadata and data fields.
nathanielc
force-pushed
the
feat/should-index
branch
from
7ab6bf9 to
d70a3af
Compare
Instead of expecting each data event header to repeat the metadata we carry forward the metadata from the previous state and only if its present do we apply a change to the metadata. Tests have been updated to reflect this new behavior.
nathanielc
force-pushed
the
feat/should-index
branch
from
d70a3af to
e24a646
Compare
nathanielc
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
event-svc/src/event/service.rs
Outdated
| Error::new_app(anyhow::anyhow!( | ||
| "Malformed event found in the database: {}", | ||
| e | ||
| )) | ||
| })?; | ||
|
|
||
| // Small wrapper container around the data field to hold other mutable metadata for the |
There was a problem hiding this comment.
can we just make it file-local instead of function-local?
olap/src/aggregator/ceramic_patch.rs
Outdated
|
|
||
| #[derive(Debug)] | ||
| struct CeramicPatchEvaluator; | ||
|
|
||
| impl CeramicPatchEvaluator { | ||
| fn apply_patch(patch: &str, previous_state: &str) -> Result<String> { | ||
| let patch: Vec<json_patch::PatchOperation> = serde_json::from_str(patch) | ||
| let patch: MIDDataContainer<Vec<PatchOperation>> = serde_json::from_str(patch) |
There was a problem hiding this comment.
is this going to work given that in the events themselves the field is called header but in the state it's called metadata?
There was a problem hiding this comment.
It's a bit surprising to me to see that we're using the same type (MIDDataContainer) for both events and states. If it works that's fine, just want to make sure we don't have any issues from overloading the type like that.
There was a problem hiding this comment.
Yeah sorry about the confusion. The field is always called metadata in the context that this struct is used.
The translation from raw event to conclusion event renames the field from header to metadata. (This was intentional as most fields inside the header end up as dimensions and only shouldIndex ends up in the metadata).
I see the confusion that a conclusion event data field has the same structure as an aggregated state data. I'll consider using two different (yet similar types) to make it clear.
There was a problem hiding this comment.
Decided the structures are the same so a good use for generics, with some type aliases to make it clear the context in how they should be used.
struct MIDDataContainer<D> {
metadata: BTreeMap<String, serde_json::Value>,
data: D,
}
type MIDDataContainerPatch = MIDDataContainer<Vec<PatchOperation>>;
type MIDDataContainerState = MIDDataContainer<serde_json::Value>;
nathanielc
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
Existing MID events have a header on data payloads. This data is mutable metadata. We needed a mechanism to represent this kind of data within the conclusion events and doc states. We added a simple wrapper type with metadata and data fields.
Fixes AES-368