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

Improve UX for complex/nested objects #186

Closed
coffeemug opened this issue Dec 26, 2012 · 19 comments
Closed

Improve UX for complex/nested objects #186

coffeemug opened this issue Dec 26, 2012 · 19 comments
Milestone

Comments

@coffeemug
Copy link
Contributor

As pointed out by @saurik here https://groups.google.com/forum/?fromgroups=#!topic/rethinkdb/sSB-rtXQPsk, the user experience of accessing nested objects isn't great. Consider the following cases:

  • Modifying a sub-sub attribute or a set of sub-sub attributes on different levels in a document
  • Modifying an element in an array

Additionally, while not discussed in the post, there is a question of querying based on sub-sub elements or groups of sub-sub-elements on different levels.

@jdoliner proposed map on objects, a general purpose merge operation based on the object map, a set of sugar operations based on that, and ability to pass a custom merge function to update. All of these are great and necessary. We'll also need to think through potential defaults for update, array modification ops, querying complex objects, and possibly more sugar functions for dealing with complex hierarchies (such as flatten).

@al3xandru
Copy link
Contributor

This question came up again in #irc. We should definitely bump this for 1.6

@neumino
Copy link
Member

neumino commented May 1, 2013

I agree with @al3xandru.
It's a frequent question we get, and the solution is really not pretty.

@jdoliner
Copy link
Contributor

jdoliner commented May 1, 2013

So here's a proposal for a simple syntax we can add that isn't terribly hard and I think covers a lot of the issues people were running in to.

obj.set('attr1', 'attr2', 'attr3', value)

The imperative version of this would be:

obj['attr1']['attr2']['attr3'] = value
return obj

People would probably prefer the version with the imperative look but I think we're going to have trouble fitting that nicely in to our query language. There's a lot of questions that come up there.

@al3xandru
Copy link
Contributor

I was planning to suggest the 2nd solution to which I arrived after starting with the 1st. I was also thinking that this might be complicated implementation wise, but while I think I could get used to the 1st, I find it a bit more less legible.

How about:

obj.set({'attr1': {'attr2': {'attr3': value}}})

It's longer, but the structure/depth is more obvious.

update: isn't the above already possible with obj.merge?

@jdoliner
Copy link
Contributor

jdoliner commented May 1, 2013

@al3xandru I believe that's the semantics of merge. I'm not sure if it's recursive right now but I think it should be.

@neumino
Copy link
Member

neumino commented May 1, 2013

What @al3xandru proposed is close to merge.
However, merge would overwrite the attr1 and attr2 field. Adding a flag (to not overwrite other field) in merge could be sufficient.

I like the first syntax of set that @jdoliner wrote.

@al3xandru
Copy link
Contributor

  1. I think merge should be recursive.
  2. We could use the same approach/syntax for set (and be consistent)

@jdoliner
Copy link
Contributor

jdoliner commented May 1, 2013

So I just checked merge isn't recursive right now which means it does overwrite the values of the sub attributes as @neumino said.

Also @al3xandru I have no idea what you mean by this: "We could use the same approach/syntax for set (and be consistent)" What exactly is going to be the same here?

@al3xandru
Copy link
Contributor

Not sure I'm using the right words to describe what I mean, but having merge and set use the same syntax would make sense to me.

doc.merge(
  { 'attr1': {
      'attr2': {
          'attr3': value }
      }
  })

overwrites attr1.

doc.set(
  { 'attr1': {
      'attr2': {
          'attr3': value }
     }
  })

sets attr3 in attr2 of attr1. This syntax would also allow setting multiple values at once, which I don't think it would be possible with the other 2 proposals.

@jdoliner
Copy link
Contributor

jdoliner commented May 1, 2013

So I'm still a bit confused. It seems like you've basically just flipped the meanings that @neumino and I were talking about in that you've made merge non recursive and set be recursive.

@al3xandru
Copy link
Contributor

I'm not sure I understand what I've flipped or not :-). I was just trying to come up with a proposal for set that:

  1. keeps it similar to merge (consistency)
  2. allows setting multiple values (concise)

