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

Implement stable sorting for events #84

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rasky
Copy link
Collaborator

@rasky rasky commented Jul 2, 2016

Currently, TrailDB sorts internal events using libc's qsort function, which is not stable. This means that events with identical timestamp will be reordered, which is especially confusing in case the user is feeding already-sorted data.

This patch implements stable sorting through timsort.

NOTE: timsort is potentially very good with pre-sorted data, but we don't get to exploit that because events are being reversed (somewhere in the code coming up to the point being patches). A possible further improvement is to fix this, so that timsort sees pre-sorted data and goes faster.

#define SORT_CMP(x,y) compare((x).timestamp, (y).timestamp)
#include "sort/sort.h"

void events_sort(struct tdb_grouped_event *buf, uint64_t num_events)
Copy link
Member

Choose a reason for hiding this comment

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

could you use 4 space indents for consistency with the existing codebase

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@KalenAnson
Copy link

KalenAnson commented Jul 5, 2016

I tested this PR with simple dumps tdb dump -i my.tdb and with a variety of filters --filter='type=FOO type=BAR' over items with complete and incomplete field values. So far the events from db_cursor_next look to be ordered as they were entered into the trail, even if the event was missing fields and if the events were filtered.

Looks good to me. Thanks @rasky.

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.

None yet

3 participants