[tarantool-patches] Re: [PATCH v1 4/4] sql: get rid of Column structure

[Vladislav Shpilevoy]

Hello. Thanks for the patch! But it again is raw. You skipped
field_def.coll initialization for non-sql code, alter.cc etc.

On 16/07/2018 19:33, Kirill Shcherbatov wrote:
> Thank you for review.
> 
>> 1. And why do you need nAlloc now? Please, do not hurry. It is not ok that
>> you overlook such conspicuous things.
> -       nAlloc = (((pNew->def->field_count - 1) / 8) * 8) + 8;
> -       assert((uint32_t)nAlloc >= pNew->def->field_count && nAlloc % 8 == 0 &&
> -              nAlloc - pNew->def->field_count < 8);
> Dropped.
> 
>> 2. What does this comment mean?
>> 3. What about other fields? Please, use = field_def_default.
> -       for (i = 0; i < pNew->def->field_count; i++) {
> -               /* FIXME: pNew->def->name = sqlite3DbStrDup(db, pCol->zName); */
> -               pNew->def->fields[i].coll = NULL;
> -               pNew->def->fields[i].coll_id = COLL_NONE;
> -       }
> +       for (uint32_t i = 0; i < pNew->def->field_count; i++)
> +               pNew->def->fields[i] = field_def_default;
> 
> 
>> 4. No. As I said you on the first review, you should not use here
>> scans or is_primkey. This check can be spread over other column check
>> functions, involved in the first commit with no additional scans.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index a6f3559..3449d3c 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -126,38 +126,48 @@ index_has_column(const i16 *parts, int parts_count, int column_idx)
>   
>   /**
>    * This is a function which should be called during execution
> - * of sqlite3EndTable. It ensures that only PRIMARY KEY
> - * constraint may have ON CONFLICT REPLACE clause.
> + * of sqlite3EndTable. It set defaults for columns having no
> + * separate NULL/NOT NULL specifiers and ensures that only
> + * PRIMARY KEY constraint may have ON CONFLICT REPLACE clause.
>    *
> + * @param parser SQL Parser object.
>    * @param table Space which should be checked.
> - * @retval False, if only primary key constraint has
> - *         ON CONFLICT REPLACE clause or if there are no indexes
> - *         with REPLACE as error action. True otherwise.
> + * @retval -1 on error. Parser SQL_TARANTOOL_ERROR is set.
> + * @retval 0 on success.
>    */
> -static bool
> -check_on_conflict_replace_entries(struct Table *table)
> -{
> -	/* Check all NOT NULL constraints. */
> -	for (int i = 0; i < (int)table->def->field_count; i++) {
> -		enum on_conflict_action on_error =
> -			table->def->fields[i].nullable_action;
> -		struct Index *pk = sqlite3PrimaryKeyIndex(table);
> -		if (on_error == ON_CONFLICT_ACTION_REPLACE &&
> +static int
> +check_on_conflict_replace_entries(struct Parse *parser, struct Table *table)
> +{
> +	const char *err_msg = NULL;
> +	struct field_def *field = table->def->fields;
> +	struct Index *pk = sqlite3PrimaryKeyIndex(table);
> +	for (uint32_t i = 0; i < table->def->field_count; ++i, ++field) {
> +		if (field->nullable_action == on_conflict_action_MAX) {
> +			/* Set default. */
> +			field->nullable_action = ON_CONFLICT_ACTION_NONE;
> +			field->is_nullable = true;
> +		}
> +		if (field->nullable_action == ON_CONFLICT_ACTION_REPLACE &&
>   		    !index_has_column(pk->aiColumn, pk->nColumn, i))
> -			return true;
> +			goto non_pk_on_conflict_error;
>   	}
> -	/* Check all UNIQUE constraints. */
> +
>   	for (struct Index *idx = table->pIndex; idx; idx = idx->pNext) {
>   		if (idx->onError == ON_CONFLICT_ACTION_REPLACE &&
> -		    !IsPrimaryKeyIndex(idx)) {
> -			return true;
> -		}
> +		    !IsPrimaryKeyIndex(idx))
> +			goto non_pk_on_conflict_error;
>   	}
> -	/*
> -	 * CHECK constraints are not allowed to have REPLACE as
> -	 * error action and therefore can be skipped.
> -	 */
> -	return false;
> +
> +	return 0;
> +
> +non_pk_on_conflict_error:
> +	err_msg = tt_sprintf("only PRIMARY KEY constraint can have "
> +			     "ON CONFLICT REPLACE clause - %s",
> +			     table->def->name);
> +	diag_set(ClientError, ER_SQL, err_msg);
> +	parser->rc = SQL_TARANTOOL_ERROR;
> +	parser->nErr++;
> +	return -1;
>   }
>   
>   /*
> @@ -1748,22 +1758,8 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
>   		}
>   	}
>   
> -	/* Set default on_nullable action if required. */
> -	struct field_def *field = p->def->fields;
> -	for (uint32_t i = 0; i < p->def->field_count; ++i, ++field) {
> -		if (field->nullable_action == on_conflict_action_MAX) {
> -			field->nullable_action = ON_CONFLICT_ACTION_NONE;
> -			field->is_nullable = true;
> -		}
> -	}
> -
> -	if (check_on_conflict_replace_entries(p)) {
> -		sqlite3ErrorMsg(pParse,
> -				"only PRIMARY KEY constraint can "
> -				"have ON CONFLICT REPLACE clause "
> -				"- %s", p->def->name);
> +	if (check_on_conflict_replace_entries(pParse, p))
>   		goto cleanup;
> -	}
>   
>   	if (db->init.busy) {
>   		/*
> @@ -1906,6 +1902,7 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
>   cleanup:
>   	sql_expr_list_delete(db, p->def->opts.checks);
>   	p->def->opts.checks = NULL;
> +	return;
>   }
> 
>> 5. Please, make the code more readable. You should avoid such long names
>> as 'collumn_id' (btw 'collumn' word does not exist), save pTab->def->fields
>> in a separate variable etc.
> @@ -905,16 +915,15 @@ sqlite3AddPrimaryKey(Parse * pParse,      /* Parsing context */
>                              sqlite3ExprSkipCollate(pList->a[i].pExpr);
>                          assert(pCExpr != 0);
>                          if (pCExpr->op != TK_ID) {
> -                               sqlite3ErrorMsg(pParse, "expressions prohibited in PRIMARY KEY");
> +                               sqlite3ErrorMsg(pParse, "expressions prohibited"
> +                                                       " in PRIMARY KEY");
>                                  goto primary_key_exit;
>                          }
> -                       const char *zCName = pCExpr->u.zToken;
> -                       for (uint32_t collumn_id = 0;
> -                            collumn_id < pTab->def->field_count; collumn_id++) {
> -                               if (strcmp(zCName,
> -                                          pTab->def->fields[collumn_id].name)
> -                                   == 0) {
> -                                       iCol = collumn_id;
> +                       const char *name = pCExpr->u.zToken;
> +                       struct space_def *def = pTab->def;
> +                       for (uint32_t idx = 0; idx < def->field_count; idx++) {
> +                               if (strcmp(name, def->fields[idx].name) == 0) {
> +                                       iCol = idx;
>                                          break;
>                                  }
>                          }
> 
> 
>> 6. Why do you need to enclose != NULL into the brackets?
> -               (expr_list != NULL) ? (uint32_t)expr_list->nExpr : 0;
> +               expr_list != NULL ? (uint32_t)expr_list->nExpr : 0;
> 
> Hm, It's is a little more convenient to read for me. I'll try to be a little more pedantic.
>