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

Primary key support for int and string columns #868

Merged
merged 32 commits into from
Sep 9, 2014
Merged

Primary key support for int and string columns #868

merged 32 commits into from
Sep 9, 2014

Conversation

alazier
Copy link
Contributor

@alazier alazier commented Sep 3, 2014

Also support for upsert for objects which have primary keys.

Missing:

  • proper core support for indexing (both strings and ints)
  • migration support and verification

@kspangsege
Copy link
Contributor

I'm working on improved primary key support in code. Hope to have something
out in a couple of days.

On Thu, Sep 4, 2014 at 1:52 AM, Ari Lazier notifications@github.com wrote:

Also support for upsert for objects which have primary keys.

Missing:

  • proper core support for indexing (both strings and ints)
  • migration support and verification

You can merge this Pull Request by running

git pull https://github.com/realm/realm-cocoa al-primary

Or view, comment on, or merge it at:

#868
Commit Summary

  • primary keys and upsert for int and string
  • Merge branch 'master' into al-primary
  • don't allow primary properties to be changed, tests
  • fixes for upsert

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#868.

@kspangsege
Copy link
Contributor

code -> core

On Thu, Sep 4, 2014 at 2:28 AM, Kristian Spangsege ks@tightdb.com wrote:

I'm working on improved primary key support in code. Hope to have
something out in a couple of days.

On Thu, Sep 4, 2014 at 1:52 AM, Ari Lazier notifications@github.com
wrote:

Also support for upsert for objects which have primary keys.

Missing:

  • proper core support for indexing (both strings and ints)
  • migration support and verification

You can merge this Pull Request by running

git pull https://github.com/realm/realm-cocoa al-primary

Or view, comment on, or merge it at:

#868
Commit Summary

  • primary keys and upsert for int and string
  • Merge branch 'master' into al-primary
  • don't allow primary properties to be changed, tests
  • fixes for upsert

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#868.

@alazier
Copy link
Contributor Author

alazier commented Sep 4, 2014

@kspangsege Sweet. Right now the only core functionality from core we are using is calling find_first for string and int to ensure uniqueness. Once we have a more efficient way to do this it shouldn't be difficult to switch over. We are not using any indexing as there is an issue with string indexing and int indexing isn't yet implemented.

@kspangsege
Copy link
Contributor

Can you say more about the issue with string indexes, or point to a related
Asana task?

On Thu, Sep 4, 2014 at 2:42 AM, Ari Lazier notifications@github.com wrote:

@kspangsege https://github.com/kspangsege Sweet. Right now the only
core functionality from core we are using is calling find_first for string
and int to ensure uniqueness. Once we have a more efficient way to do this
it shouldn't be difficult to switch over. We are not using any indexing as
there is an issue with string indexing and int indexing isn't yet
implemented.


Reply to this email directly or view it on GitHub
#868 (comment).

@alazier
Copy link
Contributor Author

alazier commented Sep 4, 2014

I'm not sure there is a specify task. This is the same problem that we discovered when running Group::Verify at every transaction boundary. Here is the callstack of where that fails in the tests:

> frame #3: 0x00000001066e09ca Realm`tightdb::util::terminate(message=<unavailable>, file=0x0000000106bd9b96, line=2231) + 186 at terminate.cpp:14
  * frame #4: 0x00000001066fa91f Realm`tightdb::Array::Verify(this=0x0000000100112c60) const + 959 at array.cpp:2231
    frame #5: 0x0000000106a25579 Realm`tightdb::StringIndex::Verify(this=0x0000000100112c40) const + 25 at index_string.cpp:720
    frame #6: 0x00000001069373e7 Realm`tightdb::AdaptiveStringColumn::Verify(this=0x0000000100112b20) const + 247 at column_string.cpp:1428
    frame #7: 0x000000010691d2f4 Realm`tightdb::ColumnBase::Verify(this=0x0000000100112b20, (null)=0x0000000107812200, (null)=0) const + 36 at column.cpp:232
    frame #8: 0x00000001069376f8 Realm`tightdb::AdaptiveStringColumn::Verify(this=0x0000000100112b20, table=0x0000000107812200, col_ndx=0) const + 88 at column_string.cpp:1436
    frame #9: 0x00000001069a0a1e Realm`tightdb::Table::Verify(this=0x0000000107812200) const + 1838 at table.cpp:4664
    frame #10: 0x00000001069f2b1a Realm`tightdb::Group::Verify(this=0x0000000102808200) const + 762 at group.cpp:1603

When verify is not used, we get a crash when creating an empty row on a table with a string index.

@kspangsege
Copy link
Contributor

Ok, thanks for the info. It seems we need more core-side testing of the
string index.

On Thu, Sep 4, 2014 at 3:16 AM, Ari Lazier notifications@github.com wrote:

