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

Duplicate Proto Records #16

Closed
lprhodes opened this issue May 29, 2013 · 20 comments
Closed

Duplicate Proto Records #16

lprhodes opened this issue May 29, 2013 · 20 comments
Assignees
Milestone

Comments

@lprhodes
Copy link

Hi,

uniquelyAddNewProtoRecord:toExistingResponseGroups: ironically adds multiple instances of the same object to the protoRecords array.

This means when protoRecordWithRecordResponseObject:entity:existingResponseGroups is called, both the uniquelyAddNewProtoRecord and completeRelationshipProtoRecordMappingToProtoRecord methods end up adding the same object to the array.

@cnstoll
Copy link
Contributor

cnstoll commented Jun 4, 2013

Please see my latest pull request. Is that what you meant?

Can you also clarify what you mean by duplicates? Are these duplicates in some array somewhere? Is that array actually being used for anything? Could it be deleted or bypassed?

When I read duplicate I freak out a bit because I think you mean duplicates in the database. I have a lot of tests that are telling me that's not happening. So I just want to make sure we're saying the same thing about what's going on here.

Thanks,

  • Conrad

@lprhodes
Copy link
Author

lprhodes commented Jun 4, 2013

The protoRecords is the array that had duplicates in - it was the same pointer for both objects. With my amend to use a prototypeDictionary it was easily fixed with the following code:

  • (void)addProtoRecord:(MMRecordProtoRecord *)protoRecord {
    if (![self.prototypeDictionary objectForKey:protoRecord.primaryKeyValue]) {
    [self.protoRecords addObject:protoRecord];
    }
    }

As a side note - I did manage to get duplicates in my database at one point - with the issue that caused me to think the result and failure block was being called…multiple calls of the exact same request were occurring simultaneously. Now I'm not calling it multiple times so it won't occur any more.

@lprhodes
Copy link
Author

lprhodes commented Jun 4, 2013

Forget my side note - one key was an NSString and one was an NSNumber so comparePrimaryKeyValues wasn't working. It could be worth putting an NSAssert in there - my mogenerator is broken for some reason.

@cnstoll
Copy link
Contributor

cnstoll commented Jun 4, 2013

Hrm, interesting. Yeah, that's a weird one. Part of me wants to just always normalize that value to a string and do string comparison...but that may not be correct in all cases.

So is the issue above fixed in your pull request?

@lprhodes
Copy link
Author

lprhodes commented Jun 4, 2013

String comparison would be slower though surely? I fixed it in the following commit:
lprhodes@74362d1

@cnstoll
Copy link
Contributor

cnstoll commented Jun 4, 2013

Yeah it's not ideal that's for sure. What you're describing though sounds like a problem in your data set. These are duplicates for one type of entity right? But that the objects coming back in JSON have either a number or a string value? I think if this happened for us I would a) want to make sure the data is consistent, and b) maybe add a check in the marshaler/representation layer to make sure.

@lprhodes
Copy link
Author

lprhodes commented Jun 4, 2013

The duplicate was in fact for a single record I was importing (describing a user)…there was one being imported and two proto records. The key was the username.

@cnstoll
Copy link
Contributor

cnstoll commented Jun 15, 2013

This was actually a pretty major deal for performance. I didn't realize it until the other day in the dub dub labs. This is fixed by my WWDC Performance pull request. Assigning to myself and to the 1.1.0 milestone.

@ghost ghost assigned cnstoll Jun 15, 2013
@cnstoll
Copy link
Contributor

cnstoll commented Jun 30, 2013

@urbanappetite do you mind checking the performance branch again to see if it's not saving records properly for you? I've tested it on all the example apps as well as my test suite and it's working fine on all of those.

@lprhodes
Copy link
Author

I'll give it a look tomorrow - Glastonbury festival this weekend. Thanks!

Sent from my iPhone

On 30 Jun 2013, at 03:55, Conrad Stoll notifications@github.com wrote:

@urbanappetite do you mind checking the performance branch again to see if it's not saving records properly for you? I've tested it on all the example apps as well as my test suite and it's working fine on all of those.


Reply to this email directly or view it on GitHub.

@cnstoll
Copy link
Contributor

cnstoll commented Jun 30, 2013

Wow, that looks awesome. I love Mumford and Sons!

I did fix a bug in this branch this morning that may have been causing the issue that was giving you trouble. Just try it again and let me know how it goes.

Thanks,

  • Conrad

@lprhodes
Copy link
Author

lprhodes commented Jul 1, 2013

Hey,

There's still an issue (the previous issue which was that it wasn't completing at all is fixed).

I have two entities that have a many to many relationship to one another. I've simplified the JSON below. The relationship is on "tableIdentifiers". Only one relationship is ever made between Table 1 and Table 2 by MMRecord rather than the 3 that should occur in the example below.

Table 1:
[
{
"tableIdentifiers" : [
"brand",
"metric",
"context"
],
"name" : "Some Name",
"key" : 5
}
]

Table 2:
[
{
"code" : "brand"
},
{
"code" : "context"
},
{
"code" : "metric"
}
]

@cnstoll
Copy link
Contributor

cnstoll commented Jul 1, 2013

Hey Luke,

Help me make sure I understand the example. You're saying that there are two entities, Table 1 and Table 2, but that a tableIdentifiers relationship on Table 1 should contain three Table 2 objects. Table 2 objects have a primary key called "code" and in this case will contain brand, metric, and context?

Did I get that all right?

That's actually very similar to one of the examples I am testing with. Let me do a bit more digging on my end.

Thanks,

  • Conrad

@lprhodes
Copy link
Author

lprhodes commented Jul 1, 2013

Exactly, I think the many-to-many relationship is likely to be the culprit given that relationships are still made.

@cnstoll
Copy link
Contributor

cnstoll commented Jul 1, 2013

Ok. Pretty sure I found the problem. I'll push up an update later that should fix that issue and you can take another look.

@cnstoll
Copy link
Contributor

cnstoll commented Jul 1, 2013

Ok, changes pushed. Let's try that one more time :)

@lprhodes
Copy link
Author

lprhodes commented Jul 1, 2013

That's fixed, thanks!

FYI - With the small test dataset I'm using I get 45 seconds import on the master branch, 5 seconds on the performance branch. I got it down to 10 seconds just with the dictionary suggestion - to halve the time again is brilliant thanks!

@cnstoll
Copy link
Contributor

cnstoll commented Jul 2, 2013

Awesome! All merged into Master now. I'm going to wait to rev the podspec for a few more days to see if we can't get deletion in there too. Thanks again!

@cnstoll cnstoll closed this as completed Jul 2, 2013
@lprhodes
Copy link
Author

lprhodes commented Jul 2, 2013

FYI - I've posted a radar requesting the ability for core data to batch insert rather than inserting pone record at a time. It may be worth posting a similar radar to help increase the requests weight.

@lprhodes
Copy link
Author

lprhodes commented Jul 2, 2013

Thanks for all your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants