Tuple merger incompatibility fixes needed for tarantool/tarantool#8147 #390

CuriousGeorgiy · 2023-10-18T13:51:20Z

This patchset brings further tuple merger incomptability fixes (see also d18ad41).

Tests
Changelog
Documentation

DifferentialOrange · 2023-10-18T14:34:09Z

.github/workflows/reusable_test.yml

@@ -37,6 +37,9 @@ jobs:
      - name: Install metrics


Have you considered removing external merger install at all?

No, this wasn't considered in discussion with @Totktonada. I guess the external tuple merger exists for compatibility reasons so that crud can work with older Tarantool versions which do not have an embedded tuple merger. But I don't think that manually forcing the installation of the external tuple key definition and tuple merger modules is correct.

Right now I am having problems understanding how to remove the external tuple meger manually for go-tarantool and tarantool-python integration workflows, so I would really like to go this way (opting out of forcefully installing the external tuple merger), but I guess it would require patching CI with older Tarantool versions.

/cc @Totktonada

Shouldn't unlocking version dependency (aka install latest tuple-merger and tuple-keydef) solve the issue?

AFAIC, the external tuple merger is currently outdated (correct me @Totktonada), and it would require a separate effort to update it.

Isn't built-in one is preferred over external one on require nonetheless? It's not an override rock

I guess not, otherwise, why would we need to explicitly Remove external merger if needed in some workflow runs?

crud/.github/workflows/test_on_push.yaml

Lines 105 to 107 in 588c215

- name: Remove external merger if needed

if: ${{ matrix.remove-merger }}

run: rm .rocks/lib/tarantool/tuple/merger.so

But this is another way we could go (i.e., make the embedded merger preferrable).

AFAIC, the external tuple merger is currently outdated (correct me @Totktonada)

It is on track with the built-in merger. There are problems in CI, though.

I think that it is good to decouple @CuriousGeorgiy's task from code health effort in crud.

I also think that if reducing dependencies in the integration testing workflow helps to run crud's testing in tarantool's testing (and others), it is OK to do.

My thoughts regarding the code health activities are in #365.

Feel free to engage me into a voice discussion if needed.

Added a commit that drops external tuple merger and tuple keydef modules installation, as we discussed.

crud/select/merger.lua

.github/workflows/reusable_test.yml

DifferentialOrange

It still isn't enough for tests to pass:
https://github.com/tarantool/crud/actions/runs/6572650221/job/17854170027?pr=392

1) select.backend:"vshard".engine:"memtx".test_select_safety_too_low_limit
...t/github/tarantool/crud/test/integration/select_test.lua:164: expected: nil, actual: {
    class_name = "SelectError",
    err = "...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.scan_value to nil (table expected, got cdata)",
    file = "...ment/github/tarantool/crud/crud/common/sharding/init.lua",
    line = 183,
    stack = "stack traceback:\
\9...ment/github/tarantool/crud/crud/common/sharding/init.lua:183: in function <...ment/github/tarantool/crud/crud/common/sharding/init.lua:168>\
\9[C]: in function 'xpcall'\
\9.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:145: in function <.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:139>\
\9[C]: in function 'pcall'\
\9...gy/Development/github/tarantool/crud/crud/stats/init.lua:411: in function <...gy/Development/github/tarantool/crud/crud/stats/init.lua:402>\
\9[C]: at 0x557849c87a70",
    str = "SelectError: ...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.scan_value to nil (table expected, got cdata)",
}
stack traceback:
	...t/github/tarantool/crud/test/integration/select_test.lua:164: in function 'select.backend:"vshard".engine:"memtx".test_select_safety_too_low_limit'
	...
	[C]: in function 'xpcall'
2) select.backend:"vshard".engine:"memtx".test_select_safety_max_negative_limit
...t/github/tarantool/crud/test/integration/select_test.lua:164: expected: nil, actual: {
    class_name = "SelectError",
    err = "...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.scan_value to nil (table expected, got cdata)",
    file = "...ment/github/tarantool/crud/crud/common/sharding/init.lua",
    line = 183,
    stack = "stack traceback:\
\9...ment/github/tarantool/crud/crud/common/sharding/init.lua:183: in function <...ment/github/tarantool/crud/crud/common/sharding/init.lua:168>\
\9[C]: in function 'xpcall'\
\9.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:145: in function <.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:139>\
\9[C]: in function 'pcall'\
\9...gy/Development/github/tarantool/crud/crud/stats/init.lua:411: in function <...gy/Development/github/tarantool/crud/crud/stats/init.lua:402>\
\9[C]: at 0x557849c87a70",
    str = "SelectError: ...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.scan_value to nil (table expected, got cdata)",
}
stack traceback:
	...t/github/tarantool/crud/test/integration/select_test.lua:164: in function 'select.backend:"vshard".engine:"memtx".test_select_safety_max_negative_limit'
	...
	[C]: in function 'xpcall'
