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

Support indexes on all compatible types #875

Merged
merged 11 commits into from
Nov 3, 2022
Merged

Support indexes on all compatible types #875

merged 11 commits into from
Nov 3, 2022

Conversation

nielsenko
Copy link
Contributor

@nielsenko nielsenko commented Aug 29, 2022

Allow (and correctly apply) @Indexed() on int, bool, String, ObjectId, Uuid, and DateTime (and the nullable version as well).

@RealmModel()
class _Indexable {
  @Indexed()
  late bool aBool;
  @Indexed()
  bool? aNullableBool;
  @Indexed()
  late int anInt;
  @Indexed()
  int? aNullableInt;
  @Indexed()
  late String aString;
  @Indexed()
  String? aNullableString;
  @Indexed()
  late ObjectId anObjectId;
  @Indexed()
  ObjectId? aNullableObjectId;
  @Indexed()
  late Uuid anUuid;
  @Indexed()
  Uuid? aNullableUuid;
  @Indexed()
  late DateTime aDateTime;
  @Indexed()
  DateTime? aNullableDateTime;
}

Fix #797,

@cla-bot cla-bot bot added the cla: yes label Aug 29, 2022
@nielsenko nielsenko self-assigned this Aug 29, 2022
@nielsenko nielsenko force-pushed the kn/indexes branch 2 times, most recently from 98df568 to f958e6f Compare August 29, 2022 20:01
@nielsenko nielsenko marked this pull request as draft August 30, 2022 05:47
@nielsenko nielsenko linked an issue Aug 30, 2022 that may be closed by this pull request
@nielsenko nielsenko force-pushed the kn/nullable-in-collections branch 2 times, most recently from 1ec063c to cb6ada9 Compare September 6, 2022 15:15
@nielsenko nielsenko force-pushed the kn/indexes branch 2 times, most recently from ac66008 to 2a4d8b1 Compare September 9, 2022 14:12
@nielsenko nielsenko force-pushed the kn/indexes branch 2 times, most recently from 1dcdc90 to 1fdf6b5 Compare September 12, 2022 11:25
@nielsenko nielsenko marked this pull request as ready for review September 12, 2022 13:16
@nielsenko nielsenko requested review from blagoev, desistefanova and nirinchev and removed request for blagoev September 12, 2022 13:16
@nielsenko nielsenko force-pushed the kn/indexes branch 2 times, most recently from 02cedea to 376729d Compare September 14, 2022 11:19
@nielsenko nielsenko changed the base branch from kn/nullable-in-collections to kn/const-schema-refactoring September 14, 2022 11:26
Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

Not sure if it's worth writing a test for this, but we could try to prove that the index exists in Core and is being used by opening 2 realms - one with indexed fields, the other with non-indexed fields, then adding 10000 objects and querying for a single object. The test should expect that the query utilizing the index completes much faster.

CHANGELOG.md Outdated Show resolved Hide resolved
generator/lib/src/field_element_ex.dart Outdated Show resolved Hide resolved
generator/lib/src/field_element_ex.dart Outdated Show resolved Hide resolved
@nielsenko
Copy link
Contributor Author

Not sure if it's worth writing a test for this, but we could try to prove that the index exists in Core and is being used by opening 2 realms - one with indexed fields, the other with non-indexed fields, then adding 10000 objects and querying for a single object. The test should expect that the query utilizing the index completes much faster.

Added test of 1.000 lookups against 100.000 objects and compared indexed vs not-indexed for all indexable types, and assert the speedup is at least 10 fold.

@nielsenko nielsenko force-pushed the kn/indexes branch 2 times, most recently from c70cccc to c34a1e0 Compare November 2, 2022 18:15
Copy link
Contributor

@blagoev blagoev left a comment

Choose a reason for hiding this comment

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

Its good, I have couple of questions.

test/indexed_test.dart Show resolved Hide resolved
test/indexed_test.dart Show resolved Hide resolved
common/lib/src/realm_types.dart Show resolved Hide resolved
common/lib/src/realm_types.dart Show resolved Hide resolved
test/indexed_test.dart Outdated Show resolved Hide resolved
test/indexed_test.dart Outdated Show resolved Hide resolved
Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

Small nits for the tests, otherwise looks good.

expect(allNotIndexed.length, max);

// Inefficient, but fast enough for this test
final searchOrder = (List.generate(max, (i) => i)..shuffle(Random(42))).take(1000).toList();
Copy link
Member

Choose a reason for hiding this comment

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

Why not

Suggested change
final searchOrder = (List.generate(max, (i) => i)..shuffle(Random(42))).take(1000).toList();
final random = Random(42);
final searchOrder = List.generate(1000, i => random.nextInt(max));

I know perf doesn't matter that much, it's just a little easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem is it can create duplicates. That is what I avoid above

Copy link
Member

Choose a reason for hiding this comment

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

It can, but do we care?

try {
// skip timestamp for now, as timestamps are not indexed properly it seems
if (fieldName != 'timestamp') {
expect(indexedTime, lessThan(notIndexedTime)); // indexed should be faster
Copy link
Member

Choose a reason for hiding this comment

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

I think we should be stricter here - something like expect(indexedTime, lessThan(0.5 * notIndexedTime))

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 used to have

expect(indexedTime * 10, lessThan(notIndexedTime));

but I was asked to loosen it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.. but maybe 2 would be fine 😄

@nielsenko nielsenko merged commit 88a1729 into master Nov 3, 2022
@nielsenko nielsenko deleted the kn/indexes branch November 3, 2022 12:22
@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.

Timestamp indexes (are there other indexes we miss?)
5 participants