-
Notifications
You must be signed in to change notification settings - Fork 178
Support sorting based on values from a dictionary. #5140
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
Conversation
Realm welcomes all contributions! The only requirement we have is that, like many other projects, we need to have a Contributor License Agreement (CLA) in place before we can accept any external code. Our own CLA is a modified version of the Apache Software Foundation’s CLA. Our records show that CLA has not been signed by @Subv. Please submit your CLA electronically using our Google form so we can accept your submissions. After signing the CLA you can recheck this PR with a |
@cla-bot check |
The cla-bot has been summoned, and re-checked this pull request! |
@Subv Thank you for your work. I absolutely get the motivation for doing this, but as you have found out, the current design of the sort mechanism is not exactly geared towards this. I will definitely avoid adding to the ColKey struct! A more viable solution could be to have |
4e3bbc7
to
e2a55e7
Compare
@jedelbo Thank you for the feedback! I reworked the patch a little to use a custom type derived from std::variant called SortableColumnKey and modified the sort descriptor code so that it can work with this new type. The variant takes care of abstracting the key-type-specific behaviors using std::visit. |
This seems like a reasonable approach to me. |
@tgoyne Thanks! I included the change for std::string child keys and the std::variant -> mpark::variant change. |
Sorting is implemented by storing the parent dictionary's ColKey and the specific dictionary key we want to sort by. This information is stored inside a new type of key called DictionaryKey. A new variant-derived structure called SortableColumnKey was added to handle dispatching the key-specific behavior needed to support both ColKeys and DictionaryKeys in the Sorter. There is currently a workaround to a GCC bug, see https://stackoverflow.com/questions/51309467/using-stdvisit-on-a-class-inheriting-from-stdvariant-libstdc-vs-libc and https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90943 for more information. Sorting by direct dictionary keys and following links through a dictionary during sorting both work. This is a work-in-progress, the performance characteristics of searching the dictionary for the desired key on every iteration of the sort predicate are unknown. Sorting by dictionary keys using keyPath is currently not implemented (but desired). In the future this could be extended to support sorting on specific indexes of a collection, ie. sort('array[0]'), by adding a new variant type to the SortableColumnKey structure and implementing the necessary visit lambdas.
…ctionaryKey. The DictionaryKeys may live on their own without being attached to their creator's lifetime.
218da95
to
2711893
Compare
Rebased the PR to fix the conflicts and use the Table::check_column function instead of Table::report_invalid_link |
@Subv Your recent updates is more in line with what I would expect. However, I think that using the |
Closing as this is superseded by #5311 |
What, How & Why?
This is a draft and a request for comments about the feature and its implementation, I figured the best way to ask for help was to open a PR.
Note that it is currently possible to filter by a value stored under a specific dictionary key (using the
dictionary['key']
syntax) but is impossible to sort by that same key, this PR aims to change that.I added some tests and FIXME comments about what I'm unsure about. This is my first time actually touching Realm's core code :)
Sorting is implemented by storing the parent dictionary's ColKey and the specific dictionary key we want to sort by.
This is a work-in-progress, the performance characteristics of searching the dictionary for the desired key on every iteration of the sort predicate are unknown.
The dictionary ColKey and the desired sort key are currently stored inside the ColKey structure to avoid changing too much of the SortDescriptor interface.
Sorting by dictionary keys using keyPath is currently not implemented (but desired).
In the future this could be extended to support sorting on specific indexes of a collection, ie. sort('array[0]').
Pain points
Changing the ColKey structure to store the extra fields needed feels like a hack, and makes it way bigger than a single 64 bit integer, this probably affects the cost of copying it all throughout the codebase. Maybe it would be better to store this data directly inside the SortDescriptor? This data is now stored in DictionaryKey and SortableColumnKey.Currently only dictionaries of integers/strings are supported, this could probably be extended to arbitrary-length paths.Following links through the dictionary keys is now supported, it was nice low-hanging fruit after the SortableColumnKey implementation.Example scenario
When you have data in the Entity-Attribute-Value pattern with dynamic attributes and want to allow the users of your application to sort by any of the fields, it's more convenient to have a dictionary of attributes keyed by attribute id in your table and be able to filter and sort by that directly.
Currently the way to solve that is to extract your Attributes into another table, filter that table by AttributeId, and then sort it based on the AttributeValue, after that you can grab the parent objects by looping through the attribute objects and grabbing a backlink to the parent Entity. But this seems convoluted when all you want is a simple sort.
Update 1
Following feedback I reworked this patch so that the ColKey structure is unchanged. A new DictionaryKey structure was added to hold the required information to reference the specific dictionary keys, and the SortDescriptor and Sorter now deal with a derived type of std::variant<ColKey, DictionaryKey> called SortableColumnKey that deals with dispatching the proper key-specific behavior depending on the key type.
☑️ ToDos