-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
} | ||
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) { |
There was a problem hiding this comment.
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
{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))}, |
There was a problem hiding this comment.
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).
b279385
to
1800d64
Compare
@@ -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" |
There was a problem hiding this comment.
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.
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: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
: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.