Tarantool development patches archive
 help / color / mirror / Atom feed
From: "n.pettik" <korablev@tarantool.org>
To: tarantool-patches@freelists.org
Cc: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH 6/8] sql: replace affinity with field type in struct Expr
Date: Mon, 28 Jan 2019 19:39:57 +0300	[thread overview]
Message-ID: <7FD8757A-713C-4887-9268-42849FFEE3A8@tarantool.org> (raw)
In-Reply-To: <2837d5ad-fc59-8c3e-0fef-b82ea764f6ca@tarantool.org>


>>>> -int
>>>> -sqlite3IndexAffinityOk(Expr * pExpr, char idx_affinity)
>>>> +enum field_type
>>>> +sql_index_type_is_ok(struct Expr *expr, enum field_type idx_type)
>>> 
>>> 4. Strictly speaking, it is not idx_type. It is a type of one
>>> column. I think, it should be renamed to just type, or field_type,
>>> as well as in struct WhereScan.
>> Yea, it’s fair. Renamed:
>> -/*
>> - * pExpr is a comparison operator.  Return the type affinity that should
>> - * be applied to both operands prior to doing the comparison.
>> +/**
>> + * pExpr is a comparison operator. Return the type affinity
>> + * that should be applied to both operands prior to doing
>> + * the comparison.
>>   */
>>  static enum field_type
>> -comparisonAffinity(Expr * pExpr)
>> +expr_cmp_mutual_type(struct Expr *pExpr)
>>  {
>>         assert(pExpr->op == TK_EQ || pExpr->op == TK_IN || pExpr->op == TK_LT ||
>>                pExpr->op == TK_GT || pExpr->op == TK_GE || pExpr->op == TK_LE ||
> 
> Are you sure? Looks like it is not renamed. Also, the comment
> was about sql_index_type_is_ok and its parameter idx_type, but you
> changed comparisonAffinity function.

Sorry, I renamed function, but forgot about param. Fixed:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 1de980f00..1d4ea6ef0 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -281,21 +281,21 @@ expr_cmp_mutual_type(struct Expr *pExpr)
 
 /**
  * @param expr is a comparison expression, eg. '=', '<', IN(...) etc.
- * @param idx_type is the type of an indexed column.
- * @retval Return true if the index with @idx_type may be used to
+ * @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.
  */
 enum field_type
-sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type idx_type)
+sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type field_type)
 {
        enum field_type type = expr_cmp_mutual_type(expr);
        switch (type) {
        case FIELD_TYPE_SCALAR:
                return 1;
        case FIELD_TYPE_STRING:
-               return idx_type == FIELD_TYPE_STRING;
+               return field_type == FIELD_TYPE_STRING;
        default:
-               return sql_type_is_numeric(idx_type);
+               return sql_type_is_numeric(field_type);
        }
 }

