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

[Kirill Shcherbatov]

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.