[tarantool-patches] Re: [PATCH 6/8] sql: replace affinity with field type in struct Expr
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Feb 5 18:08:20 MSK 2019
Thanks for the fixes! See 5 comments below.
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 5676d0720..db8577d1c 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -252,74 +244,57 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id)
> -/*
> - * pExpr is a comparison expression, eg. '=', '<', IN(...) etc.
> - * idx_affinity is the affinity of an indexed column. Return true
> - * if the index with affinity idx_affinity may be used to implement
> - * the comparison in pExpr.
> +/**
> + * @param expr is a comparison expression, eg. '=', '<', IN(...) etc.
> + * @param field_type is the type of an indexed column.
> + * @retval Return true if the index with @field_type may be used to
> + * implement the comparison in expr.
> */
> -int
> -sqlite3IndexAffinityOk(Expr * pExpr, char idx_affinity)
> +bool
> +sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type field_type)
> {
> - char aff = comparisonAffinity(pExpr);
> - switch (aff) {
> - case AFFINITY_BLOB:
> - return 1;
> - case AFFINITY_TEXT:
> - return idx_affinity == AFFINITY_TEXT;
> + enum field_type type = expr_cmp_mutual_type(expr);
> + switch (type) {
> + case FIELD_TYPE_SCALAR:
> + return true;
> + case FIELD_TYPE_STRING:
> + return field_type == FIELD_TYPE_STRING;
1. But type is never == FIELD_TYPE_STRING here. Because
expr_cmp_mutual_type() returns either SCALAR or NUMBER.
I added an assertion type != FIELD_TYPE_STRING and it did
not fail in tests.
> default:
> - return sqlite3IsNumericAffinity(idx_affinity);
> + return sql_type_is_numeric(field_type);
2. Also, I do not understand, why not to just use
field_type1_contains_type2(). sql_expr_cmp_type_is_compatible()
says, that it returns true, if a column of type field_type can
be used to "implement comparison" in an expression. So
this is the same as
field_type1_contains_type2(field_type, expr_cmp_mutual_type(expr));
If I correctly understood what does it mean - "implement comparison".
Then I tried to replace sql_expr_cmp_type_is_compatible() with the
call above and the tests failed. Then I swapped the arguments:
field_type1_contains_type2(expr_cmp_mutual_type(expr), field_type);
And the tests passed. Why?
> }
> }
>
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index ef4514374..9843eb4a5 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1728,38 +1727,31 @@ generateColumnNames(Parse * pParse, /* Parser context */
> p = pEList->a[i].pExpr;
> if (NEVER(p == 0))
> continue;
> - switch (p->affinity) {
> - case AFFINITY_INTEGER:
> + if (p->op == TK_VARIABLE)
> + var_pos[var_count++] = i;
3. Why did you move these two lines out of 'switch:' clause? As I
understand, for field types INTEGER, NUMBER, STRING, SCALAR op
never == TK_VARIABLE. So this check now executes mostly without
necessity, and should be done only for FIELD_TYPE_BOOLEAN. I
did this and the tests passed:
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 9843eb4a5..d8ab9a40d 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1727,8 +1727,6 @@ generateColumnNames(Parse * pParse, /* Parser context */
p = pEList->a[i].pExpr;
if (NEVER(p == 0))
continue;
- if (p->op == TK_VARIABLE)
- var_pos[var_count++] = i;
switch (p->type) {
case FIELD_TYPE_INTEGER:
sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "INTEGER",
@@ -1747,6 +1745,8 @@ generateColumnNames(Parse * pParse, /* Parser context */
SQLITE_TRANSIENT);
break;
case FIELD_TYPE_BOOLEAN:
+ if (p->op == TK_VARIABLE)
+ var_pos[var_count++] = i;
sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "BOOLEAN",
SQLITE_TRANSIENT);
break;
> + switch (p->type) {
> + case FIELD_TYPE_INTEGER:
> sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "INTEGER",
> SQLITE_TRANSIENT);
> break;
> - case AFFINITY_REAL:
> + case FIELD_TYPE_NUMBER:
> sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "NUMERIC",
> SQLITE_TRANSIENT);
> break;
> - case AFFINITY_TEXT:
> + case FIELD_TYPE_STRING:
> sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "TEXT",
> SQLITE_TRANSIENT);
> break;
> - case AFFINITY_BLOB:
> + case FIELD_TYPE_SCALAR:
> sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "BLOB",
> SQLITE_TRANSIENT);
> break;
> - default: ;
> - char *type;
> - /*
> - * For variables we set BOOLEAN type since
> - * unassigned bindings will be replaced
> - * with NULL automatically, i.e. without
> - * explicit call of sql_bind_value().
> - */
> - if (p->op == TK_VARIABLE) {
> - var_pos[var_count++] = i;
> - type = "BOOLEAN";
> - } else {
> - type = "UNKNOWN";
> - }
> - sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, type,
> + case FIELD_TYPE_BOOLEAN:
> + sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "BOOLEAN",
> + SQLITE_TRANSIENT);
> + break;
> + default:
> + sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "UNKNOWN",
> SQLITE_TRANSIENT);
> }
> if (pEList->a[i].zName) {
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 79999509c..047a77b68 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2091,8 +2093,8 @@ typedef int ynVar;
> struct Expr {
> u8 op; /* Operation performed by this node */
> union {
> - /** The affinity of the column. */
> - enum affinity_type affinity;
> + /** The Type of the column. */
4. 'Type' -> 'type'.
> + enum field_type type;
> /** Conflict action for RAISE() function. */
> enum on_conflict_action on_conflict_action;
> };
> @@ -4267,26 +4281,30 @@ sql_index_type_str(struct sqlite3 *db, const struct index_def *idx_def);
> /**
> - * Return the affinity character for a single column of a table.
> - * @param def space definition.
> - * @param idx column index.
> - * @retval AFFINITY
> + * Return the type of the expression pExpr.
> + *
> + * If pExpr is a column, a reference to a column via an 'AS' alias,
> + * or a sub-select with a column as the return value, then the
> + * type of that column is returned. Otherwise, type ANY is returned,
> + * indicating that the expression can feature any type.
> + *
> + * The WHERE clause expressions in the following statements all
> + * have an type:
> + *
> + * CREATE TABLE t1(a);
> + * SELECT * FROM t1 WHERE a;
> + * SELECT a AS b FROM t1 WHERE b;
> + * SELECT * FROM t1 WHERE (select a from t1);
> */
> -char
> -sqlite3TableColumnAffinity(struct space_def *def, int idx);
> -
> -char sqlite3ExprAffinity(Expr * pExpr);
> +enum field_type
> +sql_expr_type(Expr *pExpr);
5. Expr -> struct Expr.
More information about the Tarantool-patches
mailing list