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

Add Decimal128 support #1179

Merged
merged 25 commits into from
Jan 18, 2023
Merged

Add Decimal128 support #1179

merged 25 commits into from
Jan 18, 2023

Conversation

gagik
Copy link
Contributor

@gagik gagik commented Dec 14, 2022

This PR add support for Decimal128 as fields and in RealmAny.

class Decimal128Object: RealmObject {
    val decimal128Field: Decimal128 = Decimal128("1.23422354E53")

    val any: RealmAny = RealmAny.create(Decimal128("1.234523E34"))
}

TODO:

Closes #653

@@ -112,6 +113,7 @@ internal object FqNames {
val REALM_BACKLINKS = FqName("io.realm.kotlin.types.BacklinksDelegate")
val REALM_OBJECT_ID = FqName("io.realm.kotlin.types.ObjectId")
val KBSON_OBJECT_ID = FqName("org.mongodb.kbson.BsonObjectId")
val KBSON_DECIMAL128 = FqName("org.mongodb.kbson.BsonDecimal128")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I had to specify org.mongodb.kbson.BsonDecimal128 as it would error with org.mongodb.kbson.Decimal128. Seemed to work fine regardless but not too sure if this would cause errors in some cases.

@rorbech rorbech self-assigned this Jan 5, 2023
@rorbech rorbech marked this pull request as ready for review January 10, 2023 16:18
@rorbech
Copy link
Contributor

rorbech commented Jan 10, 2023

The core fix for querying on NaN is merged, but it is done on top of Core 13 so need that to remove that last FIXME in the roundtripSpecialValues ... but should be up for an initial review.

Copy link
Contributor

@edualonso edualonso left a comment

Choose a reason for hiding this comment

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

Looks good but I think you are missing testing Decimal128 inside RealmAny values in lists and sets and by extension aggregations in QueryTests. You should add a RealmAny.create(Decimal128(...)) entry in

Copy link
Contributor

@edualonso edualonso left a comment

Choose a reason for hiding this comment

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

Nice improvements, especially to the descriptors and the new flags. Also worth noting your comment during our conversation about moving TypeDescriptor.aggregateClassifiers and TypeDescriptor.anyClassifiers, so for now keep the changes to a minimum. But overall looks great.

Copy link
Contributor

@cmelchior cmelchior left a comment

Choose a reason for hiding this comment

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

Nice 🚀 Only a few minor questions/comments.

override fun decimal128Transport(value: Decimal128?): RealmValue =
createTransport(value, realm_value_type_e.RLM_TYPE_DECIMAL128) {
decimal128 = realm_decimal128_t().apply {
w = longArrayOf(it.low.toLong(), it.high.toLong())
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably correct, but should this not be ulongs?

Copy link
Contributor

Choose a reason for hiding this comment

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

JNI does not support ULong so need to assign it as a long array. But updated to use the experimental unsigned implementation of

w = ulongArrayOf(it.low, it.high).toLongArray()

This is symmetric to what we are doing on the output side in https://github.com/realm/realm-kotlin/pull/1179/files#diff-ecf6dda367cfda31a14c17876fa5eacac9bf99b8a34f9c9b71d6da4b8d062c96R45.

The values are passed to the C-API as the signed reinterpretation of the corresponding elements as described in https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/to-long-array.html

Returns an array of type LongArray, which is a copy of this array where each element is a signed reinterpretation of the corresponding element of this array.

And the C-API does the similar unsigned reinterpretation on the other side. I have verified the special values written in https://github.com/realm/realm-kotlin/pull/1179/files#diff-e3a1343b699de7bd2efcc1bd08f8f0406dc545438996074472ce7dbcd36eb159R35 is actually displayed correctly when opening the written realm with Realm studio.

public object TypeDescriptor {
object TypeDescriptor {
enum class AggregatorSupport {
MINMAX, SUM;
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels a bit weird that MINMAX covers two different ways. It would probably make more sense to split them?

Copy link
Contributor

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 would ever have a type that supports min but not max, but now I split it. It is also just in test infrastructure code so not that critical.

@rorbech rorbech merged commit 0b4ca0b into main Jan 18, 2023
@rorbech rorbech deleted the gagik/decimalsupport branch January 18, 2023 09:06
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Decimal128 field types
4 participants