3) select.backend:"vshard".engine:"memtx".test_negative_first
...t/github/tarantool/crud/test/integration/select_test.lua:311: expected: nil, actual: {
    class_name = "SelectError",
    err = "...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.scan_value to nil (table expected, got cdata)",
    file = "...ment/github/tarantool/crud/crud/common/sharding/init.lua",
    line = 183,
    stack = "stack traceback:\
\9...ment/github/tarantool/crud/crud/common/sharding/init.lua:183: in function <...ment/github/tarantool/crud/crud/common/sharding/init.lua:168>\
\9[C]: in function 'xpcall'\
\9.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:145: in function <.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:139>\
\9[C]: in function 'pcall'\
\9...gy/Development/github/tarantool/crud/crud/stats/init.lua:411: in function <...gy/Development/github/tarantool/crud/crud/stats/init.lua:402>\
\9[C]: at 0x557849c87a70",
    str = "SelectError: ...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.scan_value to nil (table expected, got cdata)",
}
stack traceback:
	...t/github/tarantool/crud/test/integration/select_test.lua:311: in function 'select.backend:"vshard".engine:"memtx".test_negative_first'
	...
	[C]: in function 'xpcall'
4) select.backend:"vshard".engine:"memtx".test_negative_first_with_batch_size
...t/github/tarantool/crud/test/integration/select_test.lua:506: expected: nil, actual: {
    class_name = "SelectError",
    err = "...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.scan_value to nil (table expected, got cdata)",
    file = "...ment/github/tarantool/crud/crud/common/sharding/init.lua",
    line = 183,
    stack = "stack traceback:\
\9...ment/github/tarantool/crud/crud/common/sharding/init.lua:183: in function <...ment/github/tarantool/crud/crud/common/sharding/init.lua:168>\
\9[C]: in function 'xpcall'\
\9.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:145: in function <.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:139>\
\9[C]: in function 'pcall'\
\9...gy/Development/github/tarantool/crud/crud/stats/init.lua:411: in function <...gy/Development/github/tarantool/crud/crud/stats/init.lua:402>\
\9[C]: at 0x557849c87a70",
    str = "SelectError: ...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.scan_value to nil (table expected, got cdata)",
}
stack traceback:
	...t/github/tarantool/crud/test/integration/select_test.lua:506: in function 'select.backend:"vshard".engine:"memtx".test_negative_first_with_batch_size'
	...
	[C]: in function 'xpcall'
5) select.backend:"vshard".engine:"memtx".test_composite_primary_index
...t/github/tarantool/crud/test/integration/select_test.lua:1018: expected: nil, actual: {
    class_name = "SelectError",
    err = "...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.after_tuple to nil (?table expected, got cdata)",
    file = "...ment/github/tarantool/crud/crud/common/sharding/init.lua",
    line = 183,
    stack = "stack traceback:\
\9...ment/github/tarantool/crud/crud/common/sharding/init.lua:183: in function <...ment/github/tarantool/crud/crud/common/sharding/init.lua:168>\
\9[C]: in function 'xpcall'\
\9.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:145: in function <.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:139>\
\9[C]: in function 'pcall'\
\9...gy/Development/github/tarantool/crud/crud/stats/init.lua:411: in function <...gy/Development/github/tarantool/crud/crud/stats/init.lua:402>\
\9[C]: at 0x557849c87a70",
    str = "SelectError: ...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.after_tuple to nil (?table expected, got cdata)",
}
stack traceback:
	...t/github/tarantool/crud/test/integration/select_test.lua:1018: in function 'select.backend:"vshard".engine:"memtx".test_composite_primary_index'
	...
	[C]: in function 'xpcall'
