[patches] Re: [PATCH v2 1/2] format: add collation to filed_def and tuple_field

v.shpilevoy at tarantool.org v.shpilevoy at tarantool.org
Sat Mar 10 14:40:55 MSK 2018


See below 2 minor remarks. In the totally the patch seems to be ok. If you will decide to fix these remarks, then send just their diff to the patches, please.

> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index 6b9c76e6d..5ed08cc52 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -346,6 +346,13 @@ function update_format(format)
>                     end
>                 elseif k == 2 and not given.type and not given.name then
>                     field.type = v
> +                elseif k == 'collation' then
> +                    local coll = box.space._collation.index.name:get{v}
> +                    if not coll then
> +                        box.error(box.error.ILLEGAL_PARAMS,
> +                            "format[" .. i .. "]: collation was not found by name '" .. v .. "'")
> +                    end
> +                    field[k] = coll[1]

1. Since tuple field names were introduced, you can use coll.id <http://coll.id/> instead of coll[1]. But it is up to you, field names usage is not forced now.

> diff --git a/test/box/ddl.result b/test/box/ddl.result
> index 0eef37992..335f55500 100644
> --- a/test/box/ddl.result
> +++ b/test/box/ddl.result
> @@ -514,3 +514,49 @@ s:format()[3].custom_field
> s:drop()

2. How about a test, when you insert directly into _space with identifier of non-existing collation? To check that "collation was not found by ID" works. It is just advice too.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180310/97810608/attachment.html>


More information about the Tarantool-patches mailing list