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

Query support #81

Merged
merged 97 commits into from
Dec 9, 2020
Merged

Query support #81

merged 97 commits into from
Dec 9, 2020

Conversation

rorbech
Copy link
Contributor

@rorbech rorbech commented Nov 25, 2020

Initial implementation of query support

This mostly just implements the inner building blocks required to do actual queries, but is not trying to finalize the public query API. This task is tracked by #84

For now the query support is implemented as lazy as possible only triggering C-API calls when actually accessing objects. Inspired by JS/Swift SDK's, but not necessarily semantic equivalent, as maybe questions about building and executing queries are not settled yet.

class Realm {
    fun <T: RealmModel> objects(clazz: KClass<T>): RealmResults<T>
}

class RealmResult<T> : AbstractList<T> {
    // For accessing elements
    fun get(index: Int): T

    // For sub queries
    fun query(query: String, vararg args: Any): RealmResults<T>

    // Delete all objects referenced by this result
    fun delete()
}
  • JVM support
  • Native support

Closes #64

@rorbech
Copy link
Contributor Author

rorbech commented Nov 25, 2020

I looked into the links, dug a bit into the Java implementation and tried to play with a lazy evaluated API to get some insight. The last commit holds an overly lazy implementation, that raises question about how to tweak doing the fluent queries, when to raise errors, etc.

Will try to do the basic blocks for Native and then we can maybe discuss the various choices in a meeting.

@rorbech rorbech marked this pull request as ready for review November 26, 2020 11:45
@rorbech rorbech mentioned this pull request Nov 27, 2020
3 tasks
Copy link
Collaborator

@nhachicha nhachicha left a comment

Choose a reason for hiding this comment

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

Nice stab at the query API, I only have one worry concerning throwing for an empty result as stated in the comments

packages/cinterop/build.gradle.kts Show resolved Hide resolved
@@ -53,7 +53,7 @@ class RealmConfiguration private constructor(
data class Builder(
var path: String? = null,
var name: String = "default", // Optional Realm name (default is 'default')
var schema: Any,
var schema: Any? = null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

for now, it's still required to provide a schema until #90 is fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Requiring it here just ruins the ability to build the configuration fluently. As far as I remember there is already a check for the schema when building it.

@@ -52,6 +53,23 @@ class InstrumentedTests {
sample.name = "Hello, World!"
kotlin.test.assertEquals("Hello, World!", sample.name)
realm.commitTransaction()

val objects1: RealmResults<Sample> = realm.objects(Sample::class)
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe add the below to its own query test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried separating the tests a bit. The really annoying part is that we have to maintain it twice as we currently cannot trigger the common tests to run on Android (neither from UI) or by connectedAndroidTest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Besides somehow overcoming the above annoyance, I guess reorganizing test should maybe follow the actual design/implementation. This was merely a PR to show a basic round trip to the underlying query API.

test/src/androidTest/kotlin/io/realm/InstrumentedTests.kt Outdated Show resolved Hide resolved
@rorbech rorbech mentioned this pull request Dec 7, 2020
Base automatically changed from cr/fixme-categorization to master December 8, 2020 10:19
val objects2: RealmResults<Sample> = realm.objects(Sample::class).query("name == $0", "Hello, World!")
val sample2 = objects2[0]
val objects2: RealmResults<Sample> =
realm.objects(Sample::class).query("name == $0", s)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reminder to add a small synopsis of the query syntax (could be in another PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have put a very brief note, basically just pointing to JS. Think it makes most sense to start elaborating along with #84.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query support
3 participants