Expanding time resolution into millisecs #2679

kneth · 2016-04-26T13:22:09Z

Closes #833

Current status: ready to be merged

bmunkholm · 2016-04-26T13:43:29Z

realm/realm-jni/src/io_realm_internal_TableQuery.cpp

@@ -111,7 +111,6 @@ static jlong getDistinctViewWithHandover
        switch (table->get_column_type(S(columnIndex))) {
            case type_Bool:
            case type_Int:
-            case type_DateTime:


Does timestamp not support distinct? That's very breaking.

I had to disable it as I got seg. faults. It has been fixed in core.

stk1m1 · 2016-04-26T16:43:53Z

realm/realm-jni/src/io_realm_internal_table.cpp

            try {
                TableView* pTableView = new TableView( pTable->get_sorted_view(S(columnIndex), ascending != 0 ? true : false) );
                return reinterpret_cast<jlong>(pTableView);
            } CATCH_STD()
        default:
-            ThrowException(env, IllegalArgument, "Sort is currently only supported on integer, boolean, double, float, String, and Date columns.");
+            ThrowException(env, IllegalArgument, "Invalid type - Only String, Date, boolean, short, int, long and their boxed variants are supported.");


boolean, byte, short, int, long

cmelchior · 2016-04-27T19:17:25Z

realm/realm-jni/src/util.hpp

@@ -654,6 +655,7 @@ extern jmethodID java_lang_float_init;
 extern jclass java_lang_double;
 extern jmethodID java_lang_double_init;

+


Remove newline

cmelchior · 2016-04-27T19:28:34Z

Done with a quick pass. Generally looks great. Like the internal renaming from Date to Timestamp. I am a bit concerned that some dividing by 1000 hasn't been caught by unit tests, which indicate our test coverage could be better?

kneth · 2016-04-28T10:38:20Z

@cmelchior We should definitely investigate our coverage to see if important methods are not tested. But in the case of the TableView setters, the conclusion might be that we should remove them.

cmelchior · 2016-04-28T11:05:20Z

Removing unused setters / getters seems like a good idea. We really shouldn't have unused code. Probably in another PR though.

nhachicha · 2016-04-30T17:18:44Z

realm/realm-jni/src/io_realm_internal_TableQuery.cpp

@@ -1523,42 +1521,44 @@ JNIEXPORT jdouble JNICALL Java_io_realm_internal_TableQuery_nativeAverageDouble(


 // date aggregates
-
-JNIEXPORT jobject JNICALL Java_io_realm_internal_TableQuery_nativeMaximumDate(
+// FIXME: This is a rough workaround while waiting for https://github.com/realm/realm-core/issues/1720 to be solved


the issue you're pointing to is closed, are we still waiting for Core?

I'll update the comment to be a link to realm/realm-core#1745

Please update the link in the comment ☝️ 😄

cmelchior · 2016-05-02T09:30:38Z

realm/realm-jni/build.gradle

@@ -201,7 +201,7 @@ task downloadCore(group: 'build setup', description: 'Download the latest versio
                    throw new GradleException("Invalid checksum for file '" +
                            "${project.coreArchiveFile.getName()}'. Expected " +
                            "${project.coreSha256Hash.toLowerCase()} but got " +
-                            "${calculatedHash.toLowerCase()}.");
+                                "${calculatedHash.toLowerCase()}.");


Wrong indent?

cmelchior · 2016-05-02T09:51:20Z

realm/realm-jni/src/io_realm_internal_table.cpp

@@ -254,6 +259,9 @@ JNIEXPORT void JNICALL Java_io_realm_internal_Table_nativeConvertColumnToNullabl
                case type_Table:
                    // checked previously
                    break;
+                case type_OldDateTime:
+                    // not used


Throw exception instead? In case someone accidentally uses it?

It should not be possible. All old Realm files are converted, and new Realm will only use Timestamp.

Then throwing an exception should be safe :) IMO I would much rather we threw exceptions in these cases as it will catch anyone accidentally using them in the future.

If it's not possible - it should assert.

cmelchior · 2016-05-02T10:13:03Z

Done with review. Mostly minor stuff, but there seems to be a bunch of FIXME's around find_all_date/find_first_date. I am not sure we even use those in the public API, so maybe something we should consider removing all together? Don't have to be in this PR though.

…g-to-timestamp

precision of Date fields.

AndyDentFree · 2016-05-02T14:13:47Z

realm/realm-jni/src/io_realm_internal_UncheckedRow.cpp

@@ -217,7 +217,7 @@ JNIEXPORT jlong JNICALL Java_io_realm_internal_UncheckedRow_nativeGetLinkView
    if (!ROW_VALID(env, ROW(nativeRowPtr)))
        return 0;

-    LinkView* link_view_ptr = LangBindHelper::get_linklist_ptr( *ROW( nativeRowPtr ), S( columnIndex) );
+    LinkViewRef* link_view_ptr = const_cast<LinkViewRef*>(&(LangBindHelper::get_linklist_ptr(*ROW(nativeRowPtr), S(columnIndex))));
    return reinterpret_cast<jlong>(link_view_ptr);


I don't understand how this ends up as a string and there's a lot of casting here. Unless we have a string buried as some kind of opaque type, this looks like a bug, which predates the change to Timestamp.

I am not quite sure what you mean: no strings are involved there. With core 0.100.0, LangBindHelper::get_linklist_ptr() has been changed to return a const LinkViewRef (a shared_ptr<LinkView*>. As we cannot use a shared_ptr in Java, I have to store a pointer to shared_ptr (returned as a jlong). And I need to get rid of the constness too.

That gives us a double indirection which you can see in all the LinkView methods.

Sorry, scratch my comment - I was misinterpreting the github diff as the return type being a jstring - hadn't noticed the line number jump that it was a different method!

AndyDentFree · 2016-05-03T09:42:06Z

👍 preferably with those exception strings fixed to restore the word sorting

…g-to-timestamp

cmelchior · 2016-05-03T09:54:44Z

👍 , but please create an issue with the overflow problem with dates coming from other platforms. As a minimum we need to document it somewhere instead of hiding it in a FIXME in the code.

EDIT: This isn't addressed? #2679 (comment)

kneth · 2016-05-03T10:05:17Z

@cmelchior An exception is thrown (#2679 (comment))

And I have created #2722.

cmelchior · 2016-05-03T10:13:01Z

👍

Expanding time resolution into millisecs

aa3c414

kneth self-assigned this Apr 26, 2016

kneth added the S:In Progress label Apr 26, 2016

bmunkholm reviewed Apr 26, 2016
View reviewed changes

WiP

8d582c2

stk1m1 reviewed Apr 26, 2016
View reviewed changes

Fixing tests and mistakes

292bdac

cmelchior reviewed Apr 27, 2016
View reviewed changes

More fixes

3c7567b

PR feedback

74075e3

nhachicha reviewed Apr 30, 2016
View reviewed changes

Upgrading to core 0.100.

713325a

cmelchior reviewed May 2, 2016
View reviewed changes

PR feedback

d71c427

kneth added 3 commits May 2, 2016 12:58

Merge branch 'master' of github.com:realm/realm-java into kg/switchin…

5da68b4

…g-to-timestamp

Updating CHANGELOG to include news on a new core and expansion of

e7e38d6

precision of Date fields.

Minor updates

14f5a8c

AndyDentFree reviewed May 2, 2016
View reviewed changes

Further PR feedback

c18a38e

Merge branch 'master' of github.com:realm/realm-java into kg/switchin…

375a2bd

…g-to-timestamp

kneth merged commit fc23cfc into master May 3, 2016

kneth removed the S:In Progress label May 3, 2016

kneth deleted the kg/switching-to-timestamp branch May 3, 2016 11:05

github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expanding time resolution into millisecs #2679

Expanding time resolution into millisecs #2679

kneth commented Apr 26, 2016 •

edited

Loading

bmunkholm Apr 26, 2016 •

edited

Loading

kneth Apr 28, 2016

stk1m1 Apr 26, 2016

kneth Apr 28, 2016

cmelchior Apr 27, 2016

cmelchior commented Apr 27, 2016

kneth commented Apr 28, 2016

cmelchior commented Apr 28, 2016

nhachicha Apr 30, 2016

kneth May 2, 2016

cmelchior May 2, 2016

cmelchior May 2, 2016

cmelchior May 2, 2016

kneth May 2, 2016

cmelchior May 2, 2016

bmunkholm May 2, 2016

cmelchior commented May 2, 2016

AndyDentFree May 2, 2016

kneth May 3, 2016

AndyDentFree May 3, 2016

AndyDentFree commented May 3, 2016

cmelchior commented May 3, 2016 •

edited

Loading

kneth commented May 3, 2016

cmelchior commented May 3, 2016

		@@ -654,6 +655,7 @@ extern jmethodID java_lang_float_init;
		extern jclass java_lang_double;
		extern jmethodID java_lang_double_init;

Expanding time resolution into millisecs #2679

Expanding time resolution into millisecs #2679

Conversation

kneth commented Apr 26, 2016 • edited Loading

bmunkholm Apr 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmelchior commented Apr 27, 2016

kneth commented Apr 28, 2016

cmelchior commented Apr 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmelchior commented May 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyDentFree commented May 3, 2016

cmelchior commented May 3, 2016 • edited Loading

kneth commented May 3, 2016

cmelchior commented May 3, 2016

kneth commented Apr 26, 2016 •

edited

Loading

bmunkholm Apr 26, 2016 •

edited

Loading

cmelchior commented May 3, 2016 •

edited

Loading