>>>>  @@ -2826,16 +2795,15 @@ sqlite3CodeSubselect(Parse * pParse,	/* Parsing context */
>>>>  				 * that columns affinity when building index keys. If <expr> is not
>>>>  				 * a column, use numeric affinity.
>>>>  				 */
>>>> -				char affinity;	/* Affinity of the LHS of the IN */
>>>>  				int i;
>>>>  				ExprList *pList = pExpr->x.pList;
>>>>  				struct ExprList_item *pItem;
>>>>  				int r1, r2, r3;
>>>>  -				affinity = sqlite3ExprAffinity(pLeft);
>>>> -				if (!affinity) {
>>>> -					affinity = AFFINITY_BLOB;
>>>> -				}
>>>> +				enum field_type lhs_type =
>>>> +					sql_expr_type(pLeft);
>>>> +				if (lhs_type == FIELD_TYPE_ANY)
>>>> +					lhs_type = FIELD_TYPE_SCALAR;
>>> 
>>> 7. Why did not you move this conversion to sql_expr_type? I mean
>>> ANY -> SCALAR.
>> Unfortunately, right now I can’t replace it with ANY. It is still used
>> when IN operator comes with one operand (see EP_Generic flag).
>> I have already sent patch which removes it:
>> https://github.com/tarantool/tarantool/tree/np/gh-3934-IN-operator-fix
> 
> I've already acked the patch. Please, hurry up Kirill Y. to push it,
> rebase this branch, and replace ANY with SCALAR.

I had to add some other fixes to make this happen:

Changes to sql: replace affinity with field type for func:

diff --git a/src/box/sql/date.c b/src/box/sql/date.c
index e452aad73..f32a5841a 100644
--- a/src/box/sql/date.c
+++ b/src/box/sql/date.c
@@ -1307,14 +1307,14 @@ sqlite3RegisterDateTimeFunctions(void)
        static FuncDef aDateTimeFuncs[] = {
 #ifndef SQLITE_OMIT_DATETIME_FUNCS
                DFUNCTION(julianday, -1, 0, 0, juliandayFunc, FIELD_TYPE_NUMBER),
-               DFUNCTION(date, -1, 0, 0, dateFunc, FIELD_TYPE_NUMBER),
-               DFUNCTION(time, -1, 0, 0, timeFunc, FIELD_TYPE_NUMBER),
-               DFUNCTION(datetime, -1, 0, 0, datetimeFunc, FIELD_TYPE_NUMBER),
-               DFUNCTION(strftime, -1, 0, 0, strftimeFunc, FIELD_TYPE_NUMBER),
-               DFUNCTION(current_time, 0, 0, 0, ctimeFunc, FIELD_TYPE_NUMBER),
+               DFUNCTION(date, -1, 0, 0, dateFunc, FIELD_TYPE_STRING),
+               DFUNCTION(time, -1, 0, 0, timeFunc, FIELD_TYPE_STRING),
+               DFUNCTION(datetime, -1, 0, 0, datetimeFunc, FIELD_TYPE_STRING),
+               DFUNCTION(strftime, -1, 0, 0, strftimeFunc, FIELD_TYPE_STRING),
+               DFUNCTION(current_time, 0, 0, 0, ctimeFunc, FIELD_TYPE_STRING),
                DFUNCTION(current_timestamp, 0, 0, 0, ctimestampFunc,
-                         FIELD_TYPE_NUMBER),
-               DFUNCTION(current_date, 0, 0, 0, cdateFunc, FIELD_TYPE_NUMBER),
+                         FIELD_TYPE_STRING),
+               DFUNCTION(current_date, 0, 0, 0, cdateFunc, FIELD_TYPE_STRING),
 #else
                STR_FUNCTION(current_time, 0, "%H:%M:%S", 0, currentTimeFunc),
                STR_FUNCTION(current_date, 0, "%Y-%m-%d", 0, currentTimeFunc),
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 347fee563..58e35e0d9 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3995,7 +3995,7 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
                                break;
                        }
 
