[tarantool-patches] Re: [PATCH v1 1/1] sql: improve vdbe_field_ref fetcher

[Vladislav Shpilevoy]

Hi! Thanks for the fixes.

See 2 comments below.

> diff --git a/test/sql/misc.test.lua b/test/sql/misc.test.lua
> index fdc19f3ac..afbb2d7f8 100644
> --- a/test/sql/misc.test.lua
> +++ b/test/sql/misc.test.lua
> @@ -35,3 +35,25 @@ s = box.schema.space.create('s',{format=format, temporary=true})
>  i = s:create_index('i')
>  box.execute('select * from "s"')
>  s:drop()
> +
> +--
> +-- gh-4267: Full power of vdbe_field_ref
> +-- The test consists of sequential reads of multiple fields:
> +-- the first is indexed while the other are not indexed.
> +-- In skope of this issue an optimization that makes second and

1. 'scope'.

> +-- third lookups faster were introduced. Test that they works
> +-- correctly.

2. This is not an explanation, but rather a bit of water. With
the same explanation you could test fields 1 and 2. Please, explain
why exactly these numbers.

Also you somewhy dropped the previous test on the border case about
a field < 64 and >= 64 in one request. Why? The test below does not
cover it.

> +--
> +format = {}
> +t = {}
> +test_run:cmd("setopt delimiter ';'")
> +for i = 1, 70 do
> +        format[i] = {name = 'FIELD'..i, type = 'unsigned'}
> +        t[i] = i
> +end
> +test_run:cmd("setopt delimiter ''");

For such a small piece of code you could use '\' instead of
setopt delimiter. IMO \ looks much better. Up to you.

> +s = box.schema.create_space('TEST', {format = format})
> +pk = s:create_index('pk', {parts = {66}})
> +s:insert(t)
> +box.execute('SELECT field66, field68, field70 FROM test')
> +box.space.TEST:drop()
>