Allow update absent nullable fields #5911

mary3000 · 2021-03-18T08:37:00Z

Update operations can't insert with gaps.
But there exist "virtual nulls" in tarantool -
i.e. fields that are specified in format and marked as is_nullable.
They are not actually stored in the database, and currently
tarantool refuses to do update ops with such nulls as "gaps".

But from the user side these nulls exist. So this patch changes
behaviour to such that update ops will fill virtual nulls with
the real ones.

Closes #3378

Gerold103 · 2021-03-18T10:03:02Z

Hi! I didn't review it in details yet, but I see you can simplify and speed up xrow_update_op_append_nulls() a lot. MP_NIL is a single byte, so you can insert a big rope batch in the end (no need to store the nils in individual rope nodes I think), and memset() it with the nil byte code.

mary3000 · 2021-03-22T20:45:43Z

Hi! I didn't review it in details yet, but I see you can simplify and speed up xrow_update_op_append_nulls() a lot. MP_NIL is a single byte, so you can insert a big rope batch in the end (no need to store the nils in individual rope nodes I think), and memset() it with the nil byte code.

Ok, fixed

Gerold103

Hi! Thanks for working on this!

Please, add a docbot request and a changelog file.

Gerold103 · 2021-03-22T21:57:24Z

src/box/xrow_update_array.c

@@ -310,6 +310,36 @@ xrow_update_array_store(struct xrow_update_field *field,
 	return out - out_begin;
 }

+/*


Out of functions we always use /** as comment starter. Lets conform.

Gerold103 · 2021-03-22T21:59:10Z

src/box/xrow_update_array.c

+ * Helper function that appends nils in the end so that op will insert
+ * without gaps
+ */
+int


Please, make the function static. Also, as you could see, for update field type the functions has xrow_update_array prefix and pass the field as a first argument (like this in C++). Please, try to keep up.

Gerold103 · 2021-03-22T22:03:21Z

src/box/xrow_update_array.c

+		xrow_update_alloc(rope->ctx, sizeof(*item));
+	if (item == NULL)
+		return -1;
+	uint32_t nil_size = mp_sizeof_nil();


Please, add an assertion that mp_sizeof_nil() == 1, and drop nil_size.

Gerold103 · 2021-03-22T22:04:02Z

src/box/xrow_update_array.c

+		return -1;
+	uint32_t nil_size = mp_sizeof_nil();
+	uint32_t byte_count = nil_size * nil_count;
+	char *pos = (char *) region_alloc(rope->ctx, byte_count);


Region_alloc does not set diag. You need to do it manually.

Gerold103 · 2021-03-22T22:08:45Z

src/box/xrow_update_array.c

+		return -1;
+	memset(pos, 0xc0, byte_count);
+	xrow_update_array_item_create(item, XUPDATE_NOP, pos, byte_count, 0);
+	if (xrow_update_rope_insert(rope, op->field_no, item, nil_count) != 0)


You can do return xrow_update_rope_insert() instead of the if. It might even activate the tail-call optimization.

Gerold103 · 2021-03-22T22:13:51Z

test/engine/upsert.result

 ...
 s:upsert({0}, {{'=', 42, 42}})
 ---
 ...
 s:select{}
 ---
- - [0, 1, 2]
+- - [0, 1, 2, null, null, null, null, null, null, null, null, null, null, null, null,


I think the purpose of the test with setting a value with an index out of range was to ensure it does not work. Now it works, so better just drop these upsert() with 42th field.

Gerold103 · 2021-03-22T22:15:15Z

test/engine/update.test.lua

+s:replace{1, 2}
+s:update({1}, {{'!', 4, 0}})
+s:update({1}, {{'#', 4, 1}})
+s:drop()


Please, add more tests with json paths. What if I use ! and = on a field whose parent's index is out of range? For example [5][1] = 100, where field [5] does not exist. The same with maps ([5].key).

Agree. Even behavior isn't considered to be expected (I mean like @Gerold103 suggest to do right now), test on the subj would be nice to see.

Gerold103

Hi! Thanks for working on this!

You still didn't add a changelog file and didn't add a docbot request. Please, do. See more comments below.

Gerold103 · 2021-03-23T22:20:53Z

src/box/xrow_update_array.c

+	if (item == NULL)
+		return -1;
+	assert(mp_sizeof_nil() == 1);
+	char *item_data = (char *) region_alloc(rope->ctx, nil_count);


In the new code for unary operators (including cast) we don't use whitespace after them.

Gerold103 · 2021-03-23T22:21:13Z

src/box/xrow_update_array.c

+	}
+	memset(item_data, 0xc0, nil_count);
+	xrow_update_array_item_create(item, XUPDATE_NOP, item_data,
+			       nil_count, 0);


Misaligned line wrap.

Gerold103 · 2021-03-23T22:37:13Z

test/engine/update.result

+...
+s:update({1}, {{'!', '[2][42]', 0}})
+---
+- error: Field ''[2][42]'' was not found in the tuple


That does not seem right. Field [2] exists and it is an array. It means there should no be a reason why it can't auto-fill the fields until [42]. Tuple's root array is not any different from the nested arrays. At least this was the idea behind the xrow update code.

Also I realized that probably # should not throw an error. Because if the feature's main purpose is to fix the 'virtual' fields assignment and = will work on them now, then # should work as well. But now it fails. For example:

t = box.tuple.new({1, 2, 3}) t:update({{'#', 5, 1}})

This fails. But if I replace it with = or ! - it works. That makes # and ! not symmetric. Maybe such deletion should become nop when out of range or a key wasn't found (if it deletes from a map). I am afraid you need to ask Mons about that. 2 questions in total:

Do we auto-fill the nested arrays or only the root array?

Should we make # nop when it is out of range? To be symmetric with the new behaviour of !.

I think we should auto-fill nested structures in every case, except type collision (update array field on map)

Agree with you, It's ok to update # also

What do you mean as every case? For instance, I have a tuple {1, 2, 3}. And I make update {{'=', '[5].key[3]', 100}}. Should it also auto-create the intermediate array and map so it would result into this: {1, 2, 3, nil, {key = {nil, nil, 100}}}? Or is this an error, and we only auto-fill arrays and never create new arrays and maps on the path?

@Mons, see above.

This is an interesting question.
By example from Perl (called autovivification) I know, that feature may lead to unexpected results, but in most cases, it is rather handy.
But since we give an option to update the deep field and give no handy option to update such a field in case of it's absence, I consider we should support auto-creation of missing structures and raise an error only in case of conflicting types (array vs map)

According to our last conversation with Mons and Alexander, we decided to make changes regarding auto-fill nested structures in a separate task

What about the example I showed above about filling a non-root array? It does not need creation of the nested structures, the array exists already, it is just is not in the root. And yet it does not work now.

This case will also be implemented in a separate task, according to our discussion. @Mons, @alyapunov i am right?

If you are not going to implement it now, it will work 50/50. For example, it will work if I will json-update some internal array using 2 operations instead of one. The second one would turn it from a bar into a rope in the code, and the auto-fill will work like in the root. But it won't work if the update operation is just one. Which looks broken and inconsistent, when result of an operation depends on presence of other independent operations. That brings the question why is such a half-done and half-working feature even needed.

Let's move discussion of the issue to the issue.

Korablev77 · 2021-04-09T11:47:13Z

changelogs/unreleased/allow-update-absent-nullable-fields.md

+
+*  Update operations can't insert with gaps. This patch changes
+   the behavior so that the update operation fills the missing
+   fields with zeros.


Missing (gh-3378) mark at the end

Korablev77 · 2021-04-09T13:25:26Z

test/engine/update.test.lua

+-- gh-3378: allow update absent nullable fields
+
+-- '!'
+s = box.schema.create_space('test')


Since this is engine test, let's check it for both memtx and vinyl (just pass engine to create_space method)

Korablev77 · 2021-04-09T13:26:32Z

test/engine/update.test.lua

+pk = s:create_index('pk')
+s:replace{1, 2}
+s:update({1}, {{'!', 4, 0}})
+s:drop()


You don't have to drop space, just use another tuple or delete it - it should be a bit faster.

Korablev77 · 2021-04-09T13:31:04Z

test/engine/update.result

+---
+- [1, 2, null]
+...
+s:update({1}, {{'+', 3, 0}})


Didn't get the purpose of the test: I believe the fact that update operation involving null and actual value is checked somewhere else.

Korablev77 · 2021-04-09T13:33:23Z

test/engine/update.result

+---
+- [1, 2, null, 0]
+...
+s:update({1}, {{'#', 4, 1}})


What about another one 'trim' operation (so that we verify that null is actually stored in the tuple - result should be [1, 2] and not [1, null])?

Gerold103

Thanks for the patch! In the commit message you duplicated the same paragraph. Please, don't. Just keep the docbot request only.

Here is the docbot request with my 2 comments inlined.

    Update operations can't insert with gaps. This patch changes the
    behavior so that the update operation fills the missing fields
    with zeros.

Firstly, the docbot request is not a 'patch'. Secondly, it fills the fields with nulls, no zeros.

    For example we create space s = box.schema.create_space('s'),
    then create index for this space pk = s:create_index('pk'), and then
    insert tuple in space s:insert{1, 2}. After all of this we try to update
    this tuple s:update({1}, {{'!', 5, 6}}), in previous version this operation
    fails with ER_NO_SUCH_FIELD_NO error, and after this patch it's finished
    with success and there is [1, 2, null, null, 6] tuple in space.

The code snippets must be wrapped into the ` signs. Otherwise some of the parts of the code might turn on a formatting rule and the text would turn into a broken mess.

Gerold103 · 2021-04-09T22:39:13Z

changelogs/unreleased/allow-update-absent-nullable-fields.md

+
+*  Update operations can't insert with gaps. This patch changes
+   the behavior so that the update operation fills the missing
+   fields with zeros (gh-3378).


Not with zeros. With nulls.

Gerold103 · 2021-04-09T22:41:56Z

src/box/xrow_update_array.c

@@ -393,8 +426,11 @@ xrow_update_op_do_array_delete(struct xrow_update_op *op,

 	struct xrow_update_rope *rope = field->array.rope;
 	uint32_t size = xrow_update_rope_size(rope);
-	if (xrow_update_op_adjust_field_no(op, size) != 0)
+	if (xrow_update_op_adjust_field_no(op, size) != 0) {


This is a separate behaviour change. Better do it in another commit with its own tests.

Gerold103 · 2021-04-09T22:43:45Z

test/engine/update.result

+--
+-- gh-3378: allow update absent nullable fields
+-- '!'
+s = box.schema.create_space('test', {engine = engine})


You don't really need spaces, indexes, or any other storage things. The updates work on tuples. So for non-formatted tuples you could simply use t = box.tuple.new(...) and t:update(...).

Yes, but this is is engine test, and @Korablev77 write (see his comment above) that i need to check both memtx and vinyl engines. So i need to create space with specified engine.

This is an engine test only because you made so. You could have a separate file, or use box/ folder. Talking of the "need to check both engines" - why? It is not related to engines anyhow as I demonstrated. But up to you.

Gerold103 · 2021-04-09T22:58:24Z

When I repeatedly said that this implementation leads to inconsistent behaviour, I meant this example:

t = box.tuple.new({1, 2, {11, 22}})
op1 = {'=', '[3][1]', 11}
op2 = {'=', '[3][4]', 44}

-- The operation does not do anything and "works".
t:update({op1})
---
- [1, 2, [11, 22]]
...

-- The operation does not work since the array is not in the root.
t:update({op2})
---
- error: Field ''[3][4]'' was not found in the tuple
...

-- But when I combine them - magic, both work.
t:update({op1, op2})
---
- [1, 2, [11, 22, null, 44]]
...

This is not because of a format, or nullable/not nullable. This is because a single update operation in the xrow update code is optimized as so called 'bar' update (see xrow_update_bar.c). And when there is a second operation touching the same field, it is expanded to a proper array/map. The first version of the patch didn't address that, and the current version also ignores it. The example above is not a feature, it is rather a bug. The behaviour in the example literally depends on whether I add a nop update operation. And I don't see what is so hard in fixing that by making the auto-fill with nils work properly. If you got a bar update, and you see you need nils, you just need to expand it to a proper array right away.

EvgenyMekhanik · 2021-04-12T09:28:30Z

When I repeatedly said that this implementation leads to inconsistent behaviour, I meant this example:
t = box.tuple.new({1, 2, {11, 22}})
op1 = {'=', '[3][1]', 11}
op2 = {'=', '[3][4]', 44}

-- The operation does not do anything and "works".
t:update({op1})
---
- [1, 2, [11, 22]]
...

-- The operation does not work since the array is not in the root.
t:update({op2})
---
- error: Field ''[3][4]'' was not found in the tuple
...

-- But when I combine them - magic, both work.
t:update({op1, op2})
---
- [1, 2, [11, 22, null, 44]]
...
This is not because of a format, or nullable/not nullable. This is because a single update operation in the xrow update code is optimized as so called 'bar' update (see xrow_update_bar.c). And when there is a second operation touching the same field, it is expanded to a proper array/map. The first version of the patch didn't address that, and the current version also ignores it. The example above is not a feature, it is rather a bug. The behaviour in the example literally depends on whether I add a nop update operation. And I don't see what is so hard in fixing that by making the auto-fill with nils work properly. If you got a bar update, and you see you need nils, you just need to expand it to a proper array right away.

Yes, but what we must do in this case for example:
t = box.tuple.new({1, 2, {11, 22}})
op = {'=', '[3][3][1]', 11}
create third field with array or check that first to field are not arrays and fail?
and if we have case ?
t = box.tuple.new({1, 2, {{11, 11}, {22, 22}}})
op = {'=', '[3][3][1]', 11}

Gerold103 · 2021-04-12T21:09:08Z

Both examples are invalid, because it was decided by Mons, that we should not create maps and arrays along the JSON path. The only thing you can do is to add nils to an array. It does not matter what are the types of the fields in array. Only field count matters.

Prepare this test for upcoming #3378 fix: bad upserts will become good, so we need another way to do them.

@TarantoolBot

Update operations could not insert with gaps. This patch changes the behavior so that the update operation fills the missing fields with nulls. Part of #3378 @TarantoolBot document Title: Allow update absent nullable fields Update operations could not insert with gaps. Changed the behavior so that the update operation fills the missing fields with nulls. For example we create space `s = box.schema.create_space('s')`, then create index for this space `pk = s:create_index('pk')`, and then insert tuple in space `s:insert{1, 2}`. After all of this we try to update this tuple `s:update({1}, {{'!', 5, 6}})`. In previous version this operation fails with ER_NO_SUCH_FIELD_NO error, and now it's finished with success and there is [1, 2, null, null, 6] tuple in space.

In previous patch update(insert) operation for absent nullable fields was allowed. This patch allows to update(delete) operation for absent nullable fileds. Closes #3378

Korablev77

I guess we should push patch-set to master as (since it solves the most of user's pain) is and consider what to do with maps/arrays in scope of next release. LGTM

alyapunov

LGTM.
Need to create follow-up.

Gerold103 · 2021-04-13T20:28:27Z

The patch still introduces inconsistent behaviour as I showed in the example above. So I can't vote it up. But you don't need my opinion here anyway, I see there are 2 other LGTMs overriding my complaint.

Gerold103 · 2021-04-14T09:13:44Z

Btw, this commit also introduces a bug similar to this one: #5105. In vinyl, if the operations in my inconsistency example are in separate upserts, the behaviour will depend on whether they were merged and applied, or applied separately. I will file a ticket later.

mary3000 requested review from alyapunov and Gerold103 March 18, 2021 09:07

mary3000 force-pushed the mary3000/gh-3378-absent-null-update branch from 1e1b3e1 to ad8f01e Compare March 22, 2021 20:44

Gerold103 reviewed Mar 22, 2021

View reviewed changes

mary3000 force-pushed the mary3000/gh-3378-absent-null-update branch from ad8f01e to 71f3a55 Compare March 23, 2021 16:07

mary3000 assigned Gerold103 Mar 23, 2021

Gerold103 reviewed Mar 23, 2021

View reviewed changes

EvgenyMekhanik force-pushed the mary3000/gh-3378-absent-null-update branch from 71f3a55 to ed7fab9 Compare April 6, 2021 15:49

EvgenyMekhanik self-assigned this Apr 6, 2021

This comment has been minimized.

Sign in to view

EvgenyMekhanik added the do not merge label Apr 6, 2021

EvgenyMekhanik force-pushed the mary3000/gh-3378-absent-null-update branch from 1bc369a to e821990 Compare April 7, 2021 08:03

EvgenyMekhanik removed the do not merge label Apr 7, 2021

Korablev77 reviewed Apr 9, 2021

View reviewed changes

EvgenyMekhanik force-pushed the mary3000/gh-3378-absent-null-update branch 2 times, most recently from 8f96b63 to 3fd2984 Compare April 9, 2021 16:44

Gerold103 reviewed Apr 9, 2021

View reviewed changes

test: change compact.test.lua bad upsert

b67b15d

Prepare this test for upcoming #3378 fix: bad upserts will become good, so we need another way to do them.

EvgenyMekhanik force-pushed the mary3000/gh-3378-absent-null-update branch from 3fd2984 to dee2ad1 Compare April 13, 2021 11:35

mary3000 and others added 2 commits April 13, 2021 15:20

update: allow update(delete) absent nullable fields

1fe1ce3

In previous patch update(insert) operation for absent nullable fields was allowed. This patch allows to update(delete) operation for absent nullable fileds. Closes #3378

EvgenyMekhanik force-pushed the mary3000/gh-3378-absent-null-update branch from dee2ad1 to 1fe1ce3 Compare April 13, 2021 12:20

Korablev77 approved these changes Apr 13, 2021

View reviewed changes

alyapunov approved these changes Apr 13, 2021

View reviewed changes

Korablev77 merged commit 6451411 into master Apr 14, 2021

Korablev77 deleted the mary3000/gh-3378-absent-null-update branch April 14, 2021 08:05

Allow update absent nullable fields #5911

Allow update absent nullable fields #5911

Uh oh!

Conversation

mary3000 commented Mar 18, 2021 • edited by EvgenyMekhanik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gerold103 commented Mar 18, 2021

Uh oh!

mary3000 commented Mar 22, 2021

Uh oh!

Gerold103 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gerold103 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gerold103 Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gerold103 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mary3000 commented Mar 18, 2021 •

edited by EvgenyMekhanik

Loading

Gerold103 Apr 8, 2021 •

edited

Loading