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;
next prev parent 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