6) select.backend:"vshard".engine:"memtx".test_multipart_primary_index
...t/github/tarantool/crud/test/integration/select_test.lua:1173: expected: nil, actual: {
    class_name = "SelectError",
    err = "...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.after_tuple to nil (?table expected, got cdata)",
    file = "...ment/github/tarantool/crud/crud/common/sharding/init.lua",
    line = 183,
    stack = "stack traceback:\
\9...ment/github/tarantool/crud/crud/common/sharding/init.lua:183: in function <...ment/github/tarantool/crud/crud/common/sharding/init.lua:168>\
\9[C]: in function 'xpcall'\
\9.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:145: in function <.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:139>\
\9[C]: in function 'pcall'\
\9...gy/Development/github/tarantool/crud/crud/stats/init.lua:411: in function <...gy/Development/github/tarantool/crud/crud/stats/init.lua:402>\
\9[C]: at 0x557849c87a70",
    str = "SelectError: ...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.after_tuple to nil (?table expected, got cdata)",
}
stack traceback:
	...t/github/tarantool/crud/test/integration/select_test.lua:1173: in function 'select.backend:"vshard".engine:"memtx".test_multipart_primary_index'
	...
	[C]: in function 'xpcall'
7) select.backend:"vshard".engine:"memtx".test_jsonpath_index_field_pagination
...t/github/tarantool/crud/test/integration/select_test.lua:1718: expected: nil, actual: {
    class_name = "SelectError",
    err = "...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.after_tuple to nil (?table expected, got cdata)",
    file = "...ment/github/tarantool/crud/crud/common/sharding/init.lua",
    line = 183,
    stack = "stack traceback:\
\9...ment/github/tarantool/crud/crud/common/sharding/init.lua:183: in function <...ment/github/tarantool/crud/crud/common/sharding/init.lua:168>\
\9[C]: in function 'xpcall'\
\9.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:145: in function <.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:139>\
\9[C]: in function 'pcall'\
\9...gy/Development/github/tarantool/crud/crud/stats/init.lua:411: in function <...gy/Development/github/tarantool/crud/crud/stats/init.lua:402>\
\9[C]: at 0x557849c87a70",
    str = "SelectError: ...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.after_tuple to nil (?table expected, got cdata)",
}
stack traceback:
	...t/github/tarantool/crud/test/integration/select_test.lua:1718: in function 'select.backend:"vshard".engine:"memtx".test_jsonpath_index_field_pagination'
	...
	[C]: in function 'xpcall'
8) select.backend:"vshard".engine:"vinyl".test_select_safety_too_low_limit
...t/github/tarantool/crud/test/integration/select_test.lua:164: expected: nil, actual: {
    class_name = "SelectError",
    err = "...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.scan_value to nil (table expected, got cdata)",
    file = "...ment/github/tarantool/crud/crud/common/sharding/init.lua",
    line = 183,
    stack = "stack traceback:\
\9...ment/github/tarantool/crud/crud/common/sharding/init.lua:183: in function <...ment/github/tarantool/crud/crud/common/sharding/init.lua:168>\
\9[C]: in function 'xpcall'\
\9.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:145: in function <.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:139>\
\9[C]: in function 'pcall'\
\9...gy/Development/github/tarantool/crud/crud/stats/init.lua:411: in function <...gy/Development/github/tarantool/crud/crud/stats/init.lua:402>\
\9[C]: at 0x5633e25b4a70",
    str = "SelectError: ...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.scan_value to nil (table expected, got cdata)",
}
stack traceback:
	...t/github/tarantool/crud/test/integration/select_test.lua:164: in function 'select.backend:"vshard".engine:"vinyl".test_select_safety_too_low_limit'
	...
	[C]: in function 'xpcall'
9) select.backend:"vshard".engine:"vinyl".test_select_safety_max_negative_limit
...t/github/tarantool/crud/test/integration/select_test.lua:164: expected: nil, actual: {
    class_name = "SelectError",
    err = "...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.scan_value to nil (table expected, got cdata)",
    file = "...ment/github/tarantool/crud/crud/common/sharding/init.lua",
    line = 183,
    stack = "stack traceback:\
\9...ment/github/tarantool/crud/crud/common/sharding/init.lua:183: in function <...ment/github/tarantool/crud/crud/common/sharding/init.lua:168>\
\9[C]: in function 'xpcall'\
\9.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:145: in function <.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:139>\
\9[C]: in function 'pcall'\
\9...gy/Development/github/tarantool/crud/crud/stats/init.lua:411: in function <...gy/Development/github/tarantool/crud/crud/stats/init.lua:402>\
\9[C]: at 0x5633e25b4a70",
    str = "SelectError: ...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.scan_value to nil (table expected, got cdata)",
}
stack traceback:
	...t/github/tarantool/crud/test/integration/select_test.lua:164: in function 'select.backend:"vshard".engine:"vinyl".test_select_safety_max_negative_limit'
	...
	[C]: in function 'xpcall'
10) select.backend:"vshard".engine:"vinyl".test_negative_first
...t/github/tarantool/crud/test/integration/select_test.lua:311: expected: nil, actual: {
    class_name = "SelectError",
    err = "...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.scan_value to nil (table expected, got cdata)",
    file = "...ment/github/tarantool/crud/crud/common/sharding/init.lua",
    line = 183,
    stack = "stack traceback:\
\9...ment/github/tarantool/crud/crud/common/sharding/init.lua:183: in function <...ment/github/tarantool/crud/crud/common/sharding/init.lua:168>\
\9[C]: in function 'xpcall'\
\9.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:145: in function <.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:139>\
\9[C]: in function 'pcall'\
\9...gy/Development/github/tarantool/crud/crud/stats/init.lua:411: in function <...gy/Development/github/tarantool/crud/crud/stats/init.lua:402>\
\9[C]: at 0x5633e25b4a70",
    str = "SelectError: ...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.scan_value to nil (table expected, got cdata)",
}
stack traceback:
	...t/github/tarantool/crud/test/integration/select_test.lua:311: in function 'select.backend:"vshard".engine:"vinyl".test_negative_first'
	...
	[C]: in function 'xpcall'
11) select.backend:"vshard".engine:"vinyl".test_negative_first_with_batch_size
...t/github/tarantool/crud/test/integration/select_test.lua:506: expected: nil, actual: {
    class_name = "SelectError",
    err = "...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.scan_value to nil (table expected, got cdata)",
    file = "...ment/github/tarantool/crud/crud/common/sharding/init.lua",
    line = 183,
    stack = "stack traceback:\
\9...ment/github/tarantool/crud/crud/common/sharding/init.lua:183: in function <...ment/github/tarantool/crud/crud/common/sharding/init.lua:168>\
\9[C]: in function 'xpcall'\
\9.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:145: in function <.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:139>\
\9[C]: in function 'pcall'\
\9...gy/Development/github/tarantool/crud/crud/stats/init.lua:411: in function <...gy/Development/github/tarantool/crud/crud/stats/init.lua:402>\
\9[C]: at 0x5633e25b4a70",
    str = "SelectError: ...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.scan_value to nil (table expected, got cdata)",
}
stack traceback:
	...t/github/tarantool/crud/test/integration/select_test.lua:506: in function 'select.backend:"vshard".engine:"vinyl".test_negative_first_with_batch_size'
	...
	[C]: in function 'xpcall'
12) select.backend:"vshard".engine:"vinyl".test_composite_primary_index
...t/github/tarantool/crud/test/integration/select_test.lua:1018: expected: nil, actual: {
    class_name = "SelectError",
    err = "...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.after_tuple to nil (?table expected, got cdata)",
    file = "...ment/github/tarantool/crud/crud/common/sharding/init.lua",
    line = 183,
    stack = "stack traceback:\
\9...ment/github/tarantool/crud/crud/common/sharding/init.lua:183: in function <...ment/github/tarantool/crud/crud/common/sharding/init.lua:168>\
\9[C]: in function 'xpcall'\
\9.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:145: in function <.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:139>\
\9[C]: in function 'pcall'\
\9...gy/Development/github/tarantool/crud/crud/stats/init.lua:411: in function <...gy/Development/github/tarantool/crud/crud/stats/init.lua:402>\
\9[C]: at 0x5633e25b4a70",
    str = "SelectError: ...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.after_tuple to nil (?table expected, got cdata)",
}
stack traceback:
	...t/github/tarantool/crud/test/integration/select_test.lua:1018: in function 'select.backend:"vshard".engine:"vinyl".test_composite_primary_index'
	...
	[C]: in function 'xpcall'
