Re: [Tarantool-patches] [PATCH 4/6] sql: extend result set with nullability

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

You added a leading whitespace to the commit title.

Here is a strange example:

    box.cfg{}
    s = box.schema.create_space('TEST', {
        format = {{'ID', 'unsigned'}, {'COL', 'unsigned', is_nullable = true}}
    })
    pk = s:create_index('pk', {parts = {{'ID'}, {'COL', is_nullable = false}}})

In that example I defined a 'false-nullable' column. It
is nullable in the space format, but is not in the
index format. The strictest rule wins, so it is not nullable
after all. That can be seen in struct tuple_format. SQL says,
that it is nullable. But this is not a real problem. Perhaps
this might be even right. However the next example certainly
misses something:

    box.execute('SELECT * FROM test')
    ---
    - metadata:
      - name: ID
        type: unsigned
      - name: COL
        type: unsigned
      rows: []
    ...

    box.execute('SELECT col FROM test')
    ---
    - metadata:
      - type: unsigned
        name: COL
        is_nullable: true
      rows: []
    ...

As you can see, nullable depends on whether I select a
column explicitly or not. Perhaps the same happens to
autoincrement meta, I didn't check.

See 5 comments below.

On 27/11/2019 13:15, Nikita Pettik wrote:
> If member of result set is (solely) column identifier, then metadata
> will contain its corresponding field nullability as boolean property.
> Note that indicating nullability for other expressions (like x + 1)
> may make sense but it requires derived nullability calculation which in
> turn seems to be overkill (at least in scope of current patch).
> 
> Part of #4407
> ---
>  src/box/execute.c                                |  18 +-
>  src/box/iproto_constants.h                       |   1 +
>  src/box/lua/execute.c                            |   5 +
>  src/box/lua/net_box.c                            |   7 +-
>  src/box/sql/select.c                             |   6 +-
>  src/box/sql/sqlInt.h                             |   3 +
>  src/box/sql/vdbe.h                               |   3 +
>  src/box/sql/vdbeInt.h                            |   5 +
>  src/box/sql/vdbeapi.c                            |   7 +
>  src/box/sql/vdbeaux.c                            |   7 +
>  test/box/sql-update-with-nested-select.result    |   5 +-
>  test/sql/boolean.result                          | 425 ++++++++++++++---------
>  test/sql/check-clear-ephemeral.result            |   5 +-
>  test/sql/collation.result                        |  74 ++--
>  test/sql/gh-3199-no-mem-leaks.result             | 120 ++++---
>  test/sql/gh2141-delete-trigger-drop-table.result |  20 +-
>  test/sql/gh2251-multiple-update.result           |  10 +-
>  test/sql/iproto.result                           | 105 +++---
>  test/sql/misc.result                             |  25 +-
>  test/sql/on-conflict.result                      |  20 +-
>  test/sql/persistency.result                      | 190 ++++++----
>  test/sql/row-count.result                        |  25 +-
>  test/sql/sql-debug.result                        |  15 +-
>  test/sql/transition.result                       | 190 ++++++----
>  test/sql/types.result                            | 105 +++---
>  test/sql/update-with-nested-select.result        |   5 +-
>  26 files changed, 862 insertions(+), 539 deletions(-)
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 20bfd0957..98812ae1e 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -267,8 +267,10 @@ error:
>  	region_truncate(region, svp);
>  	return -1;
>  }
> +
>  static size_t
> -metadata_map_sizeof(const char *name, const char *type, const char *coll)
> +metadata_map_sizeof(const char *name, const char *type, const char *coll,
> +		    int nullable)

1. Please, rename to 'is_nullable'. Here and in other places.

>  {
>  	uint32_t members_count = 2;
>  	size_t map_size = 0;
> @@ -277,6 +279,11 @@ metadata_map_sizeof(const char *name, const char *type, const char *coll)
>  		map_size += mp_sizeof_uint(IPROTO_FIELD_COLL);
>  		map_size += mp_sizeof_str(strlen(coll));
>  	}
> +	if (nullable != -1) {
> +		members_count++;
> +		map_size += mp_sizeof_uint(IPROTO_FIELD_NULLABLE);

2. This too. IPROTO_FIELD_IS_NULLABLE.

> +		map_size += mp_sizeof_bool(nullable);
> +	}
>  	map_size += mp_sizeof_uint(IPROTO_FIELD_NAME);
>  	map_size += mp_sizeof_uint(IPROTO_FIELD_TYPE);
>  	map_size += mp_sizeof_str(strlen(name));
> @@ -334,6 +342,10 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
>  			pos = mp_encode_uint(pos, IPROTO_FIELD_COLL);
>  			pos = mp_encode_str(pos, coll, strlen(coll));
>  		}
> +		if (nullable != -1) {
> +			pos = mp_encode_uint(pos, IPROTO_FIELD_NULLABLE);
> +			pos = mp_encode_bool(pos, nullable);
> +		}

3. We send autoincrement only in case it is true.
Then how about sending nullability only in case it
is true? To save a bit of the network.

>  	}
>  	return 0;
>  }
> diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
> index fa9c029a2..030c25531 100644
> --- a/src/box/iproto_constants.h
> +++ b/src/box/iproto_constants.h
> @@ -132,6 +132,7 @@ enum iproto_metadata_key {
>  	IPROTO_FIELD_NAME = 0,
>  	IPROTO_FIELD_TYPE = 1,
>  	IPROTO_FIELD_COLL = 2,
> +	IPROTO_FIELD_NULLABLE = 3

4. Please, add a comma after 3 so as not to
change this line in the next commit.

>  };
>  
>  enum iproto_ballot_key {
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 66e8c1274..b772bcead 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1836,7 +1837,10 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
>  			} else {
>  				vdbe_set_metadata_col_name(v, i, zCol);
>  			}
> -
> +			bool is_nullable = space_def->fields[iCol].is_nullable;
> +			if (p->op == TK_COLUMN)
> +				vdbe_set_metadata_col_nullability(v, i,
> +								  is_nullable);

5. Please, wrap into {}.