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

Proposal: Allow users to specify conflict resolution function for merge #873

Open
mlucy opened this issue May 22, 2013 · 14 comments
Open

Proposal: Allow users to specify conflict resolution function for merge #873

mlucy opened this issue May 22, 2013 · 14 comments

Comments

@mlucy
Copy link
Member

mlucy commented May 22, 2013

Users will be able to provide a function to merge like so:

r({:a => 1, :b => 2}).merge({:b => 3, :c => 4}) {|key, lval, rval|
  ...
}

Where for every conflict in the two objects, the block will be called with the key in question, the left value, and the right value. (In the example above, the block will be called once with the arguments 'b', 2, and 3.)

@neumino
Copy link
Member

neumino commented May 22, 2013

Is it an attempt to address #186 ?
If yes I think it's way too complicated and powerful for what 99% of the people want.

@neumino
Copy link
Member

neumino commented May 22, 2013

Forget my question, I didn't see #872.

@mlucy
Copy link
Member Author

mlucy commented Jan 18, 2015

I just answered a question on SO that would have benefited from this: http://stackoverflow.com/questions/27999748/count-the-different-variables-in-an-array-in-my-document/28007968#28007968 . @danielmewes, we should maybe think about scheduling this during the next ReQL discussion period if anyone has time for ReQL features.

@coffeemug
Copy link
Contributor

What would the user return from merge when they provide a block/lambda?

@mlucy
Copy link
Member Author

mlucy commented Apr 22, 2015

@coffeemug -- what do you mean?

@mlucy
Copy link
Member Author

mlucy commented Apr 22, 2015

I think that instead of a second argument, we should use a conflict optarg like the proposal in #3753 since it will be a lot clearer to someone reading the code.

@danielmewes danielmewes modified the milestones: 2.1, 2.1-polish Apr 22, 2015
@coffeemug
Copy link
Contributor

Never mind, my question was silly. You'd obviously expect them to return the value they want to keep.

@AtnNn
Copy link
Member

AtnNn commented Apr 22, 2015

merge already takes a variable number of arguments, any of which can be functions. The conflict resolution function would have to be a named argument.

How would this interact with recursive merge? I would expect that there would be no recursion if merge was given a conflict resolution function. r.merge({a: {b: 1}}, {a: {b: 2}}, :conflict f) would call f(:a, {b: 1}, {b: 2}) and not f(:b, 1, 2). The default value for the conflict resolution function would be r.merge itself, restoring the recursive behaviour. Short-cirtcuiting conflict resolution only for a given key would then be done by delegating back to merge, for example: lambda k, a, b: r.branch(k == "k", ..., r.merge(a, b)).

That raises the question of how to handle queries like r.merge({a: 1}, {a: r.literal()), :conflict f) or r.merge({a: {b: 2}, {a: r.literal({})}, :conflict f). Perhaps literal values could be treated like the value they wrap, but respond differently to a hypothetic r.literalp command.

Returning r.literal() from the conflict function could be allowed, thus removing the key just like in a regular merge.

@coffeemug
Copy link
Contributor

👍 for everything @AtnNn said above -- that's a great spec! (though I think r.literal handling would be quite tricky)

@mlucy
Copy link
Member Author

mlucy commented Apr 24, 2015

Ah, right, I forgot that merge can accept n arguments which can be either objects or functions. That actually throws a bit of a wrench into this, because the JS and Ruby drivers still use "an object as the last argument" to represent optional arguments, so we can't easily distinguish in the clients between merging an actual object with a single key conflict and specifying the conflict optarg.

We could potentially get around this in the server by having a rule that if the conflict optarg is specified and it resolves to a value X that isn't a function, we treat that the same as having an object {conflict: X} as an extra argument to merge. That's hacky but it would work I think.

@mlucy
Copy link
Member Author

mlucy commented Apr 24, 2015

I agree with @AtnNn that we should call the function right away rather than recursing. I'm not sure what we should do with r.literal; I think ignoring it except that returning r.literal deletes a key is a good choice. (We could also leave that out of the initial implementation if it turns out to be hard, I would guess it's pretty rare to want to do that.)

@danielmewes
Copy link
Member

While we generally seem to agree on this, there are two important questions left so I think we should reschedule this discussion for after 2.1.

We still need to figure out

  • how to handle r.literal exactly
  • how the conflict optarg can be passed into merge

@danielmewes danielmewes modified the milestones: subsequent, 2.1-polish Apr 28, 2015
@chacalle
Copy link

Has this been implemented or is there a similar way to accomplish this? Would be really useful for this use case.

@danielmewes
Copy link
Member

@chacalle No, we have not gotten to this yet.

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

No branches or pull requests

6 participants