13) select.backend:"vshard".engine:"vinyl".test_multipart_primary_index
...t/github/tarantool/crud/test/integration/select_test.lua:1173: expected: nil, actual: {
    class_name = "SelectError",
    err = "...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.after_tuple to nil (?table expected, got cdata)",
    file = "...ment/github/tarantool/crud/crud/common/sharding/init.lua",
    line = 183,
    stack = "stack traceback:\
\9...ment/github/tarantool/crud/crud/common/sharding/init.lua:183: in function <...ment/github/tarantool/crud/crud/common/sharding/init.lua:168>\
\9[C]: in function 'xpcall'\
\9.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:145: in function <.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:139>\
\9[C]: in function 'pcall'\
\9...gy/Development/github/tarantool/crud/crud/stats/init.lua:411: in function <...gy/Development/github/tarantool/crud/crud/stats/init.lua:402>\
\9[C]: at 0x5633e25b4a70",
    str = "SelectError: ...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.after_tuple to nil (?table expected, got cdata)",
}
stack traceback:
	...t/github/tarantool/crud/test/integration/select_test.lua:1173: in function 'select.backend:"vshard".engine:"vinyl".test_multipart_primary_index'
	...
	[C]: in function 'xpcall'
14) select.backend:"vshard".engine:"vinyl".test_jsonpath_index_field_pagination
...t/github/tarantool/crud/test/integration/select_test.lua:1718: expected: nil, actual: {
    class_name = "SelectError",
    err = "...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.after_tuple to nil (?table expected, got cdata)",
    file = "...ment/github/tarantool/crud/crud/common/sharding/init.lua",
    line = 183,
    stack = "stack traceback:\
\9...ment/github/tarantool/crud/crud/common/sharding/init.lua:183: in function <...ment/github/tarantool/crud/crud/common/sharding/init.lua:168>\
\9[C]: in function 'xpcall'\
\9.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:145: in function <.../github/tarantool/crud/.rocks/share/tarantool/errors.lua:139>\
\9[C]: in function 'pcall'\
\9...gy/Development/github/tarantool/crud/crud/stats/init.lua:411: in function <...gy/Development/github/tarantool/crud/crud/stats/init.lua:402>\
\9[C]: at 0x5633e25b4a70",
    str = "SelectError: ...georgy/Development/github/tarantool/crud/crud/select.lua:33: bad argument opts.after_tuple to nil (?table expected, got cdata)",
}
stack traceback:
	...t/github/tarantool/crud/test/integration/select_test.lua:1718: in function 'select.backend:"vshard".engine:"vinyl".test_jsonpath_index_field_pagination'
	...
	[C]: in function 'xpcall'
Ran 1597 tests in 264.879 seconds, 1583 succeeded, 14 failed, 2249 skipped

CuriousGeorgiy · 2023-10-21T10:55:17Z

crud/select.lua

+        if box.tuple.is(opts.after_tuple) then
+            opts.after_tuple = opts.after_tuple:totable()
+        end
+    end


Is it okay that we modify the options table here, or should I make a copy of it?

It is fine to modify options table here. First, it's already a copy since it has been transferred over netbox. Second, this opts is some internal table that was created by crud itself, so it fine to change it (even if we don't).

The other question is do we really have to totable this one? If the only reason is checks (dev_checks), then you can simply use ?table|cdata here.

I think we also need to cover external API here:

crud/crud/select/compat/select_old.lua

Line 421 in 588c215

after = '?table',

crud/crud/select/compat/select_old.lua

Line 228 in 588c215

after = '?table',

(compat/select.lua already covers this, compat/select_old.lua not cover it yet.)

I think we also need to cover external API here:

AFAIC, crud's embedded tuple merger can only be used with Tarantool 1.10, which will never have tuples over IPROTO, so we don't need to bother about it.

The other question is do we really have to totable this one? If the only reason is checks (dev_checks), then you can simply use ?table|cdata here.

I also considered this, but I preferred conversion, because we can't check the cdata specific type. Though if this function is only internal, I guess it would be fine (but then why do we do the type checking there in the first place?) — leaving this is up to you.

I am also not sure that passing the cdata down won't require patching a lot of other places downstream. If you insist, I can investigate that.

because we can't check the cdata specific type

It's fine since we don't check table specific type either (we expect an array, but may receive a map).

I am also not sure that passing the cdata down won't require patching a lot of other places downstream.

Try to patch only dev_checks first. If there are any more changes required that are more than two minutes of work, let's stick to your implementation.

Looks like it works and we can proceed with your solution.

CuriousGeorgiy · 2023-10-21T11:16:01Z

@DifferentialOrange testing against tarantool/tarantool#8630 now passes.

Totktonada

No objections from my side.

DifferentialOrange · 2023-10-23T07:53:17Z

crud/select.lua

