[tarantool-patches] Re: [PATCH v4 3/4] box: introduce column_mask for ck constraint

[Vladislav Shpilevoy]

Thanks for the patch! See 3 comments below.

On 16/05/2019 16:56, Kirill Shcherbatov wrote:
> This patch introduces a smart bitmask of fields are mentioned in

1. Remove 'are'. 'mentioned' here works as adjective. The same in
other places. Please, find them by yourself.

> ck constraint. This allows to do not bind useless fields and
> do not parse more fields that is required.
> 
> Needed for #3691
> ---
>  src/box/ck_constraint.c  | 66 ++++++++++++++++++++++++++++++++++------
>  src/box/ck_constraint.h  |  6 ++++
>  src/box/sql.h            |  5 ++-
>  src/box/sql/build.c      |  2 +-
>  src/box/sql/resolve.c    | 19 +++++++++---
>  src/box/sql/sqlInt.h     |  6 ++++
>  test/sql/checks.result   | 59 +++++++++++++++++++++++++++++++++++
>  test/sql/checks.test.lua | 20 ++++++++++++
>  8 files changed, 167 insertions(+), 16 deletions(-)
> 
> diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c
> index 43798be76..4756f7970 100644
> --- a/src/box/ck_constraint.c
> +++ b/src/box/ck_constraint.c
> @@ -46,17 +46,21 @@ const char *ck_constraint_language_strs[] = {"SQL"};
>   * tree traversal.
>   * @param ck_constraint Check constraint object to update.
>   * @param space_def Space definition to use.
> + * @param column_mask[out] The "smart" column mask of fields are
> + *                         referenced by AST.
>   * @retval 0 on success.
>   * @retval -1 on error.
>   */
>  static int
>  ck_constraint_resolve_field_names(struct Expr *expr,
> -				  struct space_def *space_def)
> +				  struct space_def *space_def,
> +				  uint64_t *column_mask)
>  {
>  	struct Parse parser;
>  	sql_parser_create(&parser, sql_get(), default_flags);
>  	parser.parse_only = true;
> -	sql_resolve_self_reference(&parser, space_def, NC_IsCheck, expr, NULL);
> +	sql_resolve_self_reference(&parser, space_def, NC_IsCheck, expr, NULL,
> +				   column_mask);

2. Next to last argument 'expr_list' is always NULL. Please, drop it.

>  	int rc = parser.is_aborted ? -1 : 0;
>  	sql_parser_destroy(&parser);
>  	return rc;
> @@ -98,10 +105,24 @@ ck_constraint_program_compile(struct ck_constraint_def *ck_constraint_def,
>  	 * bind tuple fields there before execution.
>  	 */
>  	uint32_t field_count = space_def->field_count;
> +	/*
> +	 * Use column mask to prepare binding only for
> +	 * referenced fields.
> +	 */
>  	int bind_tuple_reg = sqlGetTempRange(&parser, field_count);
> -	for (uint32_t i = 0; i < field_count; i++) {
> +	struct bit_iterator it;
> +	bit_iterator_init(&it, &column_mask, sizeof(uint64_t), true);
> +	size_t used_fieldno = bit_iterator_next(&it);
> +	for (; used_fieldno < SIZE_MAX; used_fieldno = bit_iterator_next(&it)) {
>  		sqlVdbeAddOp2(v, OP_Variable, ++parser.nVar,
> -			      bind_tuple_reg + i);
> +			      bind_tuple_reg + used_fieldno);
> +	}
> +	if (column_mask_fieldno_is_set(column_mask, 63)) {

3. Please, do not hardcode that value. Add a one-line function to
column_mask.h for that. On other places too.

> +		used_fieldno = 64;
> +		for (; used_fieldno < (size_t)field_count; used_fieldno++) {
> +			sqlVdbeAddOp2(v, OP_Variable, ++parser.nVar,
> +				      bind_tuple_reg + used_fieldno);
> +		}
>  	}
>  	/* Generate ck constraint test code. */
>  	vdbe_emit_ck_constraint(&parser, expr, ck_constraint_def->name,