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

fix(events): order entries by insertion order when selecting #11834

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Apr 5, 2024

Fixes: #11823

Replaces: #11829

I went down the rowid rabbit hole that @Stebalien mentioned in #11823, in https://www.sqlite.org/lang_createtable.html#rowid:

The rowid value can be accessed using one of the special case-independent names "rowid", "oid", or "rowid" in place of a column name.

So, it's there already, let's use it! This requires no migration, and just sorts on the way out. The example message I had in #11823 now looks like this from GetActorEventsRaw:

  "entries": [
    {
      "Flags": 3,
      "Key": "$type",
      "Codec": 81,
      "Value": "bnNlY3Rvci11cGRhdGVk"
    },
    {
      "Flags": 3,
      "Key": "sector",
      "Codec": 81,
      "Value": "GScT"
    },
    {
      "Flags": 3,
      "Key": "unsealed-cid",
      "Codec": 81,
      "Value": "2CpYKAABgeIDkiAg8pxbycLUKlq7v8IZyXva5vs6aSxy9/mz5t2ygXvZjAw="
    },
    {
      "Flags": 3,
      "Key": "piece-cid",
      "Codec": 81,
      "Value": "2CpYKAABgeIDkiAg8pxbycLUKlq7v8IZyXva5vs6aSxy9/mz5t2ygXvZjAw="
    },
    {
      "Flags": 1,
      "Key": "piece-size",
      "Codec": 81,
      "Value": "GwAAAAgAAAAA"
    }
  ],

And, from what I can see, $type is coming first on all of them.

I think it's that simple. Will be testing more today with my tooling to make sure my assumptions hold.

}
require.ElementsMatch(t, expectedEntries, event.Entries)
require.True(t, len(event.Entries) > 0)
if event.Entries[0].Key == "$type" && bytes.Equal(event.Entries[0].Value, keyBytes) {
Copy link
Member Author

Choose a reason for hiding this comment

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

important test change here - $type should be first

Comment on lines +307 to +313
{Flags: 0x03, Codec: uint64(multicodec.Cbor), Key: "$type", Value: must.One(ipld.Encode(basicnode.NewString("sector-activated"), dagcbor.Encode))},
{Flags: 0x03, Codec: uint64(multicodec.Cbor), Key: "sector", Value: must.One(ipld.Encode(basicnode.NewInt(int64(rawSector.Sector)), dagcbor.Encode))},
{Flags: 0x03, Codec: uint64(multicodec.Cbor), Key: "unsealed-cid", Value: must.One(ipld.Encode(basicnode.NewLink(cidlink.Link{Cid: expectCommD}), dagcbor.Encode))},
{Flags: 0x03, Codec: uint64(multicodec.Cbor), Key: "piece-cid", Value: must.One(ipld.Encode(basicnode.NewLink(cidlink.Link{Cid: marketPiece.PieceCID}), dagcbor.Encode))},
{Flags: 0x01, Codec: uint64(multicodec.Cbor), Key: "piece-size", Value: must.One(ipld.Encode(basicnode.NewInt(int64(marketPieceSize.Padded())), dagcbor.Encode))},
{Flags: 0x03, Codec: uint64(multicodec.Cbor), Key: "piece-cid", Value: must.One(ipld.Encode(basicnode.NewLink(cidlink.Link{Cid: rawPiece.PieceCID}), dagcbor.Encode))},
{Flags: 0x01, Codec: uint64(multicodec.Cbor), Key: "piece-size", Value: must.One(ipld.Encode(basicnode.NewInt(int64(rawPieceSize.Padded())), dagcbor.Encode))},
Copy link
Member Author

Choose a reason for hiding this comment

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

important test change here - this is a sector activation with 2 pieces, we're asserting they are strictly in this order so the piece-cid + piece-size pairings are intact. Note also the require.Equal below (I've also replaced all the require.ElementsMatch elsewhere that operate on event elements).

@rvagg rvagg merged commit 5d620a7 into release/v1.26.2 Apr 5, 2024
93 of 94 checks passed
@rvagg rvagg deleted the rvagg/event_entry_order branch April 5, 2024 05:46
@@ -557,7 +557,8 @@ func (ei *EventIndex) prefillFilter(ctx context.Context, f *eventFilter, exclude
s = s + " WHERE " + strings.Join(clauses, " AND ")
}

s += " ORDER BY event.height DESC"
// retain insertion order of event_entry rows with the implicit _rowid_ column
s += " ORDER BY event.height DESC, event_entry._rowid_ ASC"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should do a migration where we rename _rowid_? Can happen later.

@rjan90 rjan90 mentioned this pull request Apr 5, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

4 participants