+        if box.tuple.is(opts.after_tuple) then
+            opts.after_tuple = opts.after_tuple:totable()
+        end
+    end


It is fine to modify options table here. First, it's already a copy since it has been transferred over netbox. Second, this opts is some internal table that was created by crud itself, so it fine to change it (even if we don't).

The other question is do we really have to totable this one? If the only reason is checks (dev_checks), then you can simply use ?table|cdata here.

DifferentialOrange · 2023-10-23T08:10:27Z

crud/select.lua

+        if box.tuple.is(opts.after_tuple) then
+            opts.after_tuple = opts.after_tuple:totable()
+        end
+    end


I think we also need to cover external API here:

crud/crud/select/compat/select_old.lua

Line 421 in 588c215

after = '?table',

crud/crud/select/compat/select_old.lua

Line 228 in 588c215

after = '?table',

(compat/select.lua already covers this, compat/select_old.lua not cover it yet.)

crud/select/merger.lua

CHANGELOG.md

.github/workflows/test_on_push.yaml

Crud requires a tuple merger to work, and it currently works with three kinds of mergers: an embedded Tarantool one, an external one, and a crud embedded one. The latter implementation is suboptimal but does not depend on any additional symbols. The external merger replicates Tarantool's embedded merger, and serves two purposes: 1. Bring tuple merging to Tarantool 1.10 which does not have an embedded merger. 2. Allows for versioning the merger module independently from Tarantool. Because of potential symbol dependency conflicts, the external merger is installed in a hacky way, which didn't actually work when crud is built as part of another rockspec. Since Tarantool 1.10 can work with the merger embedded to crud, there is no need in pulling an external merger, so let's drop this dependency. Instead, add a CI matrix case with external merger installation to test against it. All written above also applies to the tuple keydef module. Needed for tarantool/tarantool#8147

In scope of tarantool/tarantool#8147, a new context-dependent extension for box tuples, `MP_TUPLE`, is introduced, and the IPROTO data response is extended with a new `IPROTO_TUPLE_FORMATS` field with tuples formats necessary for decoding `MP_TUPLE` sent in the `IPROTO_DATA` field. If the tuple merger's buffer source is used, raw MsgPack is received (see also d18ad41). We expect the response to only contain an `IPROTO_DATA` field, so the occurrence of a new `IPROTO_TUPLE_FORMATS` field breaks this assumption. The `IPROTO_DATA` field is still decoded correctly, but the input buffer's position now points to the `IPROTO_TUPLE_FORMATS` field instead of the end of the response. Instead of handling the issue described above we decided to opt for a simpler solution, namely, using the `skip_header` option of the `net.box` connection's API, which returns only the `IPROTO_DATA` field's value, which is what we are looking for. This option is was introduced in tarantool/tarantool@1aaf637870 (2.2.0 release), but since the issue described above can only occur with the latest Tarantool version, we can safely fallback to manually decoding the `IPROTO_DATA` header on old Tarantool versions. Needed for tarantool/tarantool#8147

In scope of tarantool/tarantool#8147, box tuples returned from remote procedure calls our now encoded as a new extension, `MP_TUPLE`, and, as, the results of such calls are now decoded as box tuples, i.e., cdata. For compatibility with this feature, we need to allow tuple fields of select options, which are returned from a remote procedure calls, to be cdata in option type checks. Needed for tarantool/tarantool#8147

DifferentialOrange

Thank you for your patch!