I'm not sure there is a specify task. This is the same problem that we
discovered when running Group::Verify at every transaction boundary. Here
is the callstack of where that fails in the tests:

frame #3: 0x00000001066e09ca Realm`tightdb::util::terminate(message=, file=0x0000000106bd9b96, line=2231) + 186 at terminate.cpp:14

When verify is not used, we get a crash when creating an empty row on a
table with a string index.


Reply to this email directly or view it on GitHub
#868 (comment).

@jpsim
Copy link
Contributor

jpsim commented Sep 4, 2014

Also, the following test:

[[RLMRealm defaultRealm] transactionWithBlock:^{
    // IndexedObject has `NSString *name` and `NSInteger age` properties, where `name` is indexed
    [IndexedObject createInDefaultRealmWithObject:@[@"nameValue", @0]];
}];

yields the following error message:

index_string.cpp:233: Assertion failed: m_array->size() == old_offsets.size()+1

@@ -252,6 +299,16 @@
*/
+ (RLMArray *)objectsInRealm:(RLMRealm *)realm withPredicate:(NSPredicate *)predicate;

/**
Returns TRUE if another RLMObject points to the same object in a RLMRealm. For RLMObject types
Copy link
Member

Choose a reason for hiding this comment

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

Returns YES

[propArray addObject:prop];
if ([primaryKey isEqualToString:propertyName]) {
//attr = attr | RLMPropertyAttributeIndexed;
schema.primaryKeyProperty = [RLMProperty propertyForObjectProperty:props[i] attributes:attr];
Copy link
Member

Choose a reason for hiding this comment

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

[RLMProperty propertyForObjectProperty:props[i] attributes:attr] could be pulled out of the if.

@eni9889
Copy link

eni9889 commented Sep 5, 2014

@alazier I'm running into the following exception:

*** Terminating app due to uncaught exception 'RLMException', reason: 'Setting unique property '90a3fa74-0fff-4d10-b982-1fc154472165' with existing value 'id''
*** First throw call stack:
(0x187cc7100 0x1941d01fc 0x10035381c 0x100353148 0x10036de34 0x1003a1e54 0x100353cc8 0x1003531cc 0x10036edcc 0x10036550c 0x1000b90fc 0x1947a8014 0x1947a7fd4 0x1947af2b8 0x1947af4fc 0x19493d6bc 0x19493d54c)
libc++abi.dylib: terminating with uncaught exception of type NSException
(lldb) bt
* thread #4: tid = 0x7c82d, 0x00000001948be58c libsystem_kernel.dylib`__pthread_kill + 8, queue = 'com.apple.root.default-priority', stop reason = signal SIGABRT
    frame #0: 0x00000001948be58c libsystem_kernel.dylib`__pthread_kill + 8
    frame #1: 0x000000019494116c libsystem_pthread.dylib`pthread_kill + 104
    frame #2: 0x0000000194852808 libsystem_c.dylib`abort + 112
    frame #3: 0x0000000193a78994 libc++abi.dylib`abort_message + 88
    frame #4: 0x0000000193a95c2c libc++abi.dylib`default_terminate_handler() + 300
    frame #5: 0x00000001941d04d4 libobjc.A.dylib`_objc_terminate() + 128
    frame #6: 0x0000000193a93168 libc++abi.dylib`std::__terminate(void (*)()) + 16
    frame #7: 0x0000000193a92a80 libc++abi.dylib`__cxa_throw + 136
    frame #8: 0x00000001941d0318 libobjc.A.dylib`objc_exception_throw + 344
    frame #9: 0x000000010035381c app`RLMSetValueUnique(obj=0x00000001780bec00, colIndex=0, propName=0x0000000170434400, val=0x0000000178869b40) + 304 at RLMAccessor.mm:115
    frame #10: 0x0000000100353148 app`RLMDynamicSet(obj=0x00000001780bec00, prop=0x0000000170c62040, val=0x0000000178869b40, enforceUnique=true) + 728 at RLMAccessor.mm:601
    frame #11: 0x000000010036de34 app`RLMAddObjectToRealm(object=0x00000001780bec00, realm=0x00000001782a0420, update=false) + 2008 at RLMObjectStore.mm:311
    frame #12: 0x00000001003a1e54 app`-[RLMRealm addObject:](self=0x00000001782a0420, _cmd=0x000000018b3603c2, object=0x00000001780bec00) + 56 at RLMRealm.mm:530
    frame #13: 0x0000000100353cc8 app`RLMSetValue(obj=0x00000001781319e0, colIndex=13, val=0x00000001780bec00) + 448 at RLMAccessor.mm:164
    frame #14: 0x00000001003531cc app`RLMDynamicSet(obj=0x00000001781319e0, prop=0x0000000170c61900, val=0x00000001780bec00, enforceUnique=false) + 860 at RLMAccessor.mm:614

This happens because I have the following json:

{
    id = "23a01308-fadc-4ffa-ac24-42dcbaf8a453";
    name = 'jon smith';
    address =     {
            city = "San Francisco ";
            id = "90a3fa74-0fff-4d10-b982-1fc154472165";
            state = CA;
            zipcode = 94107;
        };
}

Does createOrUpdateInDefaultRealmWithObject not get called on nested objects?

@alazier
Copy link
Contributor Author

alazier commented Sep 5, 2014

@eni9889 you are correct that upserting nested objects doesn't yet work properly. You can probably work around this by upserting nested objects before the parent - for the data you provided it would be something like:

Person *person = [[Person alloc] initWithObject:parsedJSON];
[realm addOrUpdateObject:person.address];
[realm addOrUpdateObject:person];

Once we get this fixed you can delete the calls to addOrUpdateObject: for the nested objects.

alazier and others added 9 commits September 4, 2014 22:48
And fix some issues with int sizes in obj-c and some Swift initialization stuff
in the process.

There's three main changes here:

1. Most of the int property stuff now operates on s, i and q separately, so
that the properties can actually act as if they were the correct type. This
fixes 64-bit properties sometimes getting truncated to 32 bits, 32-bit
properties sometimes *not* getting truncated to 32 bites, and 16-bit properties
not working at all.

2. RLMObjectSchema is now more similar for obj-c and Swift types, with only the
Swift-specific parts handled by RLMSwiftSupport. This was done to solve the
problem of that the actual size of the int property was being lost (with
everything being collapsed to a single size), but also fixes things like the
object type of array properties not being validated, and makes primary keys
work for Swift.

3. There's now a dummy implementation of RLMSwiftSupport for Xcode 5 so that
the only `#if REALM_SWIFT` checks are for which header to include, rather than
making some of the actual logic a mess.
Add full support for sized Ints in Swift
@alazier alazier changed the title [WIP] Primary key support for int and string columns Primary key support for int and string columns Sep 8, 2014
@@ -572,43 +604,56 @@ void RLMDynamicValidatedSet(RLMObject *obj, NSString *propName, id val) {
userInfo:@{@"Property name:" : propName ?: @"nil",
@"Value": val ? [val description] : @"nil"}];
}
RLMDynamicSet(obj, (RLMProperty *)prop, val);
RLMDynamicSet(obj, (RLMProperty *)prop, val, prop.isPrimary, false);
Copy link
Member

Choose a reason for hiding this comment

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

Why the cast here?

@tgoyne
Copy link
Member

tgoyne commented Sep 9, 2014

Is there a test for trying to upsert a type without a primary key? I think that's the only thing missing.

@tgoyne
Copy link
Member

tgoyne commented Sep 9, 2014

I think this is basically ready for merge. I've been developing on top of it to avoid conflicts and haven't noticed any regressions.

@alazier
Copy link
Contributor Author

alazier commented Sep 9, 2014

Any other comments?


### Bugfixes

* Realm change notifications when beginning a write transaction are now sent
after updating rather than before, to match refresh.
* `-isEqual:` now uses the default `NSObject` implementation unless a primary key
is specified for an RLMObject. In the case a primary key is specified `-isEqual:` calls
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rephrase to

When a primary key is specified, -isEqual: calls -isEqualToObject: and a corresponding implementation for -hash is also implemented.

Copy link

Choose a reason for hiding this comment

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

Sorry if this was mentioned but will nested object support be included in
this merge?

Sent from my iPhone

On Sep 9, 2014, at 3:44 PM, JP Simard notifications@github.com wrote:

In CHANGELOG.md:

Bugfixes

  • Realm change notifications when beginning a write transaction are now sent
    after updating rather than before, to match refresh.
    +* -isEqual: now uses the default NSObject implementation unless a primary key
    • is specified for an RLMObject. In the case a primary key is specified -isEqual: calls

Can you rephrase to

When a primary key is specified, -isEqual: calls -isEqualToObject: and a
corresponding implementation for -hash is also implemented.


Reply to this email directly or view it on GitHub
https://github.com/realm/realm-cocoa/pull/868/files#r17333242.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eni9889 yes there is support for nested objects in this 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.

@eni9889 this commit should have fixed the issues you were having: 7d33383

Please let us know if this is not the case.

alazier added a commit that referenced this pull request Sep 9, 2014
Primary key support for int and string columns
@alazier alazier merged commit 57c28fb into master Sep 9, 2014
@bagvendt
Copy link
Contributor

👍 over 9000

@tgoyne tgoyne deleted the al-primary branch October 25, 2014 01:12
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
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.

6 participants