-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add support for list query arguments #1389
Conversation
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.
Awesome... I can see why this is a bit suboptimal, but given that this code doesn't run in any hot path, it is probably okay 👍
packages/cinterop/src/jvm/kotlin/io/realm/kotlin/internal/interop/RealmValue.kt
Outdated
Show resolved
Hide resolved
packages/cinterop/src/nativeDarwin/kotlin/io/realm/kotlin/internal/interop/RealmValue.kt
Outdated
Show resolved
Hide resolved
@@ -2213,6 +2213,34 @@ class QueryTests { | |||
} | |||
} | |||
|
|||
@Test | |||
fun query_inListArgument() { |
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.
I don't think we need to be exhaustive here, but it would be nice with a test for this that takes objects as arguments, just as a smoke test since it involves are few more moving parts.
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.
Actually we also need to test strings with '
and "
I have added and extended some test cases (list with single element, strings and objects) in f82058e#diff-db6b549cfdb4b7e0512e5d470bdd52902d32f0647ff5d9b8e1a9f72b7b952546 but some fail, so will have to await resolution of core issues. |
Still fails for object links. Created realm/realm-core#6688 to track that. |
This is now waiting for realm/realm-core#6705 to be merged. |
realm/realm-core#6705 has been merged, so should try to update core. There seem to be a number of API changes, so not just a submodule update though. |
This adds support for passing any
Iterable
as list arguments to the RQLIN
operator.Input for review
I took some shortcuts for handling allocation and assignments of lists for query arguments. It is not as efficient as it could be, but given that we might want to tweak our scheme for transferring values to the C-API if we can generate the code, I chose to fix support for list arguments and will leave the optimizations for later. The current C-API wrappers does not offer identical was to work with lists of contiguous value types. It didn't find a uniform way to allocate a list of element and then update the elements in place with the current wrapper frameworks. Swig's
array_functions
copies values when accessed by index whereas Kotlin Native's cinterop only allows you to work with the element by reference. As our overall converter infrastructure is currently creating instances and passing it around, it was easier to justmemcpy
the elements into place in Kotlin Native than reworking the whole infrastructure to assign native references on JVM (and that would also not allow us to use the generated Swig array-functions forvalueArray
).Closes: #929