Overview This release introduces various fixes for future Tarantool 3 patches related to tuples over network. Changes * Dropped external tuple merger and tuple keydef modules installation from the package build (#390). Fixes * Compatibility with Tarantool 3.0 binary protocol change (#390). * Compatibility with Tarantool 3.0 tuple objects (#390).

CuriousGeorgiy requested review from DifferentialOrange and Totktonada October 18, 2023 13:51

CuriousGeorgiy assigned Totktonada and DifferentialOrange Oct 18, 2023

CuriousGeorgiy force-pushed the iproto-tuple-formats-fixes branch from c166792 to afd0918 Compare October 18, 2023 13:53

DifferentialOrange reviewed Oct 18, 2023

View reviewed changes

Totktonada reviewed Oct 19, 2023

View reviewed changes

crud/select/merger.lua Outdated Show resolved Hide resolved

CuriousGeorgiy force-pushed the iproto-tuple-formats-fixes branch from afd0918 to b7de7b2 Compare October 19, 2023 06:16

DifferentialOrange reviewed Oct 19, 2023

View reviewed changes

crud/select/merger.lua Show resolved Hide resolved

DifferentialOrange reviewed Oct 19, 2023

View reviewed changes

.github/workflows/reusable_test.yml Outdated Show resolved Hide resolved

DifferentialOrange requested changes Oct 19, 2023

View reviewed changes

CuriousGeorgiy force-pushed the iproto-tuple-formats-fixes branch from b7de7b2 to a66c005 Compare October 21, 2023 10:54

CuriousGeorgiy commented Oct 21, 2023

View reviewed changes

CuriousGeorgiy requested review from DifferentialOrange and Totktonada October 21, 2023 11:13

CuriousGeorgiy force-pushed the iproto-tuple-formats-fixes branch from a66c005 to e6f24f2 Compare October 21, 2023 11:30

Totktonada approved these changes Oct 22, 2023

View reviewed changes

Totktonada removed their assignment Oct 22, 2023

DifferentialOrange reviewed Oct 23, 2023

View reviewed changes

CuriousGeorgiy added 2 commits October 23, 2023 13:13

CuriousGeorgiy force-pushed the iproto-tuple-formats-fixes branch from e6f24f2 to 7dd8b82 Compare October 23, 2023 10:13

CuriousGeorgiy requested a review from DifferentialOrange October 23, 2023 10:14

CuriousGeorgiy force-pushed the iproto-tuple-formats-fixes branch from 2c9b292 to 861c90c Compare October 23, 2023 13:02

CuriousGeorgiy force-pushed the iproto-tuple-formats-fixes branch 2 times, most recently from 5596c9d to 61a871a Compare October 23, 2023 14:16

DifferentialOrange approved these changes Oct 23, 2023

View reviewed changes

DifferentialOrange merged commit 241cdee into tarantool:master Oct 23, 2023
21 checks passed

DifferentialOrange mentioned this pull request Oct 23, 2023

release 1.4.1 #394

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tuple merger incompatibility fixes needed for tarantool/tarantool#8147 #390

Tuple merger incompatibility fixes needed for tarantool/tarantool#8147 #390

CuriousGeorgiy commented Oct 18, 2023

DifferentialOrange Oct 18, 2023

CuriousGeorgiy Oct 18, 2023 •

edited

Loading

DifferentialOrange Oct 18, 2023 •

edited

Loading

CuriousGeorgiy Oct 18, 2023

DifferentialOrange Oct 18, 2023

CuriousGeorgiy Oct 18, 2023 •

edited

Loading

Totktonada Oct 19, 2023

Totktonada Oct 19, 2023

CuriousGeorgiy Oct 21, 2023

DifferentialOrange left a comment •

edited

Loading

CuriousGeorgiy Oct 21, 2023

DifferentialOrange Oct 23, 2023

DifferentialOrange Oct 23, 2023

CuriousGeorgiy Oct 23, 2023

DifferentialOrange Oct 23, 2023

CuriousGeorgiy Oct 23, 2023 •

edited

Loading

DifferentialOrange Oct 23, 2023

CuriousGeorgiy Oct 23, 2023

CuriousGeorgiy commented Oct 21, 2023 •

edited

Loading

Totktonada left a comment

DifferentialOrange Oct 23, 2023

DifferentialOrange Oct 23, 2023

DifferentialOrange left a comment

	- name: Remove external merger if needed
	if: ${{ matrix.remove-merger }}
	run: rm .rocks/lib/tarantool/tuple/merger.so

Tuple merger incompatibility fixes needed for tarantool/tarantool#8147 #390

Tuple merger incompatibility fixes needed for tarantool/tarantool#8147 #390

Conversation

CuriousGeorgiy commented Oct 18, 2023

Choose a reason for hiding this comment

CuriousGeorgiy Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

DifferentialOrange Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CuriousGeorgiy Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DifferentialOrange left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CuriousGeorgiy Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CuriousGeorgiy commented Oct 21, 2023 • edited Loading

Totktonada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DifferentialOrange left a comment

Choose a reason for hiding this comment

CuriousGeorgiy Oct 18, 2023 •

edited

Loading

DifferentialOrange Oct 18, 2023 •

edited

Loading

CuriousGeorgiy Oct 18, 2023 •

edited

Loading

DifferentialOrange left a comment •

edited

Loading

CuriousGeorgiy Oct 23, 2023 •

edited

Loading

CuriousGeorgiy commented Oct 21, 2023 •

edited

Loading