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

Actor version 8 DealProposal type change #987

Closed
1 task
frrist opened this issue Jun 15, 2022 · 6 comments · Fixed by #988
Closed
1 task

Actor version 8 DealProposal type change #987

frrist opened this issue Jun 15, 2022 · 6 comments · Fixed by #988
Assignees

Comments

@frrist
Copy link
Member

frrist commented Jun 15, 2022

Description

The DealLabel is a kinded union of string or byte slice and serializes to a CBOR string or CBOR byte string depending on which form it takes. Lilys model containing DealProposal's expects a string: https://github.com/filecoin-project/lily/blob/v0.10.0/model/actors/market/dealproposal.go#L50. We need to decide if we should provide a migration to store the byte representation of this value, or drop the value if it's not a string

Acceptance criteria

  • Decide if it's worth writing a migration to store byte representation of DealProposal Label value.

Where to begin

@frrist frrist added this to the Support Network Version v16 milestone Jun 15, 2022
@hsanjuan
Copy link
Contributor

Is there a way we can store strings (so that I can potentially find deals with certain label), and bytes?

i.e a string column with string:<label> or bytes:<base64 label>

@frrist frrist self-assigned this Jun 15, 2022
@frrist
Copy link
Member Author

frrist commented Jun 15, 2022

We could do that by:

  1. Creating a migration with MarketDealProposalV8 that has a label field for both bytes and strings, one of which will be null
  2. Encoding the label filed to base64 if its bytes and using the existing table.

@placer14
Copy link
Contributor

placer14 commented Jun 15, 2022

I would migrate to a bytea column and drop the string entirely in the newly proposed table. Bytes are the more general form and gives us the most flexibility from Lily as a source of truth.

Is there a way we can store strings (so that I can potentially find deals with certain label), and bytes?

While it's painful right now where our ingestion and read schemas are the same, I think we should be handling bytes->string as a query/DB concern. Lily should produce the most flexible format (bytes) and we use views or inline functions to convert bytes to text as needed. (https://www.postgresql.org/docs/14/functions-binarystring.html#//apple_ref/cpp/Function/encode)

This pain will only exist until we are able to refine our ETL process to separate our schema's use cases from one another. I also think it's a tolerable pain as we have been discussing how to approach this problem already and will likely chase a solution this year.

If we really don't want to deal with it during querytime, then I would recommend creating a materialized view which updates on insert into the bytes table. The materialized view would be an index on the bytes table with the decoded text for query access. The materialized view would be managed as an Infra/PLDW feature, not a Lily migration(, and would live as part of the read-optimized schema I mentioned in the prior para once it's made).

@hsanjuan
Copy link
Contributor

Chatting with @frrist, new proposal:

  • A boolean column is_string
  • A string column with the base64-encoded bytes

One worry is that it would be nice if the table is exportable to CSV and behaves well, and we're not sure how having a bytea column there would behave.

@frrist
Copy link
Member Author

frrist commented Jun 16, 2022

Adding to this, in the event the label field is empty, the boolean value will be false and the label field will be null.

@frrist
Copy link
Member Author

frrist commented Jun 21, 2022

More context on why the chain state is changing in this way: https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0027.md

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 a pull request may close this issue.

3 participants