Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: consistent pipeline table names and schemas #587

Merged
merged 14 commits into from
Nov 15, 2024

Conversation

nathanielc
Copy link
Collaborator

No description provided.

@nathanielc nathanielc requested a review from a team as a code owner November 12, 2024 23:08
@nathanielc nathanielc requested review from dav1do and removed request for a team November 12, 2024 23:08
Copy link
Contributor

@dav1do dav1do left a comment

Choose a reason for hiding this comment

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

Looks good, a couple of small questions.

pipeline/src/lib.rs Show resolved Hide resolved
pipeline/src/lib.rs Show resolved Hide resolved
pipeline/DESIGN.md Outdated Show resolved Hide resolved
pipeline/DESIGN.md Outdated Show resolved Hide resolved
pipeline/DESIGN.md Outdated Show resolved Hide resolved
pipeline/DESIGN.md Outdated Show resolved Hide resolved
pipeline/DESIGN.md Show resolved Hide resolved
pipeline/DESIGN.md Show resolved Hide resolved
pipeline/DESIGN.md Show resolved Hide resolved
pipeline/DESIGN.md Show resolved Hide resolved
pipeline/DESIGN.md Outdated Show resolved Hide resolved
With this change we use bytes instead of string for the state column.
Casting to a string is easy and straightforward.
pipeline/DESIGN.md Outdated Show resolved Hide resolved
pipeline/DESIGN.md Outdated Show resolved Hide resolved
pipeline/DESIGN.md Outdated Show resolved Hide resolved
graph LR;
raw_events;
streams;
chain_proofs;
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure about this name for this table.

First, is the table actually going to include the proofs? Or just the timestamp conclusions from the proofs? The actual proofs are in the raw_events table right? And then wouldn't this table be derived from raw_events? Or do you actually plan on splitting the raw data for time events to be in a different table than init and data events?

Also I kind of prefer time_proofs to chain_proofs, but that's a minor preference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The intent of this table is to store the ChainInclusionProof structs. This way the conclusion_events table can be built by joining the raw_events table with this table to make time based conclusions. Should we call it chain_inclusion_proofs? It is not the proof contained within the raw events but rather the timestamp and root cid that we know are included in a chain.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah that struct is kind of named weird cause it's not actually storing the proof. It's storing the derived conclusions that come from validating the proof.

How about... time_conclusions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Works for me.

pipeline/DESIGN.md Show resolved Hide resolved
pipeline/DESIGN.md Outdated Show resolved Hide resolved
pipeline/DESIGN.md Outdated Show resolved Hide resolved
pipeline/DESIGN.md Show resolved Hide resolved
pipeline/DESIGN.md Outdated Show resolved Hide resolved
pipeline/DESIGN.md Outdated Show resolved Hide resolved
Co-authored-by: Spencer T Brody <[email protected]>
Co-authored-by: Spencer T Brody <[email protected]>
Copy link
Collaborator

@stbrody stbrody left a comment

Choose a reason for hiding this comment

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

a few small comments but otherwise LGTM

pipeline/DESIGN.md Outdated Show resolved Hide resolved
pipeline/DESIGN.md Outdated Show resolved Hide resolved
pipeline/DESIGN.md Outdated Show resolved Hide resolved
pipeline/DESIGN.md Outdated Show resolved Hide resolved

| name | code | description |
| ---- | ---- | ----------- |
| Data | 0x00 | An event containing data for the stream |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this "Event Types" table only meant to apply to conclusion events? It's not nested under the conclusion events section of this document. This document also describes the raw events table as well, and in that table init events do have their own event

@nathanielc nathanielc added this pull request to the merge queue Nov 15, 2024
Merged via the queue into main with commit 315d791 Nov 15, 2024
5 checks passed
@nathanielc nathanielc deleted the docs/pipeline-design branch November 15, 2024 19:06
@smrz2001 smrz2001 mentioned this pull request Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants