Order entities by insertion #6408
Conversation
| @@ -284,6 +284,7 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep | |||
| SELECT *, i.$ROW_ID | |||
| FROM $list e, ${getRowIdTableName(list)} i | |||
| WHERE e._id = i._id | |||
| ORDER BY i.$ROW_ID | |||
There was a problem hiding this comment.
I don't think we strictly need the ORDER BY here and in the other queryWithAttachedRowId overload and I can't work out a way to test it (I think the query is structured so that it always happens to works out). I feel safer having it there though so.
| @@ -325,7 +327,7 @@ class DatabaseEntitiesRepository(context: Context, dbPath: String) : EntitiesRep | |||
|
|
|||
| writableDatabase.execSQL( | |||
| """ | |||
| CREATE TABLE ${getRowIdTableName(it)} AS SELECT _id FROM $it; | |||
| CREATE TABLE ${getRowIdTableName(it)} AS SELECT _id FROM $it ORDER BY _id; | |||
There was a problem hiding this comment.
As far as I understand it, _id should have the same order as insertion.
There was a problem hiding this comment.
There is a potential issue with this assumption: if we delete some entities, their IDs might be reused. While this shouldn't occur under normal circumstances, once we reach the maximum possible ID value of 9223372036854775807, IDs will start to be reused. However, this number is large enough that we don't need to worry about it for now I guess. Do you agree @lognaturel?
There was a problem hiding this comment.
Ahh the table gets recreated every time we add/delete an entity so it won't be and issue.
There was a problem hiding this comment.
Although, as mentioned above, this is not an issue at the moment, it might be a good idea to add a test that serves as documentation hmm?
There was a problem hiding this comment.
Preserving order is a nice to have more than a must have so I think it's ok to leave edge cases untested. It's only a nice to have because the order is not necessarily meaningful currently. Longer-term, we'll need to be able to let users specify a custom sort order.
|
We can merge this, right, @grzesiek2010? I'll let you do it just in case you do still want to add a test! |
|
Tested with Success! Verified on device with Android 14 Verified cases:
|
|
Tested with Success! Verified on device with Android 10 |
This makes sure entities are in insertion order (from the CSV and locally) by changing the way we generate
indexso that it's ordered. It appeared thatindexordering was based on entityidwhich would appear "random" (as they would usually be UUIDs).Why is this the best possible solution? Were any other approaches considered?
Comments inline.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
I've already checked that
EntitiesBenchmarkTeststill passes so it seems like we're ok in terms of performance. Other than that, I think the main thing is to check that the entities are always in the "expected" order in selects (filtered and unfiltered).Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still passDateFormatsTest