@AtnNn
Copy link
Member

AtnNn commented May 1, 2013

I don't like recursive merge. With a recursive merge, the result of this query:

r.expr({ 'key': { 'foo': bar } }).merge({ 'key' : value })

Will not always be { 'key' : value }. It could be { 'key' : {'foor':bar}.merge(value) }.

That is very surprising.

@coffeemug
Copy link
Contributor Author

I really liked @jdoliner's original proposal (outlined on ggroups) -- support map over objects, and then implement various forms of merge on top of that. This way merge could take a flag (or a function or something) on whether it should be deep or shallow, and by default it would remain the same as it is now.

@mlucy
Copy link
Member

mlucy commented May 2, 2013

I think there's actually a reasonable argument for defaulting to a deep merge. That's what I think I'd usually want.

@AtnNn
Copy link
Member

AtnNn commented May 2, 2013

@coffeemug can you paste that proposal here?

@mlucy deep merge is very useful but I would never expect it to be the default.

@coffeemug
Copy link
Contributor Author

Here's the original complaint:

Are there any examples of manipulating documents at a deeper level than the first level of keys? The experience I've had so far attempting to do mutations has been quite painful in comparison to, say, MongoDB.

As an example, say I have a document:

{ a:{b:{x:0, y:1}, c:2}, 
  q:['x', 'y']} 

If I want to set .a.c to d it seems I have to do .update({a: r.row('a').merge({c: "d"})})? Is that really the easiest way to build that query? What if I want to change .a.b.x? What I've come up with so far is .update({a: r.row('a').merge({b: r.row('a')('b').merge({x:'d'})})}). That works, but I'm somewhat concerned that as I get deeper it will get exceedingly more complex.

If I want to change .q[0] I was hoping to be able to do .update({q: r.row('q').merge({'0': 'd'})}) (as in JavaScript a string key is legal to be used on an array, and so I figured a naive implementation of merge would "just work" like that), but I got an error "Data must be an object". I /was/ able to do this: .update({q: r.expr([['d'], r.row('q').skip(1)]).concatMap(function(value) { return value; })}).run(), but I burst out laughing when I did ;P.

(Is this kind of update even an advisable use case for RethinkDB? AFAIK, this is considered a normal use case for MongoDB and is easily supported1 by using syntax like .update(..., {$set:{'q.0':'d'}, ...). That said, I don't really use MongoDB, so I'm not certain if doing these little partial updates is horribly inefficient; they seem to claim it is a key feature on their blog, though2.)

@coffeemug
Copy link
Contributor Author

Here's @jdoliner's proposal (which doesn't deal with arrays and might have a nicer user-facing API but is a good base for this):

I think that the basic primitive operation we want here is a generic Merge which would have the following semantics
Merge takes 2 Objects and a merging function so its type signature looks like:

Merge :: Object -> Object -> (JSON -> JSON -> JSON) -> Object

Merge(A, B, f) = M where:
for a key k

M[k] == Nothing (if k is not in A and k is not in B)
M[k] == A[k] (if k is in A but not in B)
M[k] == B[k] (" "  "   "  B  "    "    " A)
M[k] == f(A[k], B[k]) (if k is in both A and B)

With this we could then implement a specific type of Merge which called itself on sub objects (call it RMerge) so in your example of updating a.c to d you could say.

#here update is taking a merging scheme as well the document to merge in
.update({a : { c : d }}, RMerge)

Notice that our current merge can also be implemented in terms of this with the function (f a b -> b). And we'd probably want to add some sugar so you didn't need to specify the common cases in update.

Also as a note all of this is easily implemented in terms of map over objects which we've talked about before as well.

@neumino
Copy link
Member

neumino commented May 19, 2013

Bumping since someone asked this again on IRC.

Relevant gist: https://gist.github.com/neumino/5607840

@coffeemug
Copy link
Contributor Author

This issue has now been superceded by #895, #343, and #872. Closing.

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

6 participants