-                       if (pDef->ret_type != AFFINITY_UNDEFINED) {
+                       if (pDef->ret_type != FIELD_TYPE_SCALAR) {
                                pExpr->affinity =
                                        sql_field_type_to_affinity(pDef->ret_type);
                        } else {
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 08228fbbe..147f93ae4 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1733,14 +1733,14 @@ sqlite3RegisterBuiltinFunctions(void)
                FUNCTION(rtrim, 2, 2, 0, trimFunc, FIELD_TYPE_STRING),
                FUNCTION(trim, 1, 3, 0, trimFunc, FIELD_TYPE_STRING),
                FUNCTION(trim, 2, 3, 0, trimFunc, FIELD_TYPE_STRING),
-               FUNCTION(min, -1, 0, 1, minmaxFunc, 0),
-               FUNCTION(min, 0, 0, 1, 0, 0),
+               FUNCTION(min, -1, 0, 1, minmaxFunc, FIELD_TYPE_SCALAR),
+               FUNCTION(min, 0, 0, 1, 0, FIELD_TYPE_SCALAR),
                AGGREGATE2(min, 1, 0, 1, minmaxStep, minMaxFinalize,
-                          SQLITE_FUNC_MINMAX, 0),
-               FUNCTION(max, -1, 1, 1, minmaxFunc, 0),
-               FUNCTION(max, 0, 1, 1, 0, 0),
+                          SQLITE_FUNC_MINMAX, FIELD_TYPE_SCALAR),
+               FUNCTION(max, -1, 1, 1, minmaxFunc, FIELD_TYPE_SCALAR),
+               FUNCTION(max, 0, 1, 1, 0, FIELD_TYPE_SCALAR),
                AGGREGATE2(max, 1, 1, 1, minmaxStep, minMaxFinalize,
-                          SQLITE_FUNC_MINMAX, 0),
+                          SQLITE_FUNC_MINMAX, FIELD_TYPE_SCALAR),
                FUNCTION2(typeof, 1, 0, 0, typeofFunc, SQLITE_FUNC_TYPEOF,
                          FIELD_TYPE_STRING),
                FUNCTION2(length, 1, 0, 0, lengthFunc, SQLITE_FUNC_LENGTH,
@@ -1769,9 +1769,9 @@ sqlite3RegisterBuiltinFunctions(void)
                FUNCTION(zeroblob, 1, 0, 0, zeroblobFunc, FIELD_TYPE_SCALAR),
                FUNCTION(substr, 2, 0, 0, substrFunc, FIELD_TYPE_STRING),
                FUNCTION(substr, 3, 0, 0, substrFunc, FIELD_TYPE_STRING),
-               AGGREGATE(sum, 1, 0, 0, sumStep, sumFinalize, 0),
-               AGGREGATE(total, 1, 0, 0, sumStep, totalFinalize, 0),
-               AGGREGATE(avg, 1, 0, 0, sumStep, avgFinalize, 0),
+               AGGREGATE(sum, 1, 0, 0, sumStep, sumFinalize, FIELD_TYPE_NUMBER),
+               AGGREGATE(total, 1, 0, 0, sumStep, totalFinalize, FIELD_TYPE_NUMBER),
+               AGGREGATE(avg, 1, 0, 0, sumStep, avgFinalize, FIELD_TYPE_NUMBER),
                AGGREGATE2(count, 0, 0, 0, countStep, countFinalize,
                           SQLITE_FUNC_COUNT, FIELD_TYPE_INTEGER),
                AGGREGATE(count, 1, 0, 0, countStep, countFinalize,
@@ -1788,9 +1788,10 @@ sqlite3RegisterBuiltinFunctions(void)
 #ifdef SQLITE_ENABLE_UNKNOWN_SQL_FUNCTION
                FUNCTION(unknown, -1, 0, 0, unknownFunc, 0),
 #endif
-               FUNCTION(coalesce, 1, 0, 0, 0, 0),
-               FUNCTION(coalesce, 0, 0, 0, 0, 0),
-               FUNCTION2(coalesce, -1, 0, 0, noopFunc, SQLITE_FUNC_COALESCE, 0),
+               FUNCTION(coalesce, 1, 0, 0, 0, FIELD_TYPE_SCALAR),
+               FUNCTION(coalesce, 0, 0, 0, 0, FIELD_TYPE_SCALAR),
+               FUNCTION2(coalesce, -1, 0, 0, noopFunc, SQLITE_FUNC_COALESCE,
+                         FIELD_TYPE_SCALAR),
        };
        sqlite3AnalyzeFunctions();
        sqlite3RegisterDateTimeFunctions();
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 4c6baaac6..cbda48f78 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -637,6 +637,8 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
                                }
                        } else {
                                is_agg = pDef->xFinalize != 0;
+                               pExpr->affinity =
+                                       sql_field_type_to_affinity(pDef->ret_type);
                                if (pDef->funcFlags & SQLITE_FUNC_UNLIKELY) {
                                        ExprSetProperty(pExpr,
                                                        EP_Unlikely | EP_Skip);
diff --git a/test-run b/test-run
index 02207efd2..4c3f11812 160000
--- a/test-run
+++ b/test-run
@@ -1 +1 @@
-Subproject commit 02207efd2ca44067b76c79bf012f142016d929ae
+Subproject commit 4c3f11812c0c17f08780f31b6a748eef1126b2e2
diff --git a/test/sql-tap/analyze9.test.lua b/test/sql-tap/analyze9.test.lua
index df62a1624..c43437712 100755
--- a/test/sql-tap/analyze9.test.lua
+++ b/test/sql-tap/analyze9.test.lua
@@ -58,6 +58,9 @@ msgpack_decode_sample = function(txt)
         end
         i = i+1
     end
+    if type(decoded_str) == "number" then
+        return tostring(decoded_str)
+    end
     return decoded_str
 end
 
@@ -332,7 +335,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     4.7,
     [[
-        SELECT count(*) FROM "_sql_stat4" WHERE msgpack_decode_sample("sample") IN (34, 68, 102, 136, 170, 204, 238, 272);
+        SELECT count(*) FROM "_sql_stat4" WHERE lrange(msgpack_decode_sample("sample"), 1, 1) IN ('34', '68', '102', '136', '170', '204', '238', '272');
     ]], {
         -- <4.7>
         8
@@ -367,8 +370,8 @@ test:do_execsql_test(
         SELECT msgpack_decode_sample("sample") FROM "_sql_stat4";
     ]], {
         -- <4.9>
-        "x", 1110, 2230, 2750, 3350, 4090, 4470, 4980, 5240, 5280, 5290, 5590, 5920, 
-        5930, 6220, 6710, 7000, 7710, 7830, 7970, 8890, 8950, 9240, 9250, 9680
+        "x", "1110", "2230", "2750", "3350", "4090", "4470", "4980", "5240", "5280", "5290", "5590", "5920",
+        "5930", "6220", "6710", "7000", "7710", "7830", "7970", "8890", "8950", "9240", "9250", "9680"
         -- </4.9>
     })

Additional changes to  sql: replace affinity with field type in struct Expr:

Firstly, I placed several asserts like assert(type != FIELD_TYPE_ANY);
and noted that not for all expressions its true. So I patched sql_expr_type
function:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index fdf00273a..e1a0dfa73 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -74,10 +74,29 @@ sql_expr_type(struct Expr *pExpr)
        case TK_MINUS:
        case TK_STAR:
        case TK_SLASH:
+       case TK_REM:
+       case TK_BITAND:
+       case TK_BITOR:
+       case TK_LSHIFT:
+       case TK_RSHIFT:
                assert(pExpr->pRight != NULL && pExpr->pLeft != NULL);
                enum field_type lhs_type = sql_expr_type(pExpr->pLeft);
                enum field_type rhs_type = sql_expr_type(pExpr->pRight);
                return sql_type_result(rhs_type, lhs_type);
+       case TK_CONCAT:
+                       return FIELD_TYPE_STRING;
+       case TK_CASE:
+               assert(pExpr->x.pList->nExpr >= 2);
+               /*
+                * CASE expression comes at least with one
+                * WHEN and one THEN clauses. So, first
+                * expression always represents WHEN
+                * argument, and the second one - THEN.
+                *
+                * TODO: We must ensure that all THEN clauses
+                * have arguments of the same type.
+                */
+               return sql_expr_type(pExpr->x.pList->a[1].pExpr);
        case TK_LT:
        case TK_GT:
        case TK_EQ:
@@ -92,16 +111,14 @@ sql_expr_type(struct Expr *pExpr)
                 * return INTEGER.
                 */
                return FIELD_TYPE_INTEGER;
-       }
-       /*
-        * In case of unary plus we shouldn't discard
-        * type of operand (since plus always features
-        * NUMERIC type).
-        */
-       if (op == TK_UPLUS) {
+       case TK_UMINUS:
+       case TK_UPLUS:
+       case TK_NO:
+       case TK_BITNOT:
                assert(pExpr->pRight == NULL);
-               return pExpr->pLeft->type;
+               return sql_expr_type(pExpr->pLeft);
        }
+
        return pExpr->type;
 }

Then, I make default type of expression be SCALAR
except for binding params:

diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index ef691cb5c..d6fc41e3b 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -829,6 +829,18 @@ idlist(A) ::= nm(Y).
       case TK_FLOAT:
         p->type = FIELD_TYPE_NUMBER;
         break;
+      case TK_VARIABLE:
+        /*
+         * For variables we set BOOLEAN type since
+         * unassigned bindings will be replaced
+         * with NULL automatically, i.e. without
+         * explicit call of sql_bind_value().
+         */
+        p->type = FIELD_TYPE_BOOLEAN;
+        break;
+      default:
+        p->type = FIELD_TYPE_SCALAR;
+        break;
       }
       p->op = (u8)op;
       p->flags = EP_Leaf;

diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 8a8ccc68c..2cc0a4a78 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1725,6 +1726,8 @@ 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",
@@ -1742,21 +1745,12 @@ generateColumnNames(Parse * pParse,     /* Parser context */
                        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) {

Finally, I simplified derived type calculations and removed
all ANY -> SCALAR transformations:

diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 8a8ccc68c..2cc0a4a78 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1973,8 +1967,6 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse,            /* Parsing contexts */
        for (uint32_t i = 0; i < pTab->def->field_count; i++) {
                p = a[i].pExpr;
                enum field_type type = sql_expr_type(p);
-               if (type == FIELD_TYPE_ANY)
-                       type = FIELD_TYPE_SCALAR;
                pTab->def->fields[i].affinity = sql_field_type_to_affinity(type);
                pTab->def->fields[i].type = type;
                bool is_found;
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 930b53b2b..44305f7dc 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1169,7 +1169,7 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo,  /* Complete information about t
                int fieldno = idx_pk->key_def->parts[0].fieldno;
                enum field_type fd_type = is_format_set ?
                                          space->def->fields[fieldno].type :
-                                         FIELD_TYPE_ANY;
+                                         FIELD_TYPE_SCALAR;
 
                uint32_t pk_part_count = idx_pk->key_def->part_count;
                if (pk_part_count == 1 && fd_type == FIELD_TYPE_INTEGER) {
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index 6ffd5ae84..1fb5fa593 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -758,7 +758,7 @@ exprAnalyzeOrTerm(SrcList * pSrc,   /* the FROM clause */
                                        enum field_type lhs =
                                                sql_expr_type(pOrTerm->pExpr->
                                                        pLeft);
-                                       if (rhs != FIELD_TYPE_ANY &&
+                                       if (rhs != FIELD_TYPE_SCALAR &&
                                            rhs != lhs) {
                                                okToChngToIN = 0;
                                        } else {

diff --git a/src/box/sql.c b/src/box/sql.c
index 530ec2384..cb8f6a60a 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -387,8 +387,8 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
                part->sort_order = SORT_ORDER_ASC;
                if (def != NULL && i < def->part_count) {
                        assert(def->parts[i].type < field_type_MAX);
-                       part->type = def->parts[i].type != FIELD_TYPE_ANY ?
-                                    def->parts[i].type : FIELD_TYPE_SCALAR;
+                       assert(def->parts[i].type != FIELD_TYPE_ANY);
+                       part->type = def->parts[i].type;
                        part->coll_id = def->parts[i].coll_id;
                } else {
                        part->coll_id = COLL_NONE;

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index fdf00273a..e1a0dfa73 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -231,22 +248,9 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id)
 enum field_type
 sql_type_result(enum field_type lhs, enum field_type rhs)
 {
-       if (lhs != FIELD_TYPE_ANY && rhs != FIELD_TYPE_ANY) {
-               if (sql_type_is_numeric(lhs) || sql_type_is_numeric(rhs))
-                       return FIELD_TYPE_NUMBER;
-               else
-                       return FIELD_TYPE_SCALAR;
-       } else if (lhs == FIELD_TYPE_ANY && rhs == FIELD_TYPE_ANY) {
-               /*
-                * Neither side of the comparison is a column.
-                * Compare the results directly.
-                */
-               return FIELD_TYPE_SCALAR;
-       } else {
-               if (lhs == FIELD_TYPE_ANY)
-                       return rhs;
-               return lhs;
-       }
+       if (sql_type_is_numeric(lhs) || sql_type_is_numeric(rhs))
+               return FIELD_TYPE_NUMBER;
+       return FIELD_TYPE_SCALAR;
 }

@@ -2784,8 +2788,6 @@ sqlite3CodeSubselect(Parse * pParse,      /* Parsing context */
 
                                enum field_type lhs_type =
                                        sql_expr_type(pLeft);
-                               if (lhs_type == FIELD_TYPE_ANY)
-                                       lhs_type = FIELD_TYPE_SCALAR;
                                bool unused;
                                sql_expr_coll(pParse, pExpr->pLeft,
                                              &unused, &key_info->parts[0].coll_id);

Test below are changed due to correct calculation of derived
type for unary operators: you may notice that before it was
simple expr->type. However, it didn’t give us original type
of expr: for column, for instance we should fetch it from space_def.

diff --git a/test/sql-tap/where2.test.lua b/test/sql-tap/where2.test.lua
index 9089c97f9..56bec153f 100755
--- a/test/sql-tap/where2.test.lua
+++ b/test/sql-tap/where2.test.lua
@@ -653,14 +653,13 @@ test:do_test(
         "where2-6.9",
         function()
             return queryplan([[
-    -- The + operator removes affinity from the rhs.  No conversions
-    -- occur and the comparison is false.  The result is an empty set.
+    -- The + operator doesn't affect RHS.
     --
     SELECT b,a FROM t2249b CROSS JOIN t2249a WHERE a=+b;
   ]])
         end, {
             -- <where2-6.9>
-            "nosort", "T2249B", "*", "T2249A", "*"
+            123, "0123", "nosort", "T2249B", "*", "T2249A", "*"
             -- </where2-6.9>
         })
 
@@ -673,7 +672,7 @@ test:do_test(
   ]])
         end, {
             -- <where2-6.9.2>
-            "nosort", "T2249B", "*", "T2249A", "*"
+            123, "0123","nosort", "T2249B", "*", "T2249A", "*"
             -- </where2-6.9.2>
         })
 
@@ -681,9 +680,6 @@ test:do_test(
         "where2-6.10",
         function()
             return queryplan([[
-    -- Use + on both sides of the comparison to disable indices
-    -- completely.  Make sure we get the same result.
-    --
     SELECT b,a FROM t2249b CROSS JOIN t2249a WHERE +a=+b;
   ]])
         end, {
@@ -753,45 +749,36 @@ test:do_test(
     test:do_test(
         "where2-6.12",
         function()
-            -- In this case, the +b disables the affinity conflict and allows
-            -- the OR optimization to be used again.  The result is now an empty
-            -- set, the same as in where2-6.9.
             return queryplan([[
       SELECT b,a FROM t2249b CROSS JOIN t2249a WHERE a=+b OR a='hello';
     ]])
         end, {
             -- <where2-6.12>
-            "nosort", "T2249B", "*", "T2249A", "*"
+            123, "0123", "nosort", "T2249B", "*", "T2249A", "*"
             -- </where2-6.12>
         })
 
     test:do_test(
         "where2-6.12.2",
         function()
-            -- In this case, the +b disables the affinity conflict and allows
-            -- the OR optimization to be used again.  The result is now an empty
-            -- set, the same as in where2-6.9.
             return queryplan([[
       SELECT b,a FROM t2249b CROSS JOIN t2249a WHERE a='hello' OR +b=a;
     ]])
         end, {
             -- <where2-6.12.2>
-            "nosort", "T2249B", "*", "T2249A", "*"
+            123, "0123", "nosort", "T2249B", "*", "T2249A", "*"
             -- </where2-6.12.2>
         })
 
     test:do_test(
         "where2-6.12.3",
         function()
-            -- In this case, the +b disables the affinity conflict and allows
-            -- the OR optimization to be used again.  The result is now an empty
-            -- set, the same as in where2-6.9.
             return queryplan([[
       SELECT b,a FROM t2249b CROSS JOIN t2249a WHERE +b=a OR a='hello';
     ]])
         end, {
             -- <where2-6.12.3>
-            "nosort", "T2249B", "*", "T2249A", "*"
+            123, "0123", "nosort", "T2249B", "*", "T2249A", "*"
             -- </where2-6.12.3>
         })

> 
>> After it hits 2.1, I will replace SCALAR with ANY.
> 
> Vice versa? Not SCALAR with ANY, but ANY with SCALAR, I think.

Yep.

>>> ANY differs from SCALAR in only one thing - it is
>>> able to store MP_MAP and MP_ARRAY. So I am slightly bent upon
>>> frequency of ANY usage in SQL, wherein MAP/ARRAY does not exist.
>> But still we can SELECT data from spaces created from Lua.
>> For instance, _fk_constraint features type ARRAY in its format,
>> so we can’t ignore this type even now (IMHO).
> 
> Despite our ability to select complex types, in SQL they are all
> mere binary data with *sub*type msgpack, not array/map type. So
> in SQL we never truly work with anything but SCALAR.

Wait, at the stage of query compiling we don’t operate on
any subtypes. We simply fetch space_def->fields->type,
which may turn out to be FIELD_TYPE_MAP/ARRAY or
whatever.

>> -/*
>> - * Return TRUE if the given expression is a constant which would be
>> - * unchanged by OP_ApplyType with the type given in the second
>> - * argument.
>> - *
>> - * This routine is used to determine if the OP_ApplyType operation
>> - * can be omitted.  When in doubt return FALSE.  A false negative
>> - * is harmless.  A false positive, however, can result in the wrong
>> - * answer.
>> - */
>> int
>> -sqlite3ExprNeedsNoAffinityChange(const Expr * p, char aff)
>> +sql_expr_needs_no_type_change(const Expr *p, enum field_type type)
> 
> 1. const Expr -> const struct Expr

Fixed:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 1de980f00..ddce8c13c 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -2117,7 +2117,7 @@ sqlite3ExprCanBeNull(const Expr * p)
 }
 
 int
-sql_expr_needs_no_type_change(const Expr *p, enum field_type type)
+sql_expr_needs_no_type_change(const struct Expr *p, enum field_type type)
 {
        u8 op;
        if (type == FIELD_TYPE_SCALAR)
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index f44235cb6..fac650f62 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3721,7 +3721,7 @@ int sqlite3ExprCanBeNull(const Expr *);
  * answer.
  */
 int
-sql_expr_needs_no_type_change(const Expr *epr, enum field_type type);
+sql_expr_needs_no_type_change(const struct Expr *expr, enum field_type type);

>> +/**
>> + * Return TRUE if the given expression is a constant which would
>> + * be unchanged by OP_ApplyType with the type given in the second
>> + * argument.
>> + *
>> + * This routine is used to determine if the OP_ApplyType operation
>> + * can be omitted.  When in doubt return FALSE.  A false negative
>> + * is harmless. A false positive, however, can result in the wrong
>> + * answer.
>> + */
>> +int
>> +sql_expr_needs_no_type_change(const Expr *epr, enum field_type type);
> 
> 2. epr -> p, and the same as 1.

Fixed, see above.

>> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
>> index cd71641b0..d12a2a833 100644
>> --- a/src/box/sql/vdbemem.c
>> +++ b/src/box/sql/vdbemem.c
>> @@ -1316,8 +1315,8 @@ valueFromExpr(sqlite3 * db,	/* The database connection */
>> 			sqlite3ValueSetStr(pVal, -1, zVal, SQLITE_DYNAMIC);
>> 		}
>> 		if ((op == TK_INTEGER || op == TK_FLOAT)
>> -		    && affinity == AFFINITY_BLOB) {
>> -			sqlite3ValueApplyAffinity(pVal, AFFINITY_REAL);
>> +		    && affinity == FIELD_TYPE_SCALAR) {
>> +			sqlite3ValueApplyAffinity(pVal, FIELD_TYPE_INTEGER);
> 
> 3. Why before your patch it was REAL and now it is INTEGER?

Typo. Fixed in previous patch (while I was renaming affinity vars to type).

>> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
>> index efbc91cf8..33b860f36 100644
>> --- a/src/box/sql/wherecode.c
>> +++ b/src/box/sql/wherecode.c
>> @@ -423,10 +423,11 @@ updateRangeAffinityStr(Expr * pRight,	/* RHS of comparison */
>> {
>> 	int i;
>> 	for (i = 0; i < n; i++) {
>> +		enum field_type type = sql_affinity_to_field_type(zAff[i]);
>> 		Expr *p = sqlite3VectorFieldSubexpr(pRight, i);
>> -		enum affinity_type aff = sqlite3ExprAffinity(p);
>> -		if (sql_affinity_result(aff, zAff[i]) == AFFINITY_BLOB
>> -		    || sqlite3ExprNeedsNoAffinityChange(p, zAff[i])) {
>> +		enum field_type expr_type = sql_expr_type(p);
>> +		if (sql_type_result(expr_type, type) == FIELD_TYPE_SCALAR ||
>> +			sql_expr_needs_no_type_change(p, type)) {
>> 			zAff[i] = AFFINITY_BLOB;
>> 		}
>> 	}
> 
> 4. Something is wrong with formatting. sql_expr_needs_no_type_change should be
> aligned under sql_type_result as a part of the condition.

Fixed:

diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 33b860f36..bc57efe18 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -427,7 +427,7 @@ updateRangeAffinityStr(Expr * pRight,       /* RHS of comparison */
                Expr *p = sqlite3VectorFieldSubexpr(pRight, i);
                enum field_type expr_type = sql_expr_type(p);
                if (sql_type_result(expr_type, type) == FIELD_TYPE_SCALAR ||
-                       sql_expr_needs_no_type_change(p, type)) {
+                   sql_expr_needs_no_type_change(p, type)) {
                        zAff[i] = AFFINITY_BLOB;
                }
        }

>> @@ -1166,20 +1167,12 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about t
>> 		}
>> 		struct index_def *idx_pk = space->index[0]->def;
>> 		int fieldno = idx_pk->key_def->parts[0].fieldno;
>> -		char affinity = is_format_set ?
>> -				space->def->fields[fieldno].affinity :
>> -				AFFINITY_BLOB;
>> -		if (affinity == AFFINITY_UNDEFINED) {
>> -			if (idx_pk->key_def->part_count == 1 &&
>> -			    space->def->fields[fieldno].type ==
>> -			    FIELD_TYPE_INTEGER)
>> -				affinity = AFFINITY_INTEGER;
>> -			else
>> -				affinity = AFFINITY_BLOB;
>> -		}
>> +		char fd_type = is_format_set ?
>> +				space->def->fields[fieldno].type :
>> +				FIELD_TYPE_ANY;
>> 
> 
> 5. Why char? And the alignment is slightly wrong. Non-first lines
> should be aligned under is_format_set.

Fixed:

@@ -1167,9 +1167,9 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo,  /* Complete information about t
                }
                struct index_def *idx_pk = space->index[0]->def;
                int fieldno = idx_pk->key_def->parts[0].fieldno;
-               char fd_type = is_format_set ?
-                               space->def->fields[fieldno].type :
-                               FIELD_TYPE_ANY;
+               enum field_type fd_type = is_format_set ?
+                                         space->def->fields[fieldno].type :
+                                         FIELD_TYPE_SCALAR;

  reply	other threads:[~2019-01-28 16:39 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-28  9:34 [tarantool-patches] [PATCH 0/8] Eliminate affinity from source code Nikita Pettik
2018-12-28  9:34 ` [tarantool-patches] [PATCH 1/8] sql: remove SQLITE_ENABLE_UPDATE_DELETE_LIMIT define Nikita Pettik
2018-12-29 17:42   ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-16 14:25     ` n.pettik
2018-12-28  9:34 ` [tarantool-patches] [PATCH 2/8] sql: use field type instead of affinity for type_def Nikita Pettik
2018-12-29 17:42   ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-16 14:26     ` n.pettik
2018-12-28  9:34 ` [tarantool-patches] [PATCH 3/8] sql: remove numeric affinity Nikita Pettik
2018-12-29  9:01   ` [tarantool-patches] " Konstantin Osipov
2018-12-29 17:42   ` Vladislav Shpilevoy
2019-01-09  8:26     ` Konstantin Osipov
2019-01-16 14:26     ` n.pettik
2019-01-22 15:41       ` Vladislav Shpilevoy
2019-01-28 16:39         ` n.pettik
2019-01-30 13:04           ` Vladislav Shpilevoy
2019-02-01 16:39             ` n.pettik
2019-01-09  8:20   ` Konstantin Osipov
2018-12-28  9:34 ` [tarantool-patches] [PATCH 4/8] sql: replace affinity with field type for func Nikita Pettik
2018-12-28  9:34 ` [tarantool-patches] [PATCH 5/8] sql: replace field type with affinity for VDBE runtime Nikita Pettik
2018-12-29 17:42   ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-16 14:26     ` n.pettik
2019-01-22 15:41       ` Vladislav Shpilevoy
2019-01-28 16:39         ` n.pettik
2019-01-30 13:04           ` Vladislav Shpilevoy
2019-02-01 16:39             ` n.pettik
2019-02-05 15:08               ` Vladislav Shpilevoy
2019-02-05 17:46                 ` n.pettik
2018-12-28  9:34 ` [tarantool-patches] [PATCH 6/8] sql: replace affinity with field type in struct Expr Nikita Pettik
2018-12-29 17:42   ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-16 14:26     ` n.pettik
2019-01-22 15:41       ` Vladislav Shpilevoy
2019-01-28 16:39         ` n.pettik [this message]
2019-01-30 13:04           ` Vladislav Shpilevoy
2019-02-01 16:39             ` n.pettik
2019-02-05 15:08               ` Vladislav Shpilevoy
2019-02-05 17:46                 ` n.pettik
2018-12-28  9:34 ` [tarantool-patches] [PATCH 7/8] sql: clean-up affinity from SQL source code Nikita Pettik
2018-12-29 17:42   ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-16 14:26     ` n.pettik
2019-01-22 15:41       ` Vladislav Shpilevoy
2019-01-28 16:40         ` n.pettik
2019-01-30 13:04           ` Vladislav Shpilevoy
2019-02-01 16:39             ` n.pettik
2019-02-05 15:08               ` Vladislav Shpilevoy
2019-02-05 17:46                 ` n.pettik
2018-12-28  9:34 ` [tarantool-patches] [PATCH 8/8] Remove affinity from field definition Nikita Pettik
2019-02-05 19:41 ` [tarantool-patches] Re: [PATCH 0/8] Eliminate affinity from source code Vladislav Shpilevoy
2019-02-08 13:37 ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7FD8757A-713C-4887-9268-42849FFEE3A8@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='[tarantool-patches] Re: [PATCH 6/8] sql: replace affinity with field type in struct Expr' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox