* [tarantool-patches] [PATCH v2 0/3] Change collation compatibility rules according to ANSI SQL
@ 2018-11-13 0:07 Nikita Pettik
2018-11-13 0:07 ` [tarantool-patches] [PATCH v2 1/3] sql: do not add explicit <COLLATE "BINARY"> clause Nikita Pettik
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Nikita Pettik @ 2018-11-13 0:07 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
This patch-set introduces rules for collation compatibility in order to
restrict usage of different collations within one comparison.
Originally, SQLite uses doubtful approach: if collations of LHS and RHS
can not be used together, then collation of LHS is chosen. Such
behaviour quite misleading and results in the fact that swap of LHS and
RHS values may change result of operation. We've decided to add
restrictions on collations of LHS and RHS to disallow messing different
collations as it is stated in ANSI SQL.
Changes in v2:
Instead of using surrogate ids for "binary" and "none" collations,
now real ones are added to _collation space. They also come with
new collation type - binary. It simply uses memcmp as a collation
rule. For details see second patch in series.
Minor fixes:
- Make collations_are_compatible() be collations_check_compatibility()
and process diag_set() right there;
- Clean-up debug asserts.
Nikita Pettik (3):
sql: do not add explicit <COLLATE "BINARY"> clause
Introduce "none" and "binary" collations
sql: change collation compatibility rules
src/box/alter.cc | 13 ++++-
src/box/bootstrap.snap | Bin 1888 -> 1911 bytes
src/box/coll_id.c | 1 -
src/box/errcode.h | 2 +
src/box/field_def.c | 2 +-
src/box/key_def.c | 18 +++---
src/box/key_def.h | 6 --
src/box/lua/space.cc | 2 +-
src/box/lua/upgrade.lua | 2 +
src/box/sql.c | 10 ++--
src/box/sql/build.c | 2 +-
src/box/sql/callback.c | 19 +++++--
src/box/sql/expr.c | 73 ++++++++++++++++++------
src/box/sql/fkey.c | 2 -
src/box/sql/func.c | 2 +-
src/box/sql/select.c | 117 ++++++++++++++++++++------------------
src/box/sql/sqliteInt.h | 28 ++++++++-
src/box/sql/where.c | 15 +++--
src/box/sql/whereexpr.c | 38 ++++++-------
src/box/tuple_format.c | 4 +-
src/coll.c | 52 ++++++++++++++---
src/coll_def.c | 2 +-
src/coll_def.h | 1 +
test/box/ddl.result | 20 ++++---
test/box/misc.result | 2 +
test/box/net.box.result | 2 +-
test/sql-tap/collation.test.lua | 12 ++--
test/sql-tap/distinct.test.lua | 2 +-
test/sql-tap/e_select1.test.lua | 12 ++--
test/sql-tap/in3.test.lua | 2 +-
test/sql-tap/in4.test.lua | 2 +-
test/sql-tap/index3.test.lua | 2 +-
test/sql-tap/join.test.lua | 2 +-
test/sql-tap/tkt3493.test.lua | 4 +-
test/sql-tap/transitive1.test.lua | 4 +-
test/sql-tap/where2.test.lua | 2 +-
test/sql-tap/with1.test.lua | 2 +-
test/sql/collation.result | 90 +++++++++++++++++++++++++++++
test/sql/collation.test.lua | 36 ++++++++++++
39 files changed, 427 insertions(+), 180 deletions(-)
--
2.15.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] [PATCH v2 1/3] sql: do not add explicit <COLLATE "BINARY"> clause
2018-11-13 0:07 [tarantool-patches] [PATCH v2 0/3] Change collation compatibility rules according to ANSI SQL Nikita Pettik
@ 2018-11-13 0:07 ` Nikita Pettik
2018-11-13 0:07 ` [tarantool-patches] [PATCH v2 2/3] Introduce "none" and "binary" collations Nikita Pettik
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Nikita Pettik @ 2018-11-13 0:07 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
We don't need to add explicit COLLATE "BINARY" clause since binary
collation is anyway default. On the other hand, it may confuse
due to ambiugty when comparing two terms.
Needed for #3185
---
src/box/sql/fkey.c | 2 --
src/box/sql/select.c | 4 ----
src/box/sql/whereexpr.c | 25 ++++++++++++-------------
3 files changed, 12 insertions(+), 19 deletions(-)
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index fc39c879e..4e3270f0c 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -313,8 +313,6 @@ exprTableRegister(Parse * pParse, /* Parsing and code generating context */
pExpr->iTable = regBase + iCol + 1;
char affinity = pTab->def->fields[iCol].affinity;
pExpr->affinity = affinity;
- pExpr = sqlite3ExprAddCollateString(pParse, pExpr,
- "binary");
} else {
pExpr->iTable = regBase;
pExpr->affinity = AFFINITY_INTEGER;
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 16b1044d6..dfa6ed8e0 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -2238,10 +2238,6 @@ sql_multiselect_orderby_to_key_info(struct Parse *parse, struct Select *s,
order_by->a[i].pExpr =
sqlite3ExprAddCollateString(parse, term,
name);
- } else {
- order_by->a[i].pExpr =
- sqlite3ExprAddCollateString(parse, term,
- "BINARY");
}
}
part->coll_id = id;
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index 612868695..9fa6ce15d 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -1133,7 +1133,6 @@ exprAnalyze(SrcList * pSrc, /* the FROM clause */
Expr *pNewExpr2;
int idxNew1;
int idxNew2;
- const char *zCollSeqName; /* Name of collating sequence */
const u16 wtFlags = TERM_LIKEOPT | TERM_VIRTUAL | TERM_DYNAMIC;
pLeft = pExpr->x.pList->a[1].pExpr;
@@ -1171,24 +1170,24 @@ exprAnalyze(SrcList * pSrc, /* the FROM clause */
}
*pC = c + 1;
}
- /* Support only for unicode_ci indexes by now */
- zCollSeqName = noCase ? "unicode_ci" : "BINARY";
pNewExpr1 = sqlite3ExprDup(db, pLeft, 0);
- pNewExpr1 = sqlite3PExpr(pParse, TK_GE,
- sqlite3ExprAddCollateString(pParse,
- pNewExpr1,
- zCollSeqName),
- pStr1);
+ if (noCase) {
+ pNewExpr1 =
+ sqlite3ExprAddCollateString(pParse, pNewExpr1,
+ "unicode_ci");
+ }
+ pNewExpr1 = sqlite3PExpr(pParse, TK_GE, pNewExpr1, pStr1);
transferJoinMarkings(pNewExpr1, pExpr);
idxNew1 = whereClauseInsert(pWC, pNewExpr1, wtFlags);
testcase(idxNew1 == 0);
exprAnalyze(pSrc, pWC, idxNew1);
pNewExpr2 = sqlite3ExprDup(db, pLeft, 0);
- pNewExpr2 = sqlite3PExpr(pParse, TK_LT,
- sqlite3ExprAddCollateString(pParse,
- pNewExpr2,
- zCollSeqName),
- pStr2);
+ if (noCase) {
+ pNewExpr2 =
+ sqlite3ExprAddCollateString(pParse, pNewExpr2,
+ "unicode_ci");
+ }
+ pNewExpr2 = sqlite3PExpr(pParse, TK_LT, pNewExpr2, pStr2);
transferJoinMarkings(pNewExpr2, pExpr);
idxNew2 = whereClauseInsert(pWC, pNewExpr2, wtFlags);
testcase(idxNew2 == 0);
--
2.15.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] [PATCH v2 2/3] Introduce "none" and "binary" collations
2018-11-13 0:07 [tarantool-patches] [PATCH v2 0/3] Change collation compatibility rules according to ANSI SQL Nikita Pettik
2018-11-13 0:07 ` [tarantool-patches] [PATCH v2 1/3] sql: do not add explicit <COLLATE "BINARY"> clause Nikita Pettik
@ 2018-11-13 0:07 ` Nikita Pettik
2018-11-13 12:02 ` [tarantool-patches] " Vladislav Shpilevoy
2018-11-13 0:07 ` [tarantool-patches] [PATCH v2 3/3] sql: change collation compatibility rules Nikita Pettik
2018-11-15 11:24 ` [tarantool-patches] Re: [PATCH v2 0/3] Change collation compatibility rules according to ANSI SQL Kirill Yukhin
3 siblings, 1 reply; 12+ messages in thread
From: Nikita Pettik @ 2018-11-13 0:07 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
This patch introduces two new collation sequences: "none" and "binary".
Despite the fact that they use the same comparing algorithm (simple
byte-by-byte comparison), they don't mean the same. "binary" collation
get to the format if user explicitly points it: either specifies this
collation in space format manually or adds <COLLATE BINARY> clause to
column definition within CREATE TABLE statement. "none" collation is
used when user doesn't specify any collation at all. "none" collation always
comes with id == 0 and it can't be changed (since its id vastly used
under the hood as an indicator of absence of collation).
Difference between these collations is vital for ANSI SQL: mixing
"binary" with other collations is prohibited, meanwhile "none" collation
can be used alongside with others.
In this respect current patch extends list of available collations: now
not only ICU collations are allowed, but also BINARY.
Note, that in SQL some queries have changed their query plan. That
occurred due to the fact that our parser allows using <COLLATE> clause
with numeric fields:
CREATE TABLE (id INT PRIMARY KEY);
SELECT id COLLATE "binary" ...
In the example collation of LHS (id column) is NULL, but collation of
RHS is "binary". Before this patch both collations were NULL. Hence,
usage of certain indexes may not be allowed by query planner.
On the other hand, this feature is obviously broken, so that doesn't
seem to be big deal.
Needed for #3185
---
src/box/alter.cc | 13 ++++++++--
src/box/bootstrap.snap | Bin 1888 -> 1911 bytes
src/box/coll_id.c | 1 -
src/box/errcode.h | 1 +
src/box/field_def.c | 2 +-
src/box/key_def.c | 18 +++++++-------
src/box/key_def.h | 6 -----
src/box/lua/space.cc | 2 +-
src/box/lua/upgrade.lua | 2 ++
src/box/sql.c | 10 ++++----
src/box/sql/build.c | 2 +-
src/box/sql/callback.c | 19 +++++++++++----
src/box/sql/expr.c | 2 +-
src/box/sql/func.c | 2 +-
src/box/sql/select.c | 8 +++----
src/box/sql/where.c | 2 +-
src/box/tuple_format.c | 4 ++--
src/coll.c | 52 +++++++++++++++++++++++++++++++++-------
src/coll_def.c | 2 +-
src/coll_def.h | 1 +
test/box/ddl.result | 20 +++++++++-------
test/box/misc.result | 1 +
test/box/net.box.result | 2 +-
test/sql-tap/collation.test.lua | 12 ++++++----
test/sql-tap/distinct.test.lua | 2 +-
test/sql-tap/in3.test.lua | 2 +-
test/sql-tap/index3.test.lua | 2 +-
test/sql-tap/where2.test.lua | 2 +-
test/sql/collation.result | 27 +++++++++++++++++++++
test/sql/collation.test.lua | 14 +++++++++++
30 files changed, 166 insertions(+), 67 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 6d2c59bbc..2eb9f53e8 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -403,7 +403,7 @@ field_def_decode(struct field_def *field, const char **data,
"nullable action properties", fieldno +
TUPLE_INDEX_BASE));
}
- if (field->coll_id != COLL_NONE &&
+ if (field->coll_id != 0 &&
field->type != FIELD_TYPE_STRING &&
field->type != FIELD_TYPE_SCALAR &&
field->type != FIELD_TYPE_ANY) {
@@ -2560,7 +2560,6 @@ coll_id_def_new_from_tuple(const struct tuple *tuple, struct coll_id_def *def)
tuple_field_with_type_xc(tuple, BOX_COLLATION_FIELD_OPTIONS,
MP_MAP);
- assert(base->type == COLL_TYPE_ICU);
if (opts_decode(&base->icu, coll_icu_opts_reg, &options,
ER_WRONG_COLLATION_OPTIONS,
BOX_COLLATION_FIELD_OPTIONS, NULL) != 0)
@@ -2671,6 +2670,16 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event)
*/
int32_t old_id = tuple_field_u32_xc(old_tuple,
BOX_COLLATION_FIELD_ID);
+ /*
+ * Don't allow user to drop "none" collation
+ * since it is very special and vastly used
+ * under the hood. Hence, we can rely on the
+ * fact that "none" collation features id == 0.
+ */
+ if (old_id == 0) {
+ tnt_raise(ClientError, ER_DROP_COLLATION, "none",
+ "system collation");
+ }
struct coll_id *old_coll_id = coll_by_id(old_id);
assert(old_coll_id != NULL);
access_check_ddl(old_coll_id->name, old_coll_id->id,
diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index 65739384a66d6ba4a538553ccf4677536ba15280..5e136d147036760cb67df68e81aab5744be783e4 100644
GIT binary patch
delta 1909
zcmV-*2a5RM4)+d_7k@P^GdL}0GiGFFH!?UkWC}@cb97;DV`VxZIb=CFWHw=8Eip7@
zHZ3$|Wn?X3VP<75WjSRrW;tVGG-6_A3RXjGZ)0mZAbWiZ3e~y`y3Gdt0M5D=y5Xe&
z00000D77#B08phj0J=<Y5J}KjO#vWYm=|2a2zw054KzTf;(v`no5cMPX0Ik5Fk}sC
zZHY`tO-kA|Nr?QCPAdtQm=p%^DbL*=mX%*pI<rd`svcw`Pi0Ce?*Qij`T#~|EuDAg
zwISJ&8Ps3@D(~xEm|GitA#nG;<%8nbOU=QV_qvt29&azkI?c=W@fG>)UsuzHil~>Z
zIEK4*6Pgc(E`KBVf-I!(558x;-}}G!EEv-nW}{F_my&cb17iE?%&Kt$xH`$$zOTGR
zo2$3<@6K;R8QY{9z-I-F8GSFrS&udffUA={vcw0D9U3rH)adI_)8<hJ!*O+zlU5yO
zz8_{o!qrK3p-W!2LHAXoT%9CJby|h+KGnq+gy~DASbwG!%1fD8DG=9`laqtOp9*8Z
zvcK~Y#5P&?Lg%#dGL&UV%N8FlS9YoZsB<Vt7rdC)kZi|agcU0Nhh5*@vcd8eZAn!R
zjP~;p1;f=zy83vH_Vdw}^yB~u*5e1WHG0#4tCJl2b0k=gpDn2kgCm%=(e7nmvCOlh
zp!amPHGf*OKnctyQ5psPde#Z0kbtX`)c3(KWPv#LE_`v>_wBq4x;npR`&-|2wu#?4
zO^N>T=eb3qGYEh_!+4C_VAe)u4)pul;a6*2@GH9>Lu@_p^QWY9OMfkit<e|*T%DwN
zUo+mtkalqh*2GxfuYtmn$&n6ozaSW5n?zv>aDR1@R32nq5FEATS?Ndc!pfMwGcxu8
zRi!gkDL7e?3I*x}$^@#!iNtAjQr(QW5kYhl!X{;t!_`R=*?{O!vhGy#K~{r9W1{gu
z)4|2Bivow4n;Dy#4Kf;JGRR=4HRb}95lx{n7GgR|VQRQKNtg;#MnvVupRC84(EskV
z`F}!}j=CBeu1-=33~|!+#8HPxF5K^{<XK7oz9NzbW*1h{b-ufE9b#+bvrec-fvb~z
zU0(Av4b-!r*R!LPkeSYDL0+DLtUg2a+qngQxJ{x#CV1cPe*G=Wa}%YaCsc<7u1>O)
zeZN?byIUmMBg5567PVS=cYC8eF<hPGD1WmzsuKa0x4449)k(G<%&@%0AB&Y@;nj!w
z)tmmi*W~@DOd>voGTke}Q?0xe1}s$8>39_|W$AFY_7GWF)Q}q?T@V@|ROSa)Cs|?W
zQdxaa9p;Yp_}wZs>Qp_tbm?M+^5E(uW9IYyu&l@14z5lTK%La6Q~B%{M_$qjI)88m
z#otPl%kf~iX*h$dQLNEw)SYnX{PJ!|=?$(<@<i)%i7^iD#BhN--1zZ8F&h0O#sGSl
z3nm41UECyI7e*4=&qrG4KekFbjk=Q!u1<2tN5+hXpUyb@`G|uP6tbXDYSh`(pdJmb
zPSTqZKvpC~hAc>o8|~2nqlbs`yno<vL3LW_uvo2FsaWmEY5{?9sPSB6s02n30003<
z0MP{pG3bg05@4`MjH4)yffxuw5R4)lM?e4yLJBm9S}-&d%%{4wpNvvCz%hP|9;3(T
zF?zHEA24jqw9<6O0<kp)YS~^M3_}1zWP+j450X47k(ks|7U0<bm|XF(>VJwCyFxW&
zvSx>h*Xrupa#T-amV|4!+4G#G@C5t>lh2DV*jpCAP2@0w?0v*%tf^HhsNvQ{7l)g>
z5CZc%DXe^PZ}cV*RfytKSHf2D@T#r3MhHgUaO#{b0JA~=3XQU^`W(%7AyoI+1(QS-
zqc0cr3D2Ph*<hTN1NY20D}Nub*|X;doDFCMT{2G^FEqytSV)}uyY`&Glh#2|ZZU<a
zzUb1zX4XR_Zpg*&3z9osX4aT3gYK*`P1`?bGjG4U5)1@z%(1$?)d+h(Wr^9R`$E!f
ziSYdK3?HKdKvkBAFH^DzhpkH3f`7TKX8p--!lH5-?E5pZWK6YXqJPOLZ|Q}nl-W`j
zILKukUul5)<NRY6Bwu<4t3$j7+MW_(S6|7ho030)#c`U7rV*MP3NVaBZBM^cYTN9O
ziBi!&?!b?8tb@U(@15Tay+#-~n&rgTr+ia&_}^81l(p#5`)@np*g}UzaEkyp_}YhV
z!BtU?NRF(bYDJt+V}JRtEd<3cBuX5T-PT>QLz$n5K}W9^T8SL-I*Q|CJJQu!#!+|M
zH=_fq+5ko|oy_gO$HeItV&D`Qr?+iTcBTvGOe_s3$|U5@f+vRDt(EhWRKkg}6aa8j
ze83bezNpuT3z|n5)<;Wzkj^-TyWZmAiaL=OI__QSp=Dq<d2Y3b*#G#fQWSAX^v0ef
zxhhH|_1n4|(%~2VL`z9-(zl|(2RqRZtyz>JLomv9uig;>T;qt+2cQ^PqXJ1aA^JZK
vL55PXLzk)lO8W>&phrR$u~5(Z*LkIYMVy@l-;OZk>+zl>-;fT~5UuSBltZC&
delta 1886
zcmV-k2ch`)4&V-u7k@M@I5#mZXEQS}VKFskVq;}xWC}@cb97;DV`VxZGc`3ZV>UB6
zEi*7;Vl6Z_W-~1~FgZ0XI5lH1H#s$9F*ac{3RXjGZ)0mZAbWiZ3e~y`y3GdT0L~dp
z)=i}V00000D77#B08rI706I%F5K6#UQ2{85q9}?YD$b&eu79{-3N}O7l2mpR;KZ{2
zn+{@4)QCi;q$nk^F5UY}lDbGJHYRv&HT-RauFbizhI29aQ_<+yD7;K51?d3d0QUge
z@{IH&cw?nZUm7X<AvBlHyK|e+jK~D)u!ofw_Akusjj{;1d*AXwaqOk$AnkkY%3P1Q
z7iGQnW&8Mw9Dnz)t7}3;^vhNp!|lQa%?HER5PU&a?)!uP+3)xM)_)e1=^V38D5X=3
zcrF3DI!W2Su)M|FtGo2?&T&B*+oKAgtCJ*x<i-gWH2PnNvmS5M$ZP4kI>||^4l~~$
zyA|o`B$McrFI%7sv(c_j^4i?AS~#iHcgndb*|2O@rGL-H=B9Kq-J4QQ3x#n|IW`!q
z?C*R8@ona@&^f8F2r)^*5u~qF*^vUEPN7|$r0c;P%UjG{@M3Pu*^XZbD@6JayT03X
zi{&lalBygO?dRjEL02dF>f<fi&qrHQ6N4gHkFA)sQ5u%6PIBzei(oyrY)NMp7{%<3
zdJ6Ux%YRHW0{Ty9Yojs=j>PN{jX}`wXO~b42f8{*eIE=%7Kmm4!WWl)-_A>*t22zY
z-}PPRoA{mAj_4nKp4%LSG4S&_#%25#vp4!upkLSyTQ=7PTV>Z{i0ub{ZXxOH(r+_j
zZPbN8S0^c67>#!^tX&*}Hc{61>z}YpVx+^|FMkMz*dEcA0bQMBlm}TC1jj3#Rf(Cc
zsz^-Lh;Bxt5^)M~0&!ZIRHhsg4o!;YW{3tgV>5H=>LiC|Ox}l?S0huJ$#{tA5W^vc
z=Ed)e!VJs?7!5EPUNF2|c(G8ci$`ZeP^gQHO9e_>V!ArX77BAV6y-;stVdhW|L(QP
zLVu?ic``6vog@+%;-mYCqYiT~+%K%;8A<=XV(tU;3m@q^-`%+mu^rN~OXxSDtCNIX
zZu@i%)HI*hG^3jz5<0H|eVGQbI_=bN=N9zg_K5m;`M%%%`dyZ1B}ha}s128{PO_YR
zzgUmE+Z^qQ(A7y2xm<a7i=#Xcx;n{mW`A*1=RquQan%y2i*v%MRgMX#RHf9)q$;6W
zI5#C0%Y-VeP&n9>N}(`Vp)*g1RpHVgI-RXOKvq9aK4^M)JYsgx7ad)lWNo2S%9?}f
zFn6rS7Ov7FPlY2XDTxZb(bY-1%;)=KS&ujyU7aL=I%$!os<B@jc}XkKz!?;OBY)9u
z#)ILm;S91y(MD^LXOf{a?7JnUF}ga*5w6xtlyPvUg$mqZ29N)d!RRKj#m~oHu_ved
z;wACEa4u*+A8Vce=ql+f@=P$gI?0_IE@(7t>0Gm)k2N(wA*%_cMc&K{`nl-pB)=8`
zW<)=FW<FTpD36U5Jt`<q3LX?x=YNEbiPcVlYpC&HWT*s05C8xHNC43V2Qlc11`=Sf
zK#Zd>j)4#eLlBBW8%IC@3_=Pth*~go48;SwHJFH2Fd%F?2kk;VXcww=-~-00nO2(4
z7#_C9Kn&JPXB`m05Sd^o^n)x9k}%ou6eKwIauTj!EYZT3wXZg0T(d*RYkzg?LUB|#
zW0r(#8`yKErQ-?y^2rxLfPl3%zvXk}o-2LCX{@RGC#bmUq8G=dT?n=Lxht&Ltv5XM
zKq^G>sVm1;VNq3wzfvSZ&zbt0ErQxX{}LL1Ts3TVav_oK;iE_r^^ZOib+@*&8Jb6T
z{24#RZ0XOiX`b+=wiC=jmw(uke4NxWLj+RR*0wq)@TAqaja#AVtR8b(h~aq%?uI^M
z`prRRPT(uuXYNcJ+VJy!Hk<p!R)T;J@vKg_H;wT38<xm^m(NJmmI#uUo&7P|2}oqg
z_tGMZ@S$}GTlD{LtGR))1F&c@4JrSb3^FEkG11VJx0FKTGHdEW3V*pE$5+~-{vxmL
zf*4EBeRas!LD@4G?DCbYS}*y)SR8><QJU1qp&G+T()NPsQhQ*3fG8CW<4FAYY>n6@
z`rP@=q}MzHN3)#x+LLco2l`9ZcTS5Eseh}}gCB-3f?K0?yRSvk7El%CEOMj@g~-HV
zYP|oo3c>daiHd?`w|_NfFDEk*ne8ai;u4WnDMz7QaYtIM<u>XL{bqEKk~Rq!Oegci
z?|E^eRScX$<Mg@>shsKmIpci>6g3mx`LKW?cWdQ*5}9z8djSBq#m8;I;y?8oaX|qI
z!}@5+->Dg+0MUDNxS*Xd7KijMg=l18w|Yfa#Q*iIdK8gK^jFHBq=_p2AoZQJo5JB~
z{)9_u-DI}n!v|vG5UoYj4MPyhwO7m|j<QA`KTd1ZBMB3t<;Mk&Gr6RupO)4s0af}3
YejQs<I4l@DLYl8<_9QWfbkz{8?dNiiMF0Q*
diff --git a/src/box/coll_id.c b/src/box/coll_id.c
index 2d5f8a09a..b56c74961 100644
--- a/src/box/coll_id.c
+++ b/src/box/coll_id.c
@@ -37,7 +37,6 @@
struct coll_id *
coll_id_new(const struct coll_id_def *def)
{
- assert(def->base.type == COLL_TYPE_ICU);
size_t total_len = sizeof(struct coll_id) + def->name_len + 1;
struct coll_id *coll_id = (struct coll_id *) malloc(total_len);
if (coll_id == NULL) {
diff --git a/src/box/errcode.h b/src/box/errcode.h
index 4eb7fced5..18ffdf3d5 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -224,6 +224,7 @@ struct errcode_record {
/*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint %s does not exist") \
/*170 */_(ER_CONSTRAINT_EXISTS, "Constraint %s already exists") \
/*171 */_(ER_SQL_TYPE_MISMATCH, "Type mismatch: can not convert %s to %s") \
+ /*172 */_(ER_DROP_COLLATION, "Can't drop collation %s : %s") \
/*
* !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/field_def.c b/src/box/field_def.c
index 3a9ff3703..3e63e12a3 100644
--- a/src/box/field_def.c
+++ b/src/box/field_def.c
@@ -123,7 +123,7 @@ const struct field_def field_def_default = {
.name = NULL,
.is_nullable = false,
.nullable_action = ON_CONFLICT_ACTION_DEFAULT,
- .coll_id = COLL_NONE,
+ .coll_id = 0,
.default_value = NULL,
.default_value_expr = NULL
};
diff --git a/src/box/key_def.c b/src/box/key_def.c
index 3a560bb06..6802489f1 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -41,7 +41,7 @@ const char *sort_order_strs[] = { "asc", "desc", "undef" };
const struct key_part_def key_part_def_default = {
0,
field_type_MAX,
- COLL_NONE,
+ 0,
false,
ON_CONFLICT_ACTION_DEFAULT,
SORT_ORDER_ASC
@@ -174,7 +174,7 @@ key_def_new(const struct key_part_def *parts, uint32_t part_count)
for (uint32_t i = 0; i < part_count; i++) {
const struct key_part_def *part = &parts[i];
struct coll *coll = NULL;
- if (part->coll_id != COLL_NONE) {
+ if (part->coll_id != 0) {
struct coll_id *coll_id = coll_by_id(part->coll_id);
if (coll_id == NULL) {
diag_set(ClientError, ER_WRONG_INDEX_OPTIONS,
@@ -223,7 +223,7 @@ box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count)
key_def_set_part(key_def, item, fields[item],
(enum field_type)types[item],
ON_CONFLICT_ACTION_DEFAULT,
- NULL, COLL_NONE, SORT_ORDER_ASC);
+ NULL, 0, SORT_ORDER_ASC);
}
key_def_set_cmp(key_def);
return key_def;
@@ -319,7 +319,7 @@ key_def_sizeof_parts(const struct key_part_def *parts, uint32_t part_count)
for (uint32_t i = 0; i < part_count; i++) {
const struct key_part_def *part = &parts[i];
int count = 2;
- if (part->coll_id != COLL_NONE)
+ if (part->coll_id != 0)
count++;
if (part->is_nullable)
count++;
@@ -329,7 +329,7 @@ key_def_sizeof_parts(const struct key_part_def *parts, uint32_t part_count)
assert(part->type < field_type_MAX);
size += mp_sizeof_str(strlen(PART_OPT_TYPE));
size += mp_sizeof_str(strlen(field_type_strs[part->type]));
- if (part->coll_id != COLL_NONE) {
+ if (part->coll_id != 0) {
size += mp_sizeof_str(strlen(PART_OPT_COLLATION));
size += mp_sizeof_uint(part->coll_id);
}
@@ -348,7 +348,7 @@ key_def_encode_parts(char *data, const struct key_part_def *parts,
for (uint32_t i = 0; i < part_count; i++) {
const struct key_part_def *part = &parts[i];
int count = 2;
- if (part->coll_id != COLL_NONE)
+ if (part->coll_id != 0)
count++;
if (part->is_nullable)
count++;
@@ -361,7 +361,7 @@ key_def_encode_parts(char *data, const struct key_part_def *parts,
assert(part->type < field_type_MAX);
const char *type_str = field_type_strs[part->type];
data = mp_encode_str(data, type_str, strlen(type_str));
- if (part->coll_id != COLL_NONE) {
+ if (part->coll_id != 0) {
data = mp_encode_str(data, PART_OPT_COLLATION,
strlen(PART_OPT_COLLATION));
data = mp_encode_uint(data, part->coll_id);
@@ -431,7 +431,7 @@ key_def_decode_parts_166(struct key_part_def *parts, uint32_t part_count,
part->is_nullable = (part->fieldno < field_count ?
fields[part->fieldno].is_nullable :
key_part_def_default.is_nullable);
- part->coll_id = COLL_NONE;
+ part->coll_id = 0;
}
return 0;
}
@@ -488,7 +488,7 @@ key_def_decode_parts(struct key_part_def *parts, uint32_t part_count,
"index part: unknown field type");
return -1;
}
- if (part->coll_id != COLL_NONE &&
+ if (part->coll_id != 0 &&
part->type != FIELD_TYPE_STRING &&
part->type != FIELD_TYPE_SCALAR) {
diag_set(ClientError, ER_WRONG_INDEX_OPTIONS,
diff --git a/src/box/key_def.h b/src/box/key_def.h
index 20e79f9fe..684e9cf75 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -72,12 +72,6 @@ struct key_part_def {
extern const struct key_part_def key_part_def_default;
-/**
- * Set key_part_def.coll_id to COLL_NONE if
- * the field does not have a collation.
- */
-#define COLL_NONE UINT32_MAX
-
/** Descriptor of a single part in a multipart key. */
struct key_part {
/** Tuple field index for this part */
diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 7cae436f1..e5e09b042 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -299,7 +299,7 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i)
lua_pushboolean(L, key_part_is_nullable(part));
lua_setfield(L, -2, "is_nullable");
- if (part->coll_id != COLL_NONE) {
+ if (part->coll_id != 0) {
struct coll_id *coll_id =
coll_by_id(part->coll_id);
assert(coll_id != NULL);
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 64f74f9d3..a9525058b 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -396,8 +396,10 @@ local function create_collation_space()
box.space._index:insert{_collation.id, 1, 'name', 'tree', {unique = true}, {{1, 'string'}}}
log.info("create predefined collations")
+ box.space._collation:replace{0, "none", ADMIN, "BINARY", "", setmap{}}
box.space._collation:replace{1, "unicode", ADMIN, "ICU", "", setmap{}}
box.space._collation:replace{2, "unicode_ci", ADMIN, "ICU", "", {strength='primary'}}
+ box.space._collation:replace{3, "binary", ADMIN, "BINARY", "", setmap{}}
local _priv = box.space[box.schema.PRIV_ID]
_priv:insert{ADMIN, PUBLIC, 'space', _collation.id, box.priv.W}
diff --git a/src/box/sql.c b/src/box/sql.c
index caa66144f..686d32335 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -379,7 +379,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
if (def != NULL && i < def->part_count)
part->coll_id = def->parts[i].coll_id;
else
- part->coll_id = COLL_NONE;
+ part->coll_id = 0;
}
struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
field_count);
@@ -1139,7 +1139,7 @@ sql_encode_table(struct region *region, struct Table *table, uint32_t *size)
struct field_def *field = &def->fields[i];
const char *default_str = field->default_value;
int base_len = 5;
- if (cid != COLL_NONE)
+ if (cid != 0)
base_len += 1;
if (default_str != NULL)
base_len += 1;
@@ -1160,7 +1160,7 @@ sql_encode_table(struct region *region, struct Table *table, uint32_t *size)
const char *action =
on_conflict_action_strs[def->fields[i].nullable_action];
mpstream_encode_str(&stream, action);
- if (cid != COLL_NONE) {
+ if (cid != 0) {
mpstream_encode_str(&stream, "collation");
mpstream_encode_uint(&stream, cid);
}
@@ -1285,13 +1285,13 @@ sql_encode_index_parts(struct region *region, const struct field_def *fields,
action_is_nullable(fields[col].nullable_action));
/* Do not decode default collation. */
uint32_t cid = part->coll_id;
- mpstream_encode_map(&stream, 5 + (cid != COLL_NONE));
+ mpstream_encode_map(&stream, 5 + (cid != 0));
mpstream_encode_str(&stream, "type");
mpstream_encode_str(&stream, field_type_strs[fields[col].type]);
mpstream_encode_str(&stream, "field");
mpstream_encode_uint(&stream, col);
- if (cid != COLL_NONE) {
+ if (cid != 0) {
mpstream_encode_str(&stream, "collation");
mpstream_encode_uint(&stream, cid);
}
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 5b3348bd2..929d20dbf 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2399,7 +2399,7 @@ index_fill_def(struct Parse *parse, struct index *index,
uint32_t coll_id;
if (expr->op == TK_COLLATE) {
sql_get_coll_seq(parse, expr->u.zToken, &coll_id);
- if (coll_id == COLL_NONE &&
+ if (coll_id == 0 &&
strcasecmp(expr->u.zToken, "binary") != 0) {
diag_set(ClientError, ER_NO_SUCH_COLLATION,
expr->u.zToken);
diff --git a/src/box/sql/callback.c b/src/box/sql/callback.c
index 3cf3a835d..352745e0e 100644
--- a/src/box/sql/callback.c
+++ b/src/box/sql/callback.c
@@ -42,13 +42,22 @@
struct coll *
sql_get_coll_seq(Parse *parser, const char *name, uint32_t *coll_id)
{
- if (name == NULL || strcasecmp(name, "binary") == 0) {
- *coll_id = COLL_NONE;
- return NULL;
+ if (name == NULL) {
+ *coll_id = 0;
+ return coll_by_id(0)->coll;
}
- struct coll_id *p = coll_by_name(name, strlen(name));
+ struct coll_id *p;
+ /*
+ * In SQL all identifiers should be uppercased, so
+ * to avoid mess lets simple search binary (since it is
+ * sort of "special" collation) ignoring case at all.
+ */
+ if (strcasecmp(name, "binary") == 0)
+ p = coll_by_name("binary", strlen("binary"));
+ else
+ p = coll_by_name(name, strlen(name));
if (p == NULL) {
- *coll_id = COLL_NONE;
+ *coll_id = 0;
sqlite3ErrorMsg(parser, "no such collation sequence: %s",
name);
return NULL;
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 4d1c1a634..e52cd6407 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -194,7 +194,7 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_found, uint32_t *coll_id)
{
struct coll *coll = NULL;
*is_found = false;
- *coll_id = COLL_NONE;
+ *coll_id = 0;
while (p != NULL) {
int op = p->op;
if (p->flags & EP_Generic)
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 8c34cbb3d..580cf1e60 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -506,7 +506,7 @@ case_type##ICUFunc(sqlite3_context *context, int argc, sqlite3_value **argv) \
UErrorCode status = U_ZERO_ERROR; \
struct coll *coll = sqlite3GetFuncCollSeq(context); \
const char *locale = NULL; \
- if (coll != NULL) { \
+ if (coll != NULL && coll->collator != NULL) { \
locale = ucol_getLocaleByType(coll->collator, \
ULOC_VALID_LOCALE, &status); \
} \
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index dfa6ed8e0..cea453f08 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1345,7 +1345,7 @@ sql_key_info_new(sqlite3 *db, uint32_t part_count)
struct key_part_def *part = &key_info->parts[i];
part->fieldno = i;
part->type = FIELD_TYPE_SCALAR;
- part->coll_id = COLL_NONE;
+ part->coll_id = 0;
part->is_nullable = false;
part->nullable_action = ON_CONFLICT_ACTION_ABORT;
part->sort_order = SORT_ORDER_ASC;
@@ -1961,7 +1961,7 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse, /* Parsing contexts */
pTab->def->fields[i].type = sql_affinity_to_field_type(affinity);
bool is_found;
uint32_t coll_id;
- if (pTab->def->fields[i].coll_id == COLL_NONE &&
+ if (pTab->def->fields[i].coll_id == 0 &&
sql_expr_coll(pParse, p, &is_found, &coll_id) && is_found)
pTab->def->fields[i].coll_id = coll_id;
}
@@ -2160,7 +2160,7 @@ multi_select_coll_seq_r(Parse *parser, Select *p, int n, bool *is_found,
coll_id);
} else {
coll = NULL;
- *coll_id = COLL_NONE;
+ *coll_id = 0;
}
assert(n >= 0);
/* iCol must be less than p->pEList->nExpr. Otherwise an error would
@@ -2233,7 +2233,7 @@ sql_multiselect_orderby_to_key_info(struct Parse *parse, struct Select *s,
} else {
multi_select_coll_seq(parse, s,
item->u.x.iOrderByCol - 1, &id);
- if (id != COLL_NONE) {
+ if (id != 0) {
const char *name = coll_by_id(id)->name;
order_by->a[i].pExpr =
sqlite3ExprAddCollateString(parse, term,
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index 8c78c0c9b..1db4db874 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -2806,7 +2806,7 @@ whereLoopAddBtree(WhereLoopBuilder * pBuilder, /* WHERE clause information */
part.nullable_action = ON_CONFLICT_ACTION_ABORT;
part.is_nullable = false;
part.sort_order = SORT_ORDER_ASC;
- part.coll_id = COLL_NONE;
+ part.coll_id = 0;
struct key_def *key_def = key_def_new(&part, 1);
if (key_def == NULL) {
diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index 5bdf102e7..8e5aea51b 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -40,7 +40,7 @@ static uint32_t formats_size = 0, formats_capacity = 0;
static const struct tuple_field tuple_field_default = {
FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false,
- ON_CONFLICT_ACTION_NONE, NULL, COLL_NONE,
+ ON_CONFLICT_ACTION_DEFAULT, NULL, 0,
};
/**
@@ -67,7 +67,7 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys,
format->fields[i].nullable_action = fields[i].nullable_action;
struct coll *coll = NULL;
uint32_t cid = fields[i].coll_id;
- if (cid != COLL_NONE) {
+ if (cid != 0) {
struct coll_id *coll_id = coll_by_id(cid);
if (coll_id == NULL) {
diag_set(ClientError,ER_WRONG_COLLATION_OPTIONS,
diff --git a/src/coll.c b/src/coll.c
index 6a76f1f0b..6d9c44dbf 100644
--- a/src/coll.c
+++ b/src/coll.c
@@ -92,6 +92,17 @@ coll_icu_cmp(const char *s, size_t slen, const char *t, size_t tlen,
return (int)result;
}
+static int
+coll_bin_cmp(const char *s, size_t slen, const char *t, size_t tlen,
+ const struct coll *coll)
+{
+ (void) coll;
+ int res = memcmp(s, t, slen < tlen ? slen : tlen);
+ if (res == 0)
+ res = slen - tlen;
+ return res;
+}
+
/** Get a hash of a string using ICU collation. */
static uint32_t
coll_icu_hash(const char *s, size_t s_len, uint32_t *ph, uint32_t *pcarry,
@@ -113,6 +124,15 @@ coll_icu_hash(const char *s, size_t s_len, uint32_t *ph, uint32_t *pcarry,
return total_size;
}
+static uint32_t
+coll_bin_hash(const char *s, size_t s_len, uint32_t *ph, uint32_t *pcarry,
+ struct coll *coll)
+{
+ (void) coll;
+ PMurHash32_Process(ph, pcarry, s, s_len);
+ return s_len;
+}
+
/**
* Set up ICU collator and init cmp and hash members of collation.
* @param coll Collation to set up.
@@ -262,17 +282,22 @@ static int
coll_def_snfingerprint(char *buffer, int size, const struct coll_def *def)
{
int total = 0;
- SNPRINT(total, snprintf, buffer, size, "{locale: %s, type = %d, "\
- "icu: ", def->locale, (int) def->type);
- SNPRINT(total, coll_icu_def_snfingerprint, buffer, size, &def->icu);
- SNPRINT(total, snprintf, buffer, size, "}");
+ if (def->type == COLL_TYPE_ICU) {
+ SNPRINT(total, snprintf, buffer, size, "{locale: %s,"\
+ "type = %d, icu: ", def->locale, (int) def->type);
+ SNPRINT(total, coll_icu_def_snfingerprint, buffer,
+ size, &def->icu);
+ SNPRINT(total, snprintf, buffer, size, "}");
+ } else {
+ assert(def->type == COLL_TYPE_BINARY);
+ SNPRINT(total, snprintf, buffer, size, "{type = binary}");
+ }
return total;
}
struct coll *
coll_new(const struct coll_def *def)
{
- assert(def->type == COLL_TYPE_ICU);
int fingerprint_len = coll_def_snfingerprint(NULL, 0, def);
assert(fingerprint_len <= TT_STATIC_BUF_LEN);
char *fingerprint = tt_static_buf();
@@ -296,9 +321,20 @@ coll_new(const struct coll_def *def)
memcpy((char *) coll->fingerprint, fingerprint, fingerprint_len + 1);
coll->refs = 1;
coll->type = def->type;
- if (coll_icu_init_cmp(coll, def) != 0) {
- free(coll);
- return NULL;
+ switch (coll->type) {
+ case COLL_TYPE_ICU:
+ if (coll_icu_init_cmp(coll, def) != 0) {
+ free(coll);
+ return NULL;
+ }
+ break;
+ case COLL_TYPE_BINARY:
+ coll->collator = NULL;
+ coll->cmp = coll_bin_cmp;
+ coll->hash = coll_bin_hash;
+ break;
+ default:
+ unreachable();
}
struct mh_coll_node_t node = { fingerprint_len, hash, coll };
diff --git a/src/coll_def.c b/src/coll_def.c
index df58caca8..3a4fc2b87 100644
--- a/src/coll_def.c
+++ b/src/coll_def.c
@@ -31,7 +31,7 @@
#include "coll_def.h"
const char *coll_type_strs[] = {
- "ICU"
+ "ICU", "BINARY"
};
const char *coll_icu_on_off_strs[] = {
diff --git a/src/coll_def.h b/src/coll_def.h
index 7c20abf66..d3af89802 100644
--- a/src/coll_def.h
+++ b/src/coll_def.h
@@ -36,6 +36,7 @@
/** The supported collation types */
enum coll_type {
COLL_TYPE_ICU = 0,
+ COLL_TYPE_BINARY = 1,
coll_type_MAX,
};
diff --git a/test/box/ddl.result b/test/box/ddl.result
index c9a8e96ae..d3b0d1e0e 100644
--- a/test/box/ddl.result
+++ b/test/box/ddl.result
@@ -356,7 +356,7 @@ box.space._collation:auto_increment{'test', 0, 'ICU', 42}
...
box.space._collation:auto_increment{'test', 0, 'ICU', 'ru_RU', setmap{}} --ok
---
-- [3, 'test', 0, 'ICU', 'ru_RU', {}]
+- [4, 'test', 0, 'ICU', 'ru_RU', {}]
...
box.space._collation:auto_increment{'test', 0, 'ICU', 'ru_RU', setmap{}}
---
@@ -364,7 +364,7 @@ box.space._collation:auto_increment{'test', 0, 'ICU', 'ru_RU', setmap{}}
...
box.space._collation.index.name:delete{'test'} -- ok
---
-- [3, 'test', 0, 'ICU', 'ru_RU', {}]
+- [4, 'test', 0, 'ICU', 'ru_RU', {}]
...
box.space._collation.index.name:delete{'nothing'} -- allowed
---
@@ -480,24 +480,28 @@ _ = box.space._collation.index.name:delete{'test'} -- ok
...
box.space._collation:auto_increment{'test', 0, 'ICU', 'ru_RU', setmap{}}
---
-- [3, 'test', 0, 'ICU', 'ru_RU', {}]
+- [4, 'test', 0, 'ICU', 'ru_RU', {}]
...
box.space._collation:select{}
---
-- - [1, 'unicode', 1, 'ICU', '', {}]
+- - [0, 'none', 1, 'BINARY', '', {}]
+ - [1, 'unicode', 1, 'ICU', '', {}]
- [2, 'unicode_ci', 1, 'ICU', '', {'strength': 'primary'}]
- - [3, 'test', 0, 'ICU', 'ru_RU', {}]
+ - [3, 'binary', 1, 'BINARY', '', {}]
+ - [4, 'test', 0, 'ICU', 'ru_RU', {}]
...
test_run:cmd('restart server default')
box.space._collation:select{}
---
-- - [1, 'unicode', 1, 'ICU', '', {}]
+- - [0, 'none', 1, 'BINARY', '', {}]
+ - [1, 'unicode', 1, 'ICU', '', {}]
- [2, 'unicode_ci', 1, 'ICU', '', {'strength': 'primary'}]
- - [3, 'test', 0, 'ICU', 'ru_RU', {}]
+ - [3, 'binary', 1, 'BINARY', '', {}]
+ - [4, 'test', 0, 'ICU', 'ru_RU', {}]
...
box.space._collation.index.name:delete{'test'}
---
-- [3, 'test', 0, 'ICU', 'ru_RU', {}]
+- [4, 'test', 0, 'ICU', 'ru_RU', {}]
...
--
-- gh-3290: expose ICU into Lua. It uses built-in collations, that
diff --git a/test/box/misc.result b/test/box/misc.result
index 3ada82fb7..ea3cd8805 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -498,6 +498,7 @@ t;
169: box.error.NO_SUCH_CONSTRAINT
170: box.error.CONSTRAINT_EXISTS
171: box.error.SQL_TYPE_MISMATCH
+ 172: box.error.DROP_COLLATION
...
test_run:cmd("setopt delimiter ''");
---
diff --git a/test/box/net.box.result b/test/box/net.box.result
index 57eebfbfc..4f6979d5b 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -2654,7 +2654,7 @@ c.space.test.index.sk.parts
---
- - type: string
is_nullable: false
- collation_id: 3
+ collation_id: 4
fieldno: 1
...
c:close()
diff --git a/test/sql-tap/collation.test.lua b/test/sql-tap/collation.test.lua
index eb4f43a90..a4684b956 100755
--- a/test/sql-tap/collation.test.lua
+++ b/test/sql-tap/collation.test.lua
@@ -17,11 +17,13 @@ test:do_execsql_test(
prefix.."0.2",
"pragma collation_list",
{
- 0,"unicode",
- 1,"unicode_ci",
- 2,"unicode_numeric",
- 3,"unicode_numeric_s2",
- 4,"unicode_tur_s2"
+ 0,"none",
+ 1,"unicode",
+ 2,"unicode_ci",
+ 3,"binary",
+ 4,"unicode_numeric",
+ 5,"unicode_numeric_s2",
+ 6,"unicode_tur_s2"
}
)
diff --git a/test/sql-tap/distinct.test.lua b/test/sql-tap/distinct.test.lua
index 26a4aace2..a83b37829 100755
--- a/test/sql-tap/distinct.test.lua
+++ b/test/sql-tap/distinct.test.lua
@@ -126,7 +126,7 @@ local data = {
{"14.2", 1, "SELECT DISTINCT a, d COLLATE \"unicode_ci\" FROM t4"},
{"15 ", 0, "SELECT DISTINCT a, d COLLATE binary FROM t1"},
{"16.1", 0, "SELECT DISTINCT a, b, c COLLATE binary FROM t1"},
- {"16.2", 1, "SELECT DISTINCT a, b, c COLLATE binary FROM t4"},
+ {"16.2", 0, "SELECT DISTINCT a, b, c COLLATE binary FROM t4"},
{"17", 0, --{ \/* Technically, it would be possible to detect that DISTINCT\n ** is a no-op in cases like the following. But SQLite does not\n ** do so. *\/\n
"SELECT DISTINCT t1.id FROM t1, t2 WHERE t1.id=t2.x" },
{"18 ", 1, "SELECT DISTINCT c1, c2 FROM t3"},
diff --git a/test/sql-tap/in3.test.lua b/test/sql-tap/in3.test.lua
index 83139a3e4..ad4f4fc2c 100755
--- a/test/sql-tap/in3.test.lua
+++ b/test/sql-tap/in3.test.lua
@@ -209,7 +209,7 @@ test:do_test(
return exec_neph(" SELECT a FROM t1 WHERE a COLLATE binary IN (SELECT a FROM t1) ")
end, {
-- <in3-1.15>
- 0, 1, 3, 5
+ 1, 1, 3, 5
-- </in3-1.15>
})
diff --git a/test/sql-tap/index3.test.lua b/test/sql-tap/index3.test.lua
index 4f950be09..6a9911ada 100755
--- a/test/sql-tap/index3.test.lua
+++ b/test/sql-tap/index3.test.lua
@@ -59,7 +59,7 @@ test:do_execsql_test(
CREATE TABLE t1(a INT , b TEXT , c INT , d INT , e INT ,
PRIMARY KEY(a), UNIQUE(b COLLATE "unicode_ci" DESC));
CREATE INDEX t1c ON t1(c);
- CREATE INDEX t1d ON t1(d COLLATE binary ASC);
+ CREATE INDEX t1d ON t1(d);
WITH RECURSIVE c(x) AS (VALUES(1) UNION SELECT x+1 FROM c WHERE x<30)
INSERT INTO t1(a,b,c,d,e)
SELECT x, printf('ab%03xxy',x), x, x, x FROM c;
diff --git a/test/sql-tap/where2.test.lua b/test/sql-tap/where2.test.lua
index a2b60e347..9089c97f9 100755
--- a/test/sql-tap/where2.test.lua
+++ b/test/sql-tap/where2.test.lua
@@ -183,7 +183,7 @@ test:do_test(
]])
end, {
-- <where2-2.1>
- 85, 6, 7396, 7402, "nosort", "T1", "*"
+ 85, 6, 7396, 7402, "sort", "T1", "*"
-- </where2-2.1>
})
diff --git a/test/sql/collation.result b/test/sql/collation.result
index 419e469f7..3df4cfa70 100644
--- a/test/sql/collation.result
+++ b/test/sql/collation.result
@@ -107,6 +107,33 @@ cn:execute('select 1 limit ? collate not_exist', {1})
cn:close()
---
...
+-- Explicitly set BINARY collation is predifined and has ID.
+--
+box.sql.execute("CREATE TABLE t (id INT PRIMARY KEY, a TEXT, b TEXT COLLATE \"binary\");")
+---
+...
+box.space.T:format()[2]['collation']
+---
+- null
+...
+box.space.T:format()[3]['collation']
+---
+- 3
+...
+box.sql.execute("DROP TABLE t;")
+---
+...
+-- Collation with id == 0 is "none". It used to unify interaction
+-- with collation interface. It also can't be dropped.
+--
+box.space._collation:select{0}
+---
+- - [0, 'none', 1, 'BINARY', '', {}]
+...
+box.space._collation:delete{0}
+---
+- error: 'Can''t drop collation none : system collation'
+...
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
---
...
diff --git a/test/sql/collation.test.lua b/test/sql/collation.test.lua
index da577c910..61df33a95 100644
--- a/test/sql/collation.test.lua
+++ b/test/sql/collation.test.lua
@@ -42,4 +42,18 @@ cn = remote.connect(box.cfg.listen)
cn:execute('select 1 limit ? collate not_exist', {1})
cn:close()
+
+-- Explicitly set BINARY collation is predifined and has ID.
+--
+box.sql.execute("CREATE TABLE t (id INT PRIMARY KEY, a TEXT, b TEXT COLLATE \"binary\");")
+box.space.T:format()[2]['collation']
+box.space.T:format()[3]['collation']
+box.sql.execute("DROP TABLE t;")
+
+-- Collation with id == 0 is "none". It used to unify interaction
+-- with collation interface. It also can't be dropped.
+--
+box.space._collation:select{0}
+box.space._collation:delete{0}
+
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
--
2.15.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] [PATCH v2 3/3] sql: change collation compatibility rules
2018-11-13 0:07 [tarantool-patches] [PATCH v2 0/3] Change collation compatibility rules according to ANSI SQL Nikita Pettik
2018-11-13 0:07 ` [tarantool-patches] [PATCH v2 1/3] sql: do not add explicit <COLLATE "BINARY"> clause Nikita Pettik
2018-11-13 0:07 ` [tarantool-patches] [PATCH v2 2/3] Introduce "none" and "binary" collations Nikita Pettik
@ 2018-11-13 0:07 ` Nikita Pettik
2018-11-13 12:02 ` [tarantool-patches] " Vladislav Shpilevoy
2018-11-15 11:24 ` [tarantool-patches] Re: [PATCH v2 0/3] Change collation compatibility rules according to ANSI SQL Kirill Yukhin
3 siblings, 1 reply; 12+ messages in thread
From: Nikita Pettik @ 2018-11-13 0:07 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
Before this patch our SQL implementation relied on strange rules
when comparing strings with different collations:
- if either operand has an explicit collating function assignment using
the postfix COLLATE operator, then the explicit collating function is
used for comparison, with precedence to the collating function of the
left operand;
- if either operand is a column, then the collating function of that
column is used with precedence to the left operand.
The main concern in this implementation is that collation of the left
operand is forced over right one (even if both operands come with
explicit COLLATE clause). This contradicts ANSI SQL and seems to be
quite misleading, since if user simple swap LHS and RHS - result of
query may change.
Lets introduce restrictions concerning collations compatibilities.
Collations of LHS and RHS are compatible (i.e. no "Illegal mix of
collations" is thrown) if:
- one of collations is mentioned alongside with explicit COLLATE clause,
which forces this collation over another one. It is allowed to have
the same forced collations;
- both collations are derived from table columns and they are the same;
- one collation is derived from table column and another one is not
specified (i.e. COLL_NONE).
The compound SELECT operators UNION, INTERSECT and EXCEPT perform implicit
comparisons between values. Hence, all new rules stated above are
applied to parts of compound SELECT. Otherwise, an error is raised.
In other words, before current patch queries like below were allowed:
SELECT 'abc' COLLATE binary UNION SELECT 'ABC' COLLATE "unicode_ci";
---
- - ['ABC']
- ['abc']
...
If we swap collations, we will get another result:
SELECT 'ABC' COLLATE "unicode_ci" UNION SELECT 'abc' COLLATE BINARY
---
- - ['abc']
...
Now such queries are illegal.
Closes #3185
---
src/box/errcode.h | 1 +
src/box/sql/expr.c | 71 +++++++++++++++++++------
src/box/sql/select.c | 107 +++++++++++++++++++++-----------------
src/box/sql/sqliteInt.h | 28 +++++++++-
src/box/sql/where.c | 13 +++--
src/box/sql/whereexpr.c | 13 ++---
test/box/misc.result | 1 +
test/sql-tap/e_select1.test.lua | 12 ++---
test/sql-tap/in4.test.lua | 2 +-
test/sql-tap/join.test.lua | 2 +-
test/sql-tap/tkt3493.test.lua | 4 +-
test/sql-tap/transitive1.test.lua | 4 +-
test/sql-tap/with1.test.lua | 2 +-
test/sql/collation.result | 63 ++++++++++++++++++++++
test/sql/collation.test.lua | 22 ++++++++
15 files changed, 250 insertions(+), 95 deletions(-)
diff --git a/src/box/errcode.h b/src/box/errcode.h
index 18ffdf3d5..3b4f35c58 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -225,6 +225,7 @@ struct errcode_record {
/*170 */_(ER_CONSTRAINT_EXISTS, "Constraint %s already exists") \
/*171 */_(ER_SQL_TYPE_MISMATCH, "Type mismatch: can not convert %s to %s") \
/*172 */_(ER_DROP_COLLATION, "Can't drop collation %s : %s") \
+ /*173 */_(ER_ILLEGAL_COLLATION_MIX, "Illegal mix of collations") \
/*
* !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index e52cd6407..50de9ba6d 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -190,10 +190,10 @@ sqlite3ExprSkipCollate(Expr * pExpr)
}
struct coll *
-sql_expr_coll(Parse *parse, Expr *p, bool *is_found, uint32_t *coll_id)
+sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id)
{
struct coll *coll = NULL;
- *is_found = false;
+ *is_explicit_coll = false;
*coll_id = 0;
while (p != NULL) {
int op = p->op;
@@ -206,7 +206,7 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_found, uint32_t *coll_id)
if (op == TK_COLLATE ||
(op == TK_REGISTER && p->op2 == TK_COLLATE)) {
coll = sql_get_coll_seq(parse, p->u.zToken, coll_id);
- *is_found = true;
+ *is_explicit_coll = true;
break;
}
if ((op == TK_AGG_COLUMN || op == TK_COLUMN ||
@@ -219,7 +219,6 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_found, uint32_t *coll_id)
if (j >= 0) {
coll = sql_column_collation(p->space_def, j,
coll_id);
- *is_found = true;
}
break;
}
@@ -342,25 +341,63 @@ binaryCompareP5(Expr * pExpr1, Expr * pExpr2, int jumpIfNull)
return aff;
}
+int
+collations_check_compatibility(uint32_t lhs_id, bool is_lhs_forced,
+ uint32_t rhs_id, bool is_rhs_forced,
+ uint32_t *res_id)
+{
+ assert(res_id != NULL);
+ if (is_lhs_forced && is_rhs_forced) {
+ if (lhs_id != rhs_id)
+ goto illegal_collation_mix;
+ }
+ if (is_lhs_forced) {
+ *res_id = lhs_id;
+ return 0;
+ }
+ if (is_rhs_forced) {
+ *res_id = rhs_id;
+ return 0;
+ }
+ if (lhs_id != rhs_id) {
+ if (lhs_id == 0) {
+ *res_id = rhs_id;
+ return 0;
+ }
+ if (rhs_id == 0) {
+ *res_id = lhs_id;
+ return 0;
+ }
+ goto illegal_collation_mix;
+ }
+ *res_id = lhs_id;
+ return 0;
+illegal_collation_mix:
+ diag_set(ClientError, ER_ILLEGAL_COLLATION_MIX);
+ return -1;
+}
+
struct coll *
sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right,
uint32_t *coll_id)
{
- struct coll *coll;
- bool is_found;
assert(left != NULL);
- if ((left->flags & EP_Collate) != 0) {
- coll = sql_expr_coll(parser, left, &is_found, coll_id);
- } else if (right != NULL && (right->flags & EP_Collate) != 0) {
- coll = sql_expr_coll(parser, right, &is_found, coll_id);
- } else {
- coll = sql_expr_coll(parser, left, &is_found, coll_id);
- if (! is_found) {
- coll = sql_expr_coll(parser, right, &is_found,
- coll_id);
- }
+ bool is_lhs_forced;
+ bool is_rhs_forced;
+ uint32_t lhs_coll_id;
+ uint32_t rhs_coll_id;
+ struct coll *lhs_coll = sql_expr_coll(parser, left, &is_lhs_forced,
+ &lhs_coll_id);
+ struct coll *rhs_coll = sql_expr_coll(parser, right, &is_rhs_forced,
+ &rhs_coll_id);
+ if (collations_check_compatibility(lhs_coll_id, is_lhs_forced,
+ rhs_coll_id, is_rhs_forced,
+ coll_id) != 0) {
+ parser->rc = SQL_TARANTOOL_ERROR;
+ parser->nErr++;
+ return NULL;
}
- return coll;
+ return *coll_id == rhs_coll_id ? rhs_coll : lhs_coll;;
}
/*
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index cea453f08..ab0376a1d 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1048,7 +1048,7 @@ selectInnerLoop(Parse * pParse, /* The parser context */
regPrev + i);
VdbeCoverage(v);
}
- if (is_found) {
+ if (coll != NULL) {
sqlite3VdbeChangeP4(v, -1,
(const char *)coll,
P4_COLLSEQ);
@@ -1961,8 +1961,9 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse, /* Parsing contexts */
pTab->def->fields[i].type = sql_affinity_to_field_type(affinity);
bool is_found;
uint32_t coll_id;
+
if (pTab->def->fields[i].coll_id == 0 &&
- sql_expr_coll(pParse, p, &is_found, &coll_id) && is_found)
+ sql_expr_coll(pParse, p, &is_found, &coll_id) && coll_id != 0)
pTab->def->fields[i].coll_id = coll_id;
}
}
@@ -2150,47 +2151,51 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
}
#ifndef SQLITE_OMIT_COMPOUND_SELECT
-static struct coll *
-multi_select_coll_seq_r(Parse *parser, Select *p, int n, bool *is_found,
- uint32_t *coll_id)
+/**
+ * This function determines resulting collation sequence for
+ * @n-th column of the result set for the compound SELECT
+ * statement. Since compound SELECT performs implicit comparisons
+ * between values, all parts of compound queries must use
+ * the same collation. Otherwise, an error is raised.
+ *
+ * @param parser Parse context.
+ * @param p Select meta-information.
+ * @param n Column number of the result set.
+ * @param is_forced_coll Used if we fall into recursion.
+ * For most-outer call it is unused. Used to indicate that
+ * explicit COLLATE clause is used.
+ * @retval Id of collation to be used during string comparison.
+ */
+static uint32_t
+multi_select_coll_seq(struct Parse *parser, struct Select *p, int n,
+ bool *is_forced_coll)
{
- struct coll *coll;
+ bool is_prior_forced = false;
+ bool is_current_forced;
+ uint32_t prior_coll_id = 0;
+ uint32_t current_coll_id;
if (p->pPrior != NULL) {
- coll = multi_select_coll_seq_r(parser, p->pPrior, n, is_found,
- coll_id);
- } else {
- coll = NULL;
- *coll_id = 0;
+ prior_coll_id = multi_select_coll_seq(parser, p->pPrior, n,
+ &is_prior_forced);
}
- assert(n >= 0);
- /* iCol must be less than p->pEList->nExpr. Otherwise an error would
- * have been thrown during name resolution and we would not have gotten
- * this far
+ /*
+ * Column number must be less than p->pEList->nExpr.
+ * Otherwise an error would have been thrown during name
+ * resolution and we would not have got this far.
*/
- if (!*is_found && ALWAYS(n < p->pEList->nExpr)) {
- coll = sql_expr_coll(parser, p->pEList->a[n].pExpr, is_found,
- coll_id);
+ assert(n >= 0 && n < p->pEList->nExpr);
+ sql_expr_coll(parser, p->pEList->a[n].pExpr, &is_current_forced,
+ ¤t_coll_id);
+ uint32_t res_coll_id;
+ if (collations_check_compatibility(prior_coll_id, is_prior_forced,
+ current_coll_id, is_current_forced,
+ &res_coll_id) != 0) {
+ parser->rc = SQL_TARANTOOL_ERROR;
+ parser->nErr++;
+ return 0;
}
- return coll;
-}
-
-/**
- * The collating sequence for the compound select is taken from the
- * left-most term of the select that has a collating sequence.
- * @param parser Parser.
- * @param p Select.
- * @param n Column number.
- * @param[out] coll_id Collation identifer.
- * @retval The appropriate collating sequence for the n-th column
- * of the result set for the compound-select statement
- * "p".
- * @retval NULL The column has no default collating sequence.
- */
-static inline struct coll *
-multi_select_coll_seq(Parse *parser, Select *p, int n, uint32_t *coll_id)
-{
- bool is_found = false;
- return multi_select_coll_seq_r(parser, p, n, &is_found, coll_id);
+ *is_forced_coll = (is_prior_forced || is_current_forced);
+ return res_coll_id;
}
/**
@@ -2227,12 +2232,13 @@ sql_multiselect_orderby_to_key_info(struct Parse *parse, struct Select *s,
struct ExprList_item *item = &order_by->a[i];
struct Expr *term = item->pExpr;
uint32_t id;
+ bool unused;
if ((term->flags & EP_Collate) != 0) {
- bool is_found = false;
- sql_expr_coll(parse, term, &is_found, &id);
+ sql_expr_coll(parse, term, &unused, &id);
} else {
- multi_select_coll_seq(parse, s,
- item->u.x.iOrderByCol - 1, &id);
+ id = multi_select_coll_seq(parse, s,
+ item->u.x.iOrderByCol - 1,
+ &unused);
if (id != 0) {
const char *name = coll_by_id(id)->name;
order_by->a[i].pExpr =
@@ -2894,9 +2900,10 @@ multiSelect(Parse * pParse, /* Parsing context */
struct sql_key_info *key_info = sql_key_info_new(db, nCol);
if (key_info == NULL)
goto multi_select_end;
+ bool unused;
for (int i = 0; i < nCol; i++) {
- multi_select_coll_seq(pParse, p, i,
- &key_info->parts[i].coll_id);
+ key_info->parts[i].coll_id =
+ multi_select_coll_seq(pParse, p, i, &unused);
}
for (struct Select *pLoop = p; pLoop; pLoop = pLoop->pPrior) {
@@ -3321,10 +3328,12 @@ multiSelectOrderBy(Parse * pParse, /* Parsing context */
pParse->nMem += expr_count + 1;
sqlite3VdbeAddOp2(v, OP_Integer, 0, regPrev);
key_info_dup = sql_key_info_new(db, expr_count);
+ bool unused;
if (key_info_dup != NULL) {
for (int i = 0; i < expr_count; i++) {
- multi_select_coll_seq(pParse, p, i,
- &key_info_dup->parts[i].coll_id);
+ key_info_dup->parts[i].coll_id =
+ multi_select_coll_seq(pParse, p, i,
+ &unused);
}
}
}
@@ -5300,12 +5309,12 @@ updateAccumulator(Parse * pParse, AggInfo * pAggInfo)
struct ExprList_item *pItem;
int j;
assert(pList != 0); /* pList!=0 if pF->pFunc has NEEDCOLL */
- bool is_found = false;
+ bool unused;
uint32_t id;
- for (j = 0, pItem = pList->a; !is_found && j < nArg;
+ for (j = 0, pItem = pList->a; coll == NULL && j < nArg;
j++, pItem++) {
coll = sql_expr_coll(pParse, pItem->pExpr,
- &is_found, &id);
+ &unused, &id);
}
if (regHit == 0 && pAggInfo->nAccumulator)
regHit = ++pParse->nMem;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 1999ff568..cfe87c62a 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4330,13 +4330,14 @@ const char *sqlite3ErrStr(int);
*
* @param parse Parsing context.
* @param expr Expression to scan.
- * @param[out] is_found Flag set if collation was found.
+ * @param[out] is_found Flag set if explicit COLLATE clause is used.
* @param[out] coll_id Collation identifier.
*
* @retval Pointer to collation.
*/
struct coll *
-sql_expr_coll(Parse * pParse, Expr * pExpr, bool *is_found, uint32_t *coll_id);
+sql_expr_coll(Parse * pParse, Expr * pExpr, bool *is_explicit_coll,
+ uint32_t *coll_id);
Expr *sqlite3ExprAddCollateToken(Parse * pParse, Expr *, const Token *, int);
Expr *sqlite3ExprAddCollateString(Parse *, Expr *, const char *);
@@ -4631,6 +4632,29 @@ int sqlite3VdbeParameterIndex(Vdbe *, const char *, int);
int sqlite3TransferBindings(sqlite3_stmt *, sqlite3_stmt *);
int sqlite3Reprepare(Vdbe *);
void sqlite3ExprListCheckLength(Parse *, ExprList *, const char *);
+
+/**
+ * This function verifies that two collations (to be more precise
+ * their ids) are compatible. In terms of SQL ANSI they are
+ * compatible if:
+ * - one of collations is mentioned alongside with explicit
+ * COLLATE clause, which forces this collation over another
+ * one. It is allowed to have the same forced collations;
+ * - both collations are derived from table columns and they
+ * are the same;
+ * - one collation is derived from table column and another
+ * one is not specified (i.e. COLL_NONE);
+ * In all other cases they are not accounted to be compatible
+ * and error should be raised.
+ * Collation to be used in comparison operator is returned
+ * via @res_id: in case one of collations is absent, then
+ * the second one is utilized.
+ */
+int
+collations_check_compatibility(uint32_t lhs_id, bool is_lhs_forced,
+ uint32_t rhs_id, bool is_rhs_forced,
+ uint32_t *res_id);
+
/**
* Return a pointer to the collation sequence that should be used
* by a binary comparison operator comparing left and right.
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index 1db4db874..44c52d67f 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -304,11 +304,11 @@ whereScanNext(WhereScan * pScan)
Parse *pParse =
pWC->pWInfo->pParse;
assert(pX->pLeft);
- uint32_t id;
+ uint32_t unused;
struct coll *coll =
sql_binary_compare_coll_seq(
pParse, pX->pLeft,
- pX->pRight, &id);
+ pX->pRight, &unused);
if (coll != pScan->coll)
continue;
}
@@ -559,7 +559,7 @@ findIndexCol(Parse * pParse, /* Parse context */
struct coll *coll = sql_expr_coll(pParse,
pList->a[i].pExpr,
&is_found, &id);
- if (is_found && coll == part_to_match->coll)
+ if (coll == part_to_match->coll)
return i;
}
}
@@ -2288,8 +2288,8 @@ whereRangeVectorLen(Parse * pParse, /* Parsing context */
sqlite3TableColumnAffinity(space->def, pLhs->iColumn);
if (aff != idxaff)
break;
- uint32_t id;
- pColl = sql_binary_compare_coll_seq(pParse, pLhs, pRhs, &id);
+ uint32_t unused;
+ pColl = sql_binary_compare_coll_seq(pParse, pLhs, pRhs, &unused);
if (pColl == 0)
break;
if (idx_def->key_def->parts[i + nEq].coll != pColl)
@@ -3408,8 +3408,7 @@ wherePathSatisfiesOrderBy(WhereInfo * pWInfo, /* The WHERE clause */
&is_found, &id);
struct coll *idx_coll =
idx_def->key_def->parts[j].coll;
- if (is_found &&
- coll != idx_coll)
+ if (coll != idx_coll)
continue;
}
isMatch = 1;
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index 9fa6ce15d..e05967554 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -168,7 +168,7 @@ exprCommute(Parse * pParse, Expr * pExpr)
bool is_found;
uint32_t id;
sql_expr_coll(pParse, pExpr->pLeft, &is_found, &id);
- if (is_found) {
+ if (id != 0) {
/*
* Neither X nor Y have COLLATE
* operators, but X has a
@@ -837,15 +837,16 @@ termIsEquivalence(Parse * pParse, Expr * pExpr)
) {
return 0;
}
- uint32_t id;
+ uint32_t unused;
struct coll *coll1 =
sql_binary_compare_coll_seq(pParse, pExpr->pLeft, pExpr->pRight,
- &id);
+ &unused);
if (coll1 == NULL)
return 1;
- bool unused;
- coll1 = sql_expr_coll(pParse, pExpr->pLeft, &unused, &id);
- struct coll *coll2 = sql_expr_coll(pParse, pExpr->pRight, &unused, &id);
+ bool unused1;
+ coll1 = sql_expr_coll(pParse, pExpr->pLeft, &unused1, &unused);
+ struct coll *coll2 = sql_expr_coll(pParse, pExpr->pRight, &unused1,
+ &unused);
return coll1 != NULL && coll1 == coll2;
}
diff --git a/test/box/misc.result b/test/box/misc.result
index ea3cd8805..4401fc1b6 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -499,6 +499,7 @@ t;
170: box.error.CONSTRAINT_EXISTS
171: box.error.SQL_TYPE_MISMATCH
172: box.error.DROP_COLLATION
+ 173: box.error.ILLEGAL_COLLATION_MIX
...
test_run:cmd("setopt delimiter ''");
---
diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
index 47fb7a809..e9f29465c 100755
--- a/test/sql-tap/e_select1.test.lua
+++ b/test/sql-tap/e_select1.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(525)
+test:plan(523)
--!./tcltestrunner.lua
-- 2010 July 16
@@ -1588,12 +1588,10 @@ test:do_select_tests(
{"1", "SELECT 'abc' UNION SELECT 'ABC'", {"ABC", "abc"}},
{"2", "SELECT 'abc' COLLATE \"unicode_ci\" UNION SELECT 'ABC'", {"ABC" }},
{"3", "SELECT 'abc' UNION SELECT 'ABC' COLLATE \"unicode_ci\"", {"ABC" }},
- {"4", "SELECT 'abc' COLLATE binary UNION SELECT 'ABC' COLLATE \"unicode_ci\"", {"ABC", "abc"}},
- {"5", "SELECT 'abc' COLLATE \"unicode_ci\" UNION SELECT 'ABC' COLLATE binary", {"ABC" }},
- {"6", "SELECT a FROM y1 UNION SELECT b FROM y1", {"abc" }},
- {"7", "SELECT b FROM y1 UNION SELECT a FROM y1", {"Abc", "abc"}},
- {"8", "SELECT a FROM y1 UNION SELECT c FROM y1", {"aBC" }},
- {"9", "SELECT a FROM y1 UNION SELECT c COLLATE binary FROM y1", {"aBC" }},
+ {"4", "SELECT 'abc' COLLATE binary UNION SELECT 'ABC'", {"ABC", "abc"}},
+ {"5", "SELECT 'abc' COLLATE \"unicode_ci\" UNION SELECT 'ABC'", {"ABC" }},
+ {"6", "SELECT a FROM y1 UNION SELECT c FROM y1", {"aBC" }},
+ {"7", "SELECT a FROM y1 UNION SELECT c COLLATE binary FROM y1", {"Abc", "aBC" }},
})
-- EVIDENCE-OF: R-32706-07403 No affinity transformations are applied to
diff --git a/test/sql-tap/in4.test.lua b/test/sql-tap/in4.test.lua
index ef426b092..b029535af 100755
--- a/test/sql-tap/in4.test.lua
+++ b/test/sql-tap/in4.test.lua
@@ -531,7 +531,7 @@ test:do_execsql_test(
SELECT c FROM t4a WHERE a=b ORDER BY c;
]], {
-- <in4-4.1>
- 3
+ 1, 3
-- </in4-4.1>
})
diff --git a/test/sql-tap/join.test.lua b/test/sql-tap/join.test.lua
index 4e4ec6422..d8a4f2e7f 100755
--- a/test/sql-tap/join.test.lua
+++ b/test/sql-tap/join.test.lua
@@ -1003,7 +1003,7 @@ test:do_execsql_test(
SELECT a,b FROM t2 NATURAL JOIN t1
]], {
-- <join-11.7>
- "two", 2
+ "one", 1, "two", 2
-- </join-11.7>
})
diff --git a/test/sql-tap/tkt3493.test.lua b/test/sql-tap/tkt3493.test.lua
index 26ca2271b..7eb4fd726 100755
--- a/test/sql-tap/tkt3493.test.lua
+++ b/test/sql-tap/tkt3493.test.lua
@@ -246,7 +246,7 @@ test:do_execsql_test(
test:do_execsql_test(
"tkt3493-3.1",
[[
- CREATE TABLE t2(a TEXT COLLATE "unicode_ci" PRIMARY KEY, b TEXT COLLATE BINARY);
+ CREATE TABLE t2(a TEXT COLLATE "unicode_ci" PRIMARY KEY, b TEXT);
INSERT INTO t2 VALUES('aBc', 'DeF');
]], {
-- <tkt3493-3.1>
@@ -304,7 +304,7 @@ test:do_execsql_test(
SELECT b>a FROM t2 GROUP BY a, b
]], {
-- <tkt3493-3.3.3>
- 0
+ 1
-- </tkt3493-3.3.3>
})
diff --git a/test/sql-tap/transitive1.test.lua b/test/sql-tap/transitive1.test.lua
index 178fd9da6..8baad49c2 100755
--- a/test/sql-tap/transitive1.test.lua
+++ b/test/sql-tap/transitive1.test.lua
@@ -503,7 +503,7 @@ test:do_execsql_test(
SELECT * FROM c1 WHERE x=y AND z=y AND z='abc';
]], {
-- <transitive1-570>
-
+ 1, "ABC", "ABC", "abc"
-- </transitive1-570>
})
@@ -516,7 +516,7 @@ test:do_execsql_test(
SELECT * FROM c1 WHERE x=y AND z=y AND z='abc';
]], {
-- <transitive1-570eqp>
- "/SEARCH TABLE C1 USING COVERING INDEX C1X/"
+ "/SCAN TABLE C1/"
-- </transitive1-570eqp>
})
diff --git a/test/sql-tap/with1.test.lua b/test/sql-tap/with1.test.lua
index faa99811c..43c913bbd 100755
--- a/test/sql-tap/with1.test.lua
+++ b/test/sql-tap/with1.test.lua
@@ -880,7 +880,7 @@ test:do_execsql_test("10.8.4.2", [[
SELECT a FROM tst UNION ALL SELECT b COLLATE "unicode_ci" FROM tst ORDER BY 1;
]], {
-- <10.8.4.2>
- "A", "B", "C", "a", "b", "c"
+ "a", "A", "b", "B", "c", "C"
-- </10.8.4.2>
})
diff --git a/test/sql/collation.result b/test/sql/collation.result
index 3df4cfa70..c6bea382b 100644
--- a/test/sql/collation.result
+++ b/test/sql/collation.result
@@ -134,6 +134,69 @@ box.space._collation:delete{0}
---
- error: 'Can''t drop collation none : system collation'
...
+-- gh-3185: collations of LHS and RHS must be compatible.
+--
+box.sql.execute("CREATE TABLE t (id INT PRIMARY KEY, a TEXT, b TEXT COLLATE BINARY, c TEXT COLLATE \"unicode_ci\");")
+---
+...
+box.sql.execute("SELECT * FROM t WHERE a = b;")
+---
+- []
+...
+box.sql.execute("SELECT * FROM t WHERE a COLLATE BINARY = b;")
+---
+- []
+...
+box.sql.execute("SELECT * FROM t WHERE b = c;")
+---
+- error: Illegal mix of collations
+...
+box.sql.execute("SELECT * FROM t WHERE b COLLATE BINARY = c;")
+---
+- []
+...
+box.sql.execute("SELECT * FROM t WHERE a = c;")
+---
+- []
+...
+box.sql.execute("SELECT * FROM t WHERE a COLLATE BINARY = c COLLATE \"unicode\";")
+---
+- error: Illegal mix of collations
+...
+-- Compound queries perform implicit comparisons between values.
+-- Hence, rules for collations compatibilities are the same.
+--
+box.sql.execute("SELECT 'abc' COLLATE binary UNION SELECT 'ABC' COLLATE \"unicode_ci\"")
+---
+- error: Illegal mix of collations
+...
+box.sql.execute("SELECT 'abc' COLLATE \"unicode_ci\" UNION SELECT 'ABC' COLLATE binary")
+---
+- error: Illegal mix of collations
+...
+box.sql.execute("SELECT c FROM t UNION SELECT b FROM t;")
+---
+- error: Illegal mix of collations
+...
+box.sql.execute("SELECT b FROM t UNION SELECT a FROM t;")
+---
+- []
+...
+box.sql.execute("SELECT a FROM t UNION SELECT c FROM t;")
+---
+- []
+...
+box.sql.execute("SELECT c COLLATE BINARY FROM t UNION SELECT a FROM t;")
+---
+- []
+...
+box.sql.execute("SELECT b COLLATE \"unicode\" FROM t UNION SELECT a FROM t;")
+---
+- []
+...
+box.sql.execute("DROP TABLE t;")
+---
+...
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
---
...
diff --git a/test/sql/collation.test.lua b/test/sql/collation.test.lua
index 61df33a95..00e5113b1 100644
--- a/test/sql/collation.test.lua
+++ b/test/sql/collation.test.lua
@@ -56,4 +56,26 @@ box.sql.execute("DROP TABLE t;")
box.space._collation:select{0}
box.space._collation:delete{0}
+-- gh-3185: collations of LHS and RHS must be compatible.
+--
+box.sql.execute("CREATE TABLE t (id INT PRIMARY KEY, a TEXT, b TEXT COLLATE BINARY, c TEXT COLLATE \"unicode_ci\");")
+box.sql.execute("SELECT * FROM t WHERE a = b;")
+box.sql.execute("SELECT * FROM t WHERE a COLLATE BINARY = b;")
+box.sql.execute("SELECT * FROM t WHERE b = c;")
+box.sql.execute("SELECT * FROM t WHERE b COLLATE BINARY = c;")
+box.sql.execute("SELECT * FROM t WHERE a = c;")
+box.sql.execute("SELECT * FROM t WHERE a COLLATE BINARY = c COLLATE \"unicode\";")
+
+-- Compound queries perform implicit comparisons between values.
+-- Hence, rules for collations compatibilities are the same.
+--
+box.sql.execute("SELECT 'abc' COLLATE binary UNION SELECT 'ABC' COLLATE \"unicode_ci\"")
+box.sql.execute("SELECT 'abc' COLLATE \"unicode_ci\" UNION SELECT 'ABC' COLLATE binary")
+box.sql.execute("SELECT c FROM t UNION SELECT b FROM t;")
+box.sql.execute("SELECT b FROM t UNION SELECT a FROM t;")
+box.sql.execute("SELECT a FROM t UNION SELECT c FROM t;")
+box.sql.execute("SELECT c COLLATE BINARY FROM t UNION SELECT a FROM t;")
+box.sql.execute("SELECT b COLLATE \"unicode\" FROM t UNION SELECT a FROM t;")
+
+box.sql.execute("DROP TABLE t;")
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
--
2.15.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH v2 2/3] Introduce "none" and "binary" collations
2018-11-13 0:07 ` [tarantool-patches] [PATCH v2 2/3] Introduce "none" and "binary" collations Nikita Pettik
@ 2018-11-13 12:02 ` Vladislav Shpilevoy
2018-11-13 22:37 ` n.pettik
0 siblings, 1 reply; 12+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-13 12:02 UTC (permalink / raw)
To: tarantool-patches, Nikita Pettik
Hi! Thanks for the fixes!
Please, remember to put branch/issue link
into a covering letter next time.
See 5 comments below, my fixes at the end
of the email and on the branch.
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 6d2c59bbc..2eb9f53e8 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -403,7 +403,7 @@ field_def_decode(struct field_def *field, const char **data,
> "nullable action properties", fieldno +
> TUPLE_INDEX_BASE));
> }
> - if (field->coll_id != COLL_NONE &&
> + if (field->coll_id != 0 &&
1. Why not to leave enum COLL_NONE, but change its value to 0?
I guess, it is a purpose of define/enum features - define a name
for a constant to be able to change the value under the hood. It
could have reduced the diff.
> field->type != FIELD_TYPE_STRING &&
> field->type != FIELD_TYPE_SCALAR &&
> field->type != FIELD_TYPE_ANY) {
> diff --git a/src/box/sql/callback.c b/src/box/sql/callback.c
> index 3cf3a835d..352745e0e 100644
> --- a/src/box/sql/callback.c
> +++ b/src/box/sql/callback.c
> @@ -42,13 +42,22 @@
> struct coll *
> sql_get_coll_seq(Parse *parser, const char *name, uint32_t *coll_id)
> {
> - if (name == NULL || strcasecmp(name, "binary") == 0) {
> - *coll_id = COLL_NONE;
> - return NULL;
> + if (name == NULL) {
> + *coll_id = 0;
> + return coll_by_id(0)->coll;
> }
> - struct coll_id *p = coll_by_name(name, strlen(name));
> + struct coll_id *p;
> + /*
> + * In SQL all identifiers should be uppercased, so
> + * to avoid mess lets simple search binary (since it is
> + * sort of "special" collation) ignoring case at all.
> + */
2. I am not sure if it should be so special. I think, we should
treat it just like any other collation. If tests have BINARY
uppercased, then they should be fixed, I guess.
> + if (strcasecmp(name, "binary") == 0)
> + p = coll_by_name("binary", strlen("binary"));
> + else
> + p = coll_by_name(name, strlen(name));
> if (p == NULL) {
> - *coll_id = COLL_NONE;
> + *coll_id = 0;
> sqlite3ErrorMsg(parser, "no such collation sequence: %s",
> name);
> return NULL;
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 8c34cbb3d..580cf1e60 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -506,7 +506,7 @@ case_type##ICUFunc(sqlite3_context *context, int argc, sqlite3_value **argv) \
> UErrorCode status = U_ZERO_ERROR; \
> struct coll *coll = sqlite3GetFuncCollSeq(context); \
> const char *locale = NULL; \
> - if (coll != NULL) { \
> + if (coll != NULL && coll->collator != NULL) { \
3. Why do you use coll->collator != 0 instead of coll->type == COLL_TYPE_ICU?
I do not think it is safe since in future we can move collator into a union,
which will not be NULL for a non-ICU collation.
> locale = ucol_getLocaleByType(coll->collator, \
> ULOC_VALID_LOCALE, &status); \
> } > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
> index 5bdf102e7..8e5aea51b 100644
> --- a/src/box/tuple_format.c
> +++ b/src/box/tuple_format.c
> @@ -40,7 +40,7 @@ static uint32_t formats_size = 0, formats_capacity = 0;
>
> static const struct tuple_field tuple_field_default = {
> FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false,
> - ON_CONFLICT_ACTION_NONE, NULL, COLL_NONE,
> + ON_CONFLICT_ACTION_DEFAULT, NULL, 0,
> };
4. Are you sure that changing on_conflict_action belongs
to this patch?
5. How about tests like this one?
box.schema.create_space('test', {format = {{1, 'unsigned'}, {2, 'string', collation = 'binary'}}})
I mean specifying collations via Lua. And what happens if I use collation = 'none'
in a space format or index parts? I think, it should throw an error, but it is up
to Kostja or Kirill.
==================================================
commit 58691e9f2f593771865264c6aaed93b36cc13c08
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date: Tue Nov 13 14:56:17 2018 +0300
Speculative review fixes
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 2eb9f53e8..474ef791e 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -403,7 +403,7 @@ field_def_decode(struct field_def *field, const char **data,
"nullable action properties", fieldno +
TUPLE_INDEX_BASE));
}
- if (field->coll_id != 0 &&
+ if (field->coll_id != COLL_NONE &&
field->type != FIELD_TYPE_STRING &&
field->type != FIELD_TYPE_SCALAR &&
field->type != FIELD_TYPE_ANY) {
@@ -2676,7 +2676,7 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event)
* under the hood. Hence, we can rely on the
* fact that "none" collation features id == 0.
*/
- if (old_id == 0) {
+ if (old_id == COLL_NONE) {
tnt_raise(ClientError, ER_DROP_COLLATION, "none",
"system collation");
}
diff --git a/src/box/coll_id.h b/src/box/coll_id.h
index 1b67a3f86..82e50d11b 100644
--- a/src/box/coll_id.h
+++ b/src/box/coll_id.h
@@ -37,6 +37,10 @@
extern "C" {
#endif /* defined(__cplusplus) */
+enum {
+ COLL_NONE = 0,
+};
+
struct coll_id_def;
struct coll;
diff --git a/src/box/field_def.c b/src/box/field_def.c
index 3e63e12a3..3a9ff3703 100644
--- a/src/box/field_def.c
+++ b/src/box/field_def.c
@@ -123,7 +123,7 @@ const struct field_def field_def_default = {
.name = NULL,
.is_nullable = false,
.nullable_action = ON_CONFLICT_ACTION_DEFAULT,
- .coll_id = 0,
+ .coll_id = COLL_NONE,
.default_value = NULL,
.default_value_expr = NULL
};
diff --git a/src/box/key_def.c b/src/box/key_def.c
index 6802489f1..3a560bb06 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -41,7 +41,7 @@ const char *sort_order_strs[] = { "asc", "desc", "undef" };
const struct key_part_def key_part_def_default = {
0,
field_type_MAX,
- 0,
+ COLL_NONE,
false,
ON_CONFLICT_ACTION_DEFAULT,
SORT_ORDER_ASC
@@ -174,7 +174,7 @@ key_def_new(const struct key_part_def *parts, uint32_t part_count)
for (uint32_t i = 0; i < part_count; i++) {
const struct key_part_def *part = &parts[i];
struct coll *coll = NULL;
- if (part->coll_id != 0) {
+ if (part->coll_id != COLL_NONE) {
struct coll_id *coll_id = coll_by_id(part->coll_id);
if (coll_id == NULL) {
diag_set(ClientError, ER_WRONG_INDEX_OPTIONS,
@@ -223,7 +223,7 @@ box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count)
key_def_set_part(key_def, item, fields[item],
(enum field_type)types[item],
ON_CONFLICT_ACTION_DEFAULT,
- NULL, 0, SORT_ORDER_ASC);
+ NULL, COLL_NONE, SORT_ORDER_ASC);
}
key_def_set_cmp(key_def);
return key_def;
@@ -319,7 +319,7 @@ key_def_sizeof_parts(const struct key_part_def *parts, uint32_t part_count)
for (uint32_t i = 0; i < part_count; i++) {
const struct key_part_def *part = &parts[i];
int count = 2;
- if (part->coll_id != 0)
+ if (part->coll_id != COLL_NONE)
count++;
if (part->is_nullable)
count++;
@@ -329,7 +329,7 @@ key_def_sizeof_parts(const struct key_part_def *parts, uint32_t part_count)
assert(part->type < field_type_MAX);
size += mp_sizeof_str(strlen(PART_OPT_TYPE));
size += mp_sizeof_str(strlen(field_type_strs[part->type]));
- if (part->coll_id != 0) {
+ if (part->coll_id != COLL_NONE) {
size += mp_sizeof_str(strlen(PART_OPT_COLLATION));
size += mp_sizeof_uint(part->coll_id);
}
@@ -348,7 +348,7 @@ key_def_encode_parts(char *data, const struct key_part_def *parts,
for (uint32_t i = 0; i < part_count; i++) {
const struct key_part_def *part = &parts[i];
int count = 2;
- if (part->coll_id != 0)
+ if (part->coll_id != COLL_NONE)
count++;
if (part->is_nullable)
count++;
@@ -361,7 +361,7 @@ key_def_encode_parts(char *data, const struct key_part_def *parts,
assert(part->type < field_type_MAX);
const char *type_str = field_type_strs[part->type];
data = mp_encode_str(data, type_str, strlen(type_str));
- if (part->coll_id != 0) {
+ if (part->coll_id != COLL_NONE) {
data = mp_encode_str(data, PART_OPT_COLLATION,
strlen(PART_OPT_COLLATION));
data = mp_encode_uint(data, part->coll_id);
@@ -431,7 +431,7 @@ key_def_decode_parts_166(struct key_part_def *parts, uint32_t part_count,
part->is_nullable = (part->fieldno < field_count ?
fields[part->fieldno].is_nullable :
key_part_def_default.is_nullable);
- part->coll_id = 0;
+ part->coll_id = COLL_NONE;
}
return 0;
}
@@ -488,7 +488,7 @@ key_def_decode_parts(struct key_part_def *parts, uint32_t part_count,
"index part: unknown field type");
return -1;
}
- if (part->coll_id != 0 &&
+ if (part->coll_id != COLL_NONE &&
part->type != FIELD_TYPE_STRING &&
part->type != FIELD_TYPE_SCALAR) {
diag_set(ClientError, ER_WRONG_INDEX_OPTIONS,
diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index e5e09b042..7cae436f1 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -299,7 +299,7 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i)
lua_pushboolean(L, key_part_is_nullable(part));
lua_setfield(L, -2, "is_nullable");
- if (part->coll_id != 0) {
+ if (part->coll_id != COLL_NONE) {
struct coll_id *coll_id =
coll_by_id(part->coll_id);
assert(coll_id != NULL);
diff --git a/src/box/sql.c b/src/box/sql.c
index 686d32335..caa66144f 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -379,7 +379,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
if (def != NULL && i < def->part_count)
part->coll_id = def->parts[i].coll_id;
else
- part->coll_id = 0;
+ part->coll_id = COLL_NONE;
}
struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
field_count);
@@ -1139,7 +1139,7 @@ sql_encode_table(struct region *region, struct Table *table, uint32_t *size)
struct field_def *field = &def->fields[i];
const char *default_str = field->default_value;
int base_len = 5;
- if (cid != 0)
+ if (cid != COLL_NONE)
base_len += 1;
if (default_str != NULL)
base_len += 1;
@@ -1160,7 +1160,7 @@ sql_encode_table(struct region *region, struct Table *table, uint32_t *size)
const char *action =
on_conflict_action_strs[def->fields[i].nullable_action];
mpstream_encode_str(&stream, action);
- if (cid != 0) {
+ if (cid != COLL_NONE) {
mpstream_encode_str(&stream, "collation");
mpstream_encode_uint(&stream, cid);
}
@@ -1285,13 +1285,13 @@ sql_encode_index_parts(struct region *region, const struct field_def *fields,
action_is_nullable(fields[col].nullable_action));
/* Do not decode default collation. */
uint32_t cid = part->coll_id;
- mpstream_encode_map(&stream, 5 + (cid != 0));
+ mpstream_encode_map(&stream, 5 + (cid != COLL_NONE));
mpstream_encode_str(&stream, "type");
mpstream_encode_str(&stream, field_type_strs[fields[col].type]);
mpstream_encode_str(&stream, "field");
mpstream_encode_uint(&stream, col);
- if (cid != 0) {
+ if (cid != COLL_NONE) {
mpstream_encode_str(&stream, "collation");
mpstream_encode_uint(&stream, cid);
}
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 929d20dbf..5b3348bd2 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2399,7 +2399,7 @@ index_fill_def(struct Parse *parse, struct index *index,
uint32_t coll_id;
if (expr->op == TK_COLLATE) {
sql_get_coll_seq(parse, expr->u.zToken, &coll_id);
- if (coll_id == 0 &&
+ if (coll_id == COLL_NONE &&
strcasecmp(expr->u.zToken, "binary") != 0) {
diag_set(ClientError, ER_NO_SUCH_COLLATION,
expr->u.zToken);
diff --git a/src/box/sql/callback.c b/src/box/sql/callback.c
index 352745e0e..927eb2643 100644
--- a/src/box/sql/callback.c
+++ b/src/box/sql/callback.c
@@ -43,8 +43,8 @@ struct coll *
sql_get_coll_seq(Parse *parser, const char *name, uint32_t *coll_id)
{
if (name == NULL) {
- *coll_id = 0;
- return coll_by_id(0)->coll;
+ *coll_id = COLL_NONE;
+ return coll_by_id(COLL_NONE)->coll;
}
struct coll_id *p;
/*
@@ -57,7 +57,7 @@ sql_get_coll_seq(Parse *parser, const char *name, uint32_t *coll_id)
else
p = coll_by_name(name, strlen(name));
if (p == NULL) {
- *coll_id = 0;
+ *coll_id = COLL_NONE;
sqlite3ErrorMsg(parser, "no such collation sequence: %s",
name);
return NULL;
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index e52cd6407..4d1c1a634 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -194,7 +194,7 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_found, uint32_t *coll_id)
{
struct coll *coll = NULL;
*is_found = false;
- *coll_id = 0;
+ *coll_id = COLL_NONE;
while (p != NULL) {
int op = p->op;
if (p->flags & EP_Generic)
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index cea453f08..dfa6ed8e0 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1345,7 +1345,7 @@ sql_key_info_new(sqlite3 *db, uint32_t part_count)
struct key_part_def *part = &key_info->parts[i];
part->fieldno = i;
part->type = FIELD_TYPE_SCALAR;
- part->coll_id = 0;
+ part->coll_id = COLL_NONE;
part->is_nullable = false;
part->nullable_action = ON_CONFLICT_ACTION_ABORT;
part->sort_order = SORT_ORDER_ASC;
@@ -1961,7 +1961,7 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse, /* Parsing contexts */
pTab->def->fields[i].type = sql_affinity_to_field_type(affinity);
bool is_found;
uint32_t coll_id;
- if (pTab->def->fields[i].coll_id == 0 &&
+ if (pTab->def->fields[i].coll_id == COLL_NONE &&
sql_expr_coll(pParse, p, &is_found, &coll_id) && is_found)
pTab->def->fields[i].coll_id = coll_id;
}
@@ -2160,7 +2160,7 @@ multi_select_coll_seq_r(Parse *parser, Select *p, int n, bool *is_found,
coll_id);
} else {
coll = NULL;
- *coll_id = 0;
+ *coll_id = COLL_NONE;
}
assert(n >= 0);
/* iCol must be less than p->pEList->nExpr. Otherwise an error would
@@ -2233,7 +2233,7 @@ sql_multiselect_orderby_to_key_info(struct Parse *parse, struct Select *s,
} else {
multi_select_coll_seq(parse, s,
item->u.x.iOrderByCol - 1, &id);
- if (id != 0) {
+ if (id != COLL_NONE) {
const char *name = coll_by_id(id)->name;
order_by->a[i].pExpr =
sqlite3ExprAddCollateString(parse, term,
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index 1db4db874..8c78c0c9b 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -2806,7 +2806,7 @@ whereLoopAddBtree(WhereLoopBuilder * pBuilder, /* WHERE clause information */
part.nullable_action = ON_CONFLICT_ACTION_ABORT;
part.is_nullable = false;
part.sort_order = SORT_ORDER_ASC;
- part.coll_id = 0;
+ part.coll_id = COLL_NONE;
struct key_def *key_def = key_def_new(&part, 1);
if (key_def == NULL) {
diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index 8e5aea51b..4a8905262 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -40,7 +40,7 @@ static uint32_t formats_size = 0, formats_capacity = 0;
static const struct tuple_field tuple_field_default = {
FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false,
- ON_CONFLICT_ACTION_DEFAULT, NULL, 0,
+ ON_CONFLICT_ACTION_DEFAULT, NULL, COLL_NONE,
};
/**
@@ -67,7 +67,7 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys,
format->fields[i].nullable_action = fields[i].nullable_action;
struct coll *coll = NULL;
uint32_t cid = fields[i].coll_id;
- if (cid != 0) {
+ if (cid != COLL_NONE) {
struct coll_id *coll_id = coll_by_id(cid);
if (coll_id == NULL) {
diag_set(ClientError,ER_WRONG_COLLATION_OPTIONS,
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH v2 3/3] sql: change collation compatibility rules
2018-11-13 0:07 ` [tarantool-patches] [PATCH v2 3/3] sql: change collation compatibility rules Nikita Pettik
@ 2018-11-13 12:02 ` Vladislav Shpilevoy
2018-11-13 22:37 ` n.pettik
0 siblings, 1 reply; 12+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-13 12:02 UTC (permalink / raw)
To: Nikita Pettik, tarantool-patches
Thanks for the fixes!
See 2 comments below, my fixes at the end
of the email and on the branch.
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index cea453f08..ab0376a1d 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -2150,47 +2151,51 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
> }
>
> #ifndef SQLITE_OMIT_COMPOUND_SELECT
> -static struct coll *
> -multi_select_coll_seq_r(Parse *parser, Select *p, int n, bool *is_found,
> - uint32_t *coll_id)
> +/**
> + * This function determines resulting collation sequence for
> + * @n-th column of the result set for the compound SELECT
> + * statement. Since compound SELECT performs implicit comparisons
> + * between values, all parts of compound queries must use
> + * the same collation. Otherwise, an error is raised.
> + *
> + * @param parser Parse context.
> + * @param p Select meta-information.
> + * @param n Column number of the result set.
> + * @param is_forced_coll Used if we fall into recursion.
> + * For most-outer call it is unused. Used to indicate that
> + * explicit COLLATE clause is used.
> + * @retval Id of collation to be used during string comparison.
> + */
> +static uint32_t
> +multi_select_coll_seq(struct Parse *parser, struct Select *p, int n,
> + bool *is_forced_coll)
1. I renamed this function into multi_select_coll_seq_r and
created a wrapper without is_forced_coll to remove some of
'bool unused' things. Apply or drop the fix, up to you.
> diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
> index 9fa6ce15d..e05967554 100644
> --- a/src/box/sql/whereexpr.c
> +++ b/src/box/sql/whereexpr.c
> @@ -168,7 +168,7 @@ exprCommute(Parse * pParse, Expr * pExpr)
> bool is_found;
> uint32_t id;
> sql_expr_coll(pParse, pExpr->pLeft, &is_found, &id);
> - if (is_found) {
> + if (id != 0) {
2. If you fix a comment about COLL_NONE constant return,
do not forget to pick this hunk up as well, please. Maybe
this patch contains more, I am not sure.
> /*
> * Neither X nor Y have COLLATE
> * operators, but X has a
====================================================================
commit b138616a1851dbe905a2e86b981f36793a7eb117
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date: Tue Nov 13 14:39:45 2018 +0300
Speculative review fixes
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index ffbe83aa5..2db8e1c7f 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -2167,16 +2167,16 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
* @retval Id of collation to be used during string comparison.
*/
static uint32_t
-multi_select_coll_seq(struct Parse *parser, struct Select *p, int n,
- bool *is_forced_coll)
+multi_select_coll_seq_r(struct Parse *parser, struct Select *p, int n,
+ bool *is_forced_coll)
{
bool is_prior_forced = false;
bool is_current_forced;
uint32_t prior_coll_id = 0;
uint32_t current_coll_id;
if (p->pPrior != NULL) {
- prior_coll_id = multi_select_coll_seq(parser, p->pPrior, n,
- &is_prior_forced);
+ prior_coll_id = multi_select_coll_seq_r(parser, p->pPrior, n,
+ &is_prior_forced);
}
/*
* Column number must be less than p->pEList->nExpr.
@@ -2198,6 +2198,13 @@ multi_select_coll_seq(struct Parse *parser, struct Select *p, int n,
return res_coll_id;
}
+static inline uint32_t
+multi_select_coll_seq(struct Parse *parser, struct Select *p, int n)
+{
+ bool unused;
+ return multi_select_coll_seq_r(parser, p, n, &unused);
+}
+
/**
* The select statement passed as the second parameter is a
* compound SELECT with an ORDER BY clause. This function
@@ -2237,8 +2244,7 @@ sql_multiselect_orderby_to_key_info(struct Parse *parse, struct Select *s,
sql_expr_coll(parse, term, &unused, &id);
} else {
id = multi_select_coll_seq(parse, s,
- item->u.x.iOrderByCol - 1,
- &unused);
+ item->u.x.iOrderByCol - 1);
if (id != COLL_NONE) {
const char *name = coll_by_id(id)->name;
order_by->a[i].pExpr =
@@ -2900,10 +2906,9 @@ multiSelect(Parse * pParse, /* Parsing context */
struct sql_key_info *key_info = sql_key_info_new(db, nCol);
if (key_info == NULL)
goto multi_select_end;
- bool unused;
for (int i = 0; i < nCol; i++) {
key_info->parts[i].coll_id =
- multi_select_coll_seq(pParse, p, i, &unused);
+ multi_select_coll_seq(pParse, p, i);
}
for (struct Select *pLoop = p; pLoop; pLoop = pLoop->pPrior) {
@@ -3328,12 +3333,10 @@ multiSelectOrderBy(Parse * pParse, /* Parsing context */
pParse->nMem += expr_count + 1;
sqlite3VdbeAddOp2(v, OP_Integer, 0, regPrev);
key_info_dup = sql_key_info_new(db, expr_count);
- bool unused;
if (key_info_dup != NULL) {
for (int i = 0; i < expr_count; i++) {
key_info_dup->parts[i].coll_id =
- multi_select_coll_seq(pParse, p, i,
- &unused);
+ multi_select_coll_seq(pParse, p, i);
}
}
}
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index cfe87c62a..470320e8d 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4330,7 +4330,8 @@ const char *sqlite3ErrStr(int);
*
* @param parse Parsing context.
* @param expr Expression to scan.
- * @param[out] is_found Flag set if explicit COLLATE clause is used.
+ * @param[out] is_explicit_coll Flag set if explicit COLLATE
+ * clause is used.
* @param[out] coll_id Collation identifier.
*
* @retval Pointer to collation.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH v2 2/3] Introduce "none" and "binary" collations
2018-11-13 12:02 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-11-13 22:37 ` n.pettik
2018-11-14 11:52 ` Vladislav Shpilevoy
0 siblings, 1 reply; 12+ messages in thread
From: n.pettik @ 2018-11-13 22:37 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
> On 13 Nov 2018, at 15:02, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> Hi! Thanks for the fixes!
>
> Please, remember to put branch/issue link
> into a covering letter next time.
I’m sorry, it was late at night...
> See 5 comments below, my fixes at the end
> of the email and on the branch.
>
>> diff --git a/src/box/alter.cc b/src/box/alter.cc
>> index 6d2c59bbc..2eb9f53e8 100644
>> --- a/src/box/alter.cc
>> +++ b/src/box/alter.cc
>> @@ -403,7 +403,7 @@ field_def_decode(struct field_def *field, const char **data,
>> "nullable action properties", fieldno +
>> TUPLE_INDEX_BASE));
>> }
>> - if (field->coll_id != COLL_NONE &&
>> + if (field->coll_id != 0 &&
>
> 1. Why not to leave enum COLL_NONE, but change its value to 0?
> I guess, it is a purpose of define/enum features - define a name
> for a constant to be able to change the value under the hood. It
> could have reduced the diff.
For me it looks more convenient. But I don’t mind applying your fixes.
But there is one more point to avoid using COLL_NONE.
See my comment at the end of review.
>> field->type != FIELD_TYPE_STRING &&
>> field->type != FIELD_TYPE_SCALAR &&
>> field->type != FIELD_TYPE_ANY) {
>> diff --git a/src/box/sql/callback.c b/src/box/sql/callback.c
>> index 3cf3a835d..352745e0e 100644
>> --- a/src/box/sql/callback.c
>> +++ b/src/box/sql/callback.c
>> @@ -42,13 +42,22 @@
>> struct coll *
>> sql_get_coll_seq(Parse *parser, const char *name, uint32_t *coll_id)
>> {
>> - if (name == NULL || strcasecmp(name, "binary") == 0) {
>> - *coll_id = COLL_NONE;
>> - return NULL;
>> + if (name == NULL) {
>> + *coll_id = 0;
>> + return coll_by_id(0)->coll;
>> }
>> - struct coll_id *p = coll_by_name(name, strlen(name));
>> + struct coll_id *p;
>> + /*
>> + * In SQL all identifiers should be uppercased, so
>> + * to avoid mess lets simple search binary (since it is
>> + * sort of "special" collation) ignoring case at all.
>> + */
>
> 2. I am not sure if it should be so special. I think, we should
> treat it just like any other collation. If tests have BINARY
> uppercased, then they should be fixed, I guess.
Thing is in SQLite binary can be used without quotes, like:
… COLLATE BiNaRy …
And since ids are uppercased, this thing comes as “BINARY".
If I forced only lower case “binary”, a lot (>80 separate sub-tests,
12 separate file tests) of tests would fail. Moreover, I see that similar
things take place at Lua-land (schema.cc):
local function update_index_parts(format, parts)
...
elseif k == 'collation' then
-- find ID by name
local coll = box.space._collation.index.name:get{v}
if not coll then
coll = box.space._collation.index.name:get{v:lower()}
end
If you still think that we should allow only “binary” format,
I will do it alongside with tests in a separate commit.
>> + if (strcasecmp(name, "binary") == 0)
>> + p = coll_by_name("binary", strlen("binary"));
>> + else
>> + p = coll_by_name(name, strlen(name));
>> if (p == NULL) {
>> - *coll_id = COLL_NONE;
>> + *coll_id = 0;
>> sqlite3ErrorMsg(parser, "no such collation sequence: %s",
>> name);
>> return NULL;
>> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
>> index 8c34cbb3d..580cf1e60 100644
>> --- a/src/box/sql/func.c
>> +++ b/src/box/sql/func.c
>> @@ -506,7 +506,7 @@ case_type##ICUFunc(sqlite3_context *context, int argc, sqlite3_value **argv) \
>> UErrorCode status = U_ZERO_ERROR; \
>> struct coll *coll = sqlite3GetFuncCollSeq(context); \
>> const char *locale = NULL; \
>> - if (coll != NULL) { \
>> + if (coll != NULL && coll->collator != NULL) { \
>
> 3. Why do you use coll->collator != 0 instead of coll->type == COLL_TYPE_ICU?
> I do not think it is safe since in future we can move collator into a union,
> which will not be NULL for a non-ICU collation.
Yep, you are absolutely right, I should use type of collation, instead of
checking existence of collator. Fixed:
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 580cf1e60..a2ac9ba2d 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -506,7 +506,7 @@ case_type##ICUFunc(sqlite3_context *context, int argc, sqlite3_value **argv) \
UErrorCode status = U_ZERO_ERROR; \
struct coll *coll = sqlite3GetFuncCollSeq(context); \
const char *locale = NULL; \
- if (coll != NULL && coll->collator != NULL) { \
+ if (coll != NULL && coll->type == COLL_TYPE_ICU) { \
locale = ucol_getLocaleByType(coll->collator, \
ULOC_VALID_LOCALE, &status); \
} \
>
>> locale = ucol_getLocaleByType(coll->collator, \
>> ULOC_VALID_LOCALE, &status); \
>> } > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
>> index 5bdf102e7..8e5aea51b 100644
>> --- a/src/box/tuple_format.c
>> +++ b/src/box/tuple_format.c
>> @@ -40,7 +40,7 @@ static uint32_t formats_size = 0, formats_capacity = 0;
>> static const struct tuple_field tuple_field_default = {
>> FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false,
>> - ON_CONFLICT_ACTION_NONE, NULL, COLL_NONE,
>> + ON_CONFLICT_ACTION_DEFAULT, NULL, 0,
>> };
>
> 4. Are you sure that changing on_conflict_action belongs
> to this patch?
No, it doesn’t. I guess it is an artefact of rebasing.
Glad that you caught it. Fixed.
diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index 8e5aea51b..badffc701 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -40,7 +40,7 @@ static uint32_t formats_size = 0, formats_capacity = 0;
static const struct tuple_field tuple_field_default = {
FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false,
- ON_CONFLICT_ACTION_DEFAULT, NULL, 0,
+ ON_CONFLICT_ACTION_NONE, NULL, 0,
>
> 5. How about tests like this one?
>
> box.schema.create_space('test', {format = {{1, 'unsigned'}, {2, 'string', collation = 'binary'}}})
>
> I mean specifying collations via Lua. And what happens if I use collation = 'none'
> in a space format or index parts? I think, it should throw an error, but it is up
> to Kostja or Kirill.
Hmm, personally I see no reason why an error should be raised.
If you specify “none” collation, then it will act as an absence of collation.
That is simple. Anyway, added tests on binary collation:
diff --git a/test/sql/collation.result b/test/sql/collation.result
index 3df4cfa70..b42d3e034 100644
--- a/test/sql/collation.result
+++ b/test/sql/collation.result
@@ -123,6 +123,49 @@ box.space.T:format()[3]['collation']
box.sql.execute("DROP TABLE t;")
---
...
+-- BINARY collation works in the same way as there is no collation
+-- at all.
+--
+t = box.schema.create_space('test', {format = {{'id', 'unsigned'}, {'a', 'string', collation = 'binary'}}})
+---
+...
+t:format()[2]['collation']
+---
+- 3
+...
+pk = t:create_index('primary', {parts = {1}})
+---
+...
+i = t:create_index('secondary', {parts = {2, 'str', collation='binary'}})
+---
+...
+t:insert({1, 'AsD'})
+---
+- [1, 'AsD']
+...
+t:insert({2, 'ASD'})
+---
+- [2, 'ASD']
+...
+t:insert({3, 'asd'})
+---
+- [3, 'asd']
+...
+i:select('asd')
+---
+- - [3, 'asd']
+...
+i:select('ASD')
+---
+- - [2, 'ASD']
+...
+i:select('AsD')
+---
+- - [1, 'AsD']
+...
+t:drop()
+---
+...
-- Collation with id == 0 is "none". It used to unify interaction
-- with collation interface. It also can't be dropped.
--
diff --git a/test/sql/collation.test.lua b/test/sql/collation.test.lua
index 61df33a95..05f666f5e 100644
--- a/test/sql/collation.test.lua
+++ b/test/sql/collation.test.lua
@@ -50,6 +50,21 @@ box.space.T:format()[2]['collation']
box.space.T:format()[3]['collation']
box.sql.execute("DROP TABLE t;")
+-- BINARY collation works in the same way as there is no collation
+-- at all.
+--
+t = box.schema.create_space('test', {format = {{'id', 'unsigned'}, {'a', 'string', collation = 'binary'}}})
+t:format()[2]['collation']
+pk = t:create_index('primary', {parts = {1}})
+i = t:create_index('secondary', {parts = {2, 'str', collation='binary'}})
+t:insert({1, 'AsD'})
+t:insert({2, 'ASD'})
+t:insert({3, 'asd'})
+i:select('asd')
+i:select('ASD')
+i:select('AsD')
+t:drop()
+
I would like to reveal my main concern about this patch:
pointer to “none” collation is not set in key_def/field_def, even if “none”
collation is forced. The way I wanted to implement this patch is like:
@@ -174,17 +174,15 @@ key_def_new(const struct key_part_def *parts, uint32_t part_count)
for (uint32_t i = 0; i < part_count; i++) {
const struct key_part_def *part = &parts[i];
struct coll *coll = NULL;
- if (part->type == FIELD_TYPE_SCALAR ||
- part->type == FIELD_TYPE_STRING) {
+ if (part->coll_id != COLL_NONE) {
struct coll_id *coll_id = coll_by_id(part->coll_id);
- if (part->coll_id != 0 && coll_id == NULL) {
+ if (coll_id == NULL) {
diag_set(ClientError, ER_WRONG_INDEX_OPTIONS,
i + 1, "collation was not found by ID");
key_def_delete(def);
return NULL;
}
- if (coll_id != NULL)
- coll = coll_id->coll;
+ coll = coll_id->coll;
}
key_def_set_part(def, i, part->fieldno, part->type,
part->nullable_action, coll, part->coll_id,
@@ -321,8 +319,7 @@ key_def_sizeof_parts(const struct key_part_def *parts, uint32_t part_count)
for (uint32_t i = 0; i < part_count; i++) {
const struct key_part_def *part = &parts[i];
int count = 2;
- if (part->type == FIELD_TYPE_STRING ||
- part->type == FIELD_TYPE_SCALAR)
+ if (part->coll_id != COLL_NONE)
count++;
if (part->is_nullable)
count++;
@@ -332,8 +329,7 @@ key_def_sizeof_parts(const struct key_part_def *parts, uint32_t part_count)
assert(part->type < field_type_MAX);
size += mp_sizeof_str(strlen(PART_OPT_TYPE));
size += mp_sizeof_str(strlen(field_type_strs[part->type]));
- if (part->type == FIELD_TYPE_STRING ||
- part->type == FIELD_TYPE_SCALAR) {
+ if (part->coll_id != COLL_NONE) {
size += mp_sizeof_str(strlen(PART_OPT_COLLATION));
size += mp_sizeof_uint(part->coll_id);
}
@@ -352,8 +348,7 @@ key_def_encode_parts(char *data, const struct key_part_def *parts,
for (uint32_t i = 0; i < part_count; i++) {
const struct key_part_def *part = &parts[i];
int count = 2;
- if (part->type == FIELD_TYPE_STRING ||
- part->type == FIELD_TYPE_SCALAR)
+ if (part->coll_id != COLL_NONE)
count++;
if (part->is_nullable)
count++;
@@ -366,8 +361,7 @@ key_def_encode_parts(char *data, const struct key_part_def *parts,
assert(part->type < field_type_MAX);
const char *type_str = field_type_strs[part->type];
data = mp_encode_str(data, type_str, strlen(type_str));
- if (part->type == FIELD_TYPE_STRING ||
- part->type == FIELD_TYPE_SCALAR) {
+ if (part->coll_id != COLL_NONE) {
data = mp_encode_str(data, PART_OPT_COLLATION,
strlen(PART_OPT_COLLATION));
data = mp_encode_uint(data, part->coll_id);
diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index efaa10da0..7cae436f1 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -299,15 +299,12 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i)
lua_pushboolean(L, key_part_is_nullable(part));
lua_setfield(L, -2, "is_nullable");
- if (part->type == FIELD_TYPE_STRING ||
- part->type == FIELD_TYPE_SCALAR) {
- if (! space_is_system(space)) {
- struct coll_id *coll_id =
- coll_by_id(part->coll_id);
- assert(coll_id != NULL);
- lua_pushstring(L, coll_id->name);
- lua_setfield(L, -2, "collation");
- }
+ if (part->coll_id != COLL_NONE) {
+ struct coll_id *coll_id =
+ coll_by_id(part->coll_id);
+ assert(coll_id != NULL);
+ lua_pushstring(L, coll_id->name);
+ lua_setfield(L, -2, "collation");
}
In other words, each field with type of STRING/SCALAR and mb ANY
would have real pointer to collation. However, in turn such change
would break too many tests, without significant refactoring of SQL
query planner (at least I failed to do it in several hours).
On the other hand, in this case we can remove COLL_NONE
and unify collation interface: each field or key part MUST feature
collation. And I mean not only valid ID (it is how it works now),
but also valid (i.e. != NULL) pointer to collation struct. So, it seems
that we would be able to remove any checks like:
if (coll != NULL)
*use collation*
else
*process without involving collations*
Would become:
If (type_is_string_like)
*always use collation*
To be honest I was sure that you would notice this pickle…
Or current approach is not dat bad?
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH v2 3/3] sql: change collation compatibility rules
2018-11-13 12:02 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-11-13 22:37 ` n.pettik
0 siblings, 0 replies; 12+ messages in thread
From: n.pettik @ 2018-11-13 22:37 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
>> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
>> index cea453f08..ab0376a1d 100644
>> --- a/src/box/sql/select.c
>> +++ b/src/box/sql/select.c
>> @@ -2150,47 +2151,51 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
>> }
>> #ifndef SQLITE_OMIT_COMPOUND_SELECT
>> -static struct coll *
>> -multi_select_coll_seq_r(Parse *parser, Select *p, int n, bool *is_found,
>> - uint32_t *coll_id)
>> +/**
>> + * This function determines resulting collation sequence for
>> + * @n-th column of the result set for the compound SELECT
>> + * statement. Since compound SELECT performs implicit comparisons
>> + * between values, all parts of compound queries must use
>> + * the same collation. Otherwise, an error is raised.
>> + *
>> + * @param parser Parse context.
>> + * @param p Select meta-information.
>> + * @param n Column number of the result set.
>> + * @param is_forced_coll Used if we fall into recursion.
>> + * For most-outer call it is unused. Used to indicate that
>> + * explicit COLLATE clause is used.
>> + * @retval Id of collation to be used during string comparison.
>> + */
>> +static uint32_t
>> +multi_select_coll_seq(struct Parse *parser, struct Select *p, int n,
>> + bool *is_forced_coll)
>
> 1. I renamed this function into multi_select_coll_seq_r and
> created a wrapper without is_forced_coll to remove some of
> 'bool unused' things. Apply or drop the fix, up to you.
Ok, I don’t mind your fixes. Applied.
>> diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
>> index 9fa6ce15d..e05967554 100644
>> --- a/src/box/sql/whereexpr.c
>> +++ b/src/box/sql/whereexpr.c
>> @@ -168,7 +168,7 @@ exprCommute(Parse * pParse, Expr * pExpr)
>> bool is_found;
>> uint32_t id;
>> sql_expr_coll(pParse, pExpr->pLeft, &is_found, &id);
>> - if (is_found) {
>> + if (id != 0) {
>
> 2. If you fix a comment about COLL_NONE constant return,
> do not forget to pick this hunk up as well, please. Maybe
> this patch contains more, I am not sure.
Ok, I’ve returned COLL_NONE usages:
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 09c248154..b67b22c23 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -360,11 +360,11 @@ collations_check_compatibility(uint32_t lhs_id, bool is_lhs_forced,
return 0;
}
if (lhs_id != rhs_id) {
- if (lhs_id == 0) {
+ if (lhs_id == COLL_NONE) {
*res_id = rhs_id;
return 0;
}
- if (rhs_id == 0) {
+ if (rhs_id == COLL_NONE) {
*res_id = lhs_id;
return 0;
}
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 2db8e1c7f..efdac9dba 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1963,7 +1963,8 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse, /* Parsing contexts */
uint32_t coll_id;
if (pTab->def->fields[i].coll_id == COLL_NONE &&
- sql_expr_coll(pParse, p, &is_found, &coll_id) && coll_id != 0)
+ sql_expr_coll(pParse, p, &is_found, &coll_id) &&
+ coll_id != COLL_NONE)
pTab->def->fields[i].coll_id = coll_id;
}
}
@@ -2172,7 +2173,7 @@ multi_select_coll_seq_r(struct Parse *parser, struct Select *p, int n,
{
bool is_prior_forced = false;
bool is_current_forced;
- uint32_t prior_coll_id = 0;
+ uint32_t prior_coll_id = COLL_NONE;
uint32_t current_coll_id;
if (p->pPrior != NULL) {
prior_coll_id = multi_select_coll_seq_r(parser, p->pPrior, n,
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index e05967554..971f428b7 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -168,7 +168,7 @@ exprCommute(Parse * pParse, Expr * pExpr)
bool is_found;
uint32_t id;
sql_expr_coll(pParse, pExpr->pLeft, &is_found, &id);
- if (id != 0) {
+ if (id != COLL_NONE) {
/*
* Neither X nor Y have COLLATE
* operators, but X has ag
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH v2 2/3] Introduce "none" and "binary" collations
2018-11-13 22:37 ` n.pettik
@ 2018-11-14 11:52 ` Vladislav Shpilevoy
2018-11-14 20:59 ` n.pettik
0 siblings, 1 reply; 12+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-14 11:52 UTC (permalink / raw)
To: n.pettik, tarantool-patches
>>> + /*
>>> + * In SQL all identifiers should be uppercased, so
>>> + * to avoid mess lets simple search binary (since it is
>>> + * sort of "special" collation) ignoring case at all.
>>> + */
>>
>> 2. I am not sure if it should be so special. I think, we should
>> treat it just like any other collation. If tests have BINARY
>> uppercased, then they should be fixed, I guess.
>
> Thing is in SQLite binary can be used without quotes, like:
>
> … COLLATE BiNaRy …
>
> And since ids are uppercased, this thing comes as “BINARY".
> If I forced only lower case “binary”, a lot (>80 separate sub-tests,
> 12 separate file tests) of tests would fail. Moreover, I see that similar
> things take place at Lua-land (schema.cc):
>
> local function update_index_parts(format, parts)
> ...
> elseif k == 'collation' then
> -- find ID by name
> local coll = box.space._collation.index.name:get{v}
> if not coll then
> coll = box.space._collation.index.name:get{v:lower()}
> end
>
> If you still think that we should allow only “binary” format,
> I will do it alongside with tests in a separate commit.
I still think that since 'binary' is even in _collation space
now, it should be just a regular collation. If you do not want
to change tests, then you can try to search any collation firstly
as is, and if not found, then lowerify and search again, regardless
of is it binary or not.
My point is that we should not treat binary differently from
others. Please, ask in the server team chat or consult Peter.
Especially, if you like the idea of lowering any collation name
if it is not found (just like we do in Lua as you decently
highlighted).
By the way, what if I write SQL like this? :
SELECT ... WHERE ... COLLATE NONE ...
Looks like this code:
> + if (strcasecmp(name, "binary") == 0)
> + p = coll_by_name("binary", strlen("binary"));
> + else
> + p = coll_by_name(name, strlen(name));
will find it ok, but maybe should not, is it?
> diff --git a/test/sql/collation.test.lua b/test/sql/collation.test.lua
> index 61df33a95..05f666f5e 100644
> --- a/test/sql/collation.test.lua
> +++ b/test/sql/collation.test.lua
> @@ -321,8 +319,7 @@ key_def_sizeof_parts(const struct key_part_def *parts, uint32_t part_count)
> for (uint32_t i = 0; i < part_count; i++) {
> const struct key_part_def *part = &parts[i];
> int count = 2;
> - if (part->type == FIELD_TYPE_STRING ||
> - part->type == FIELD_TYPE_SCALAR)
> + if (part->coll_id != COLL_NONE)
> count++;
> if (part->is_nullable)
> count++;
> @@ -332,8 +329,7 @@ key_def_sizeof_parts(const struct key_part_def *parts, uint32_t part_count)
> assert(part->type < field_type_MAX);
> size += mp_sizeof_str(strlen(PART_OPT_TYPE));
> size += mp_sizeof_str(strlen(field_type_strs[part->type]));
> - if (part->type == FIELD_TYPE_STRING ||
> - part->type == FIELD_TYPE_SCALAR) {
> + if (part->coll_id != COLL_NONE) {
> size += mp_sizeof_str(strlen(PART_OPT_COLLATION));
> size += mp_sizeof_uint(part->coll_id);
> }
> @@ -352,8 +348,7 @@ key_def_encode_parts(char *data, const struct key_part_def *parts,
> for (uint32_t i = 0; i < part_count; i++) {
> const struct key_part_def *part = &parts[i];
> int count = 2;
> - if (part->type == FIELD_TYPE_STRING ||
> - part->type == FIELD_TYPE_SCALAR)
> + if (part->coll_id != COLL_NONE)
> count++;
> if (part->is_nullable)
> count++;
> @@ -366,8 +361,7 @@ key_def_encode_parts(char *data, const struct key_part_def *parts,
> assert(part->type < field_type_MAX);
> const char *type_str = field_type_strs[part->type];
> data = mp_encode_str(data, type_str, strlen(type_str));
> - if (part->type == FIELD_TYPE_STRING ||
> - part->type == FIELD_TYPE_SCALAR) {
> + if (part->coll_id != COLL_NONE) {
> data = mp_encode_str(data, PART_OPT_COLLATION,
> strlen(PART_OPT_COLLATION));
> data = mp_encode_uint(data, part->coll_id);
>
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index efaa10da0..7cae436f1 100644
> --- a/src/box/lua/space.cc
> +++ b/src/box/lua/space.cc
> @@ -299,15 +299,12 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i)
> lua_pushboolean(L, key_part_is_nullable(part));
> lua_setfield(L, -2, "is_nullable");
>
> - if (part->type == FIELD_TYPE_STRING ||
> - part->type == FIELD_TYPE_SCALAR) {
> - if (! space_is_system(space)) {
> - struct coll_id *coll_id =
> - coll_by_id(part->coll_id);
> - assert(coll_id != NULL);
> - lua_pushstring(L, coll_id->name);
> - lua_setfield(L, -2, "collation");
> - }
> + if (part->coll_id != COLL_NONE) {
> + struct coll_id *coll_id =
> + coll_by_id(part->coll_id);
> + assert(coll_id != NULL);
> + lua_pushstring(L, coll_id->name);
> + lua_setfield(L, -2, "collation");
> }
>
> In other words, each field with type of STRING/SCALAR and mb ANY
> would have real pointer to collation. However, in turn such change
> would break too many tests, without significant refactoring of SQL
> query planner (at least I failed to do it in several hours).
> On the other hand, in this case we can remove COLL_NONE
> and unify collation interface: each field or key part MUST feature
> collation. And I mean not only valid ID (it is how it works now),
> but also valid (i.e. != NULL) pointer to collation struct. So, it seems
> that we would be able to remove any checks like:
>
> if (coll != NULL)
> *use collation*
> else
> *process without involving collations*
>
> Would become:
>
> If (type_is_string_like)
> *always use collation*
>
> To be honest I was sure that you would notice this pickle…
> Or current approach is not dat bad?
>
Yeah, I understand your aspiration, but I have some remarks
about the draft patch above and about the concept as it is:
* you said that you want to get rid of COLL_NONE, but in the draft
above you still use it. Even if you had removed COLL_NONE
usage from key_def builders, it would have stayed in alter.cc
as a check that you can not drop 'none' collation. I think that
COLL_NONE as a constant equal to 0 should not be removed from
source code until it is not used at all. But it is a minor
comment rather about coding style: I prefer code like
'id != UNDERSTANDABLE_NAME' to 'id != <strange_constant>'.
* the main flaw in storing a collation for each string is that it
will dramatically slow down comparators. The awful problem is in
virtual functions calling - it is much slower than regular
functions. Now we have two levels of avoiding coll->cmp() calling.
First in *_compare_create functions. If a key has no collations,
we can use highly specified template recursive comparators
(struct TupleCompare, struct TupleCompareWithKey) which call
memcmp. Second, we are trying to do not call coll->cmp() in other
comparators (see mp_compare_str).
Yes, we still can follow you, store a collation object in each
key_part, tuple_field, field_def, but in tuple_compare.cc we
still will need a way how to check that a collation comparator
can be inlined into memcmp. This way is check coll->id == COLL_NONE.
Now it is like
use_fast_cmp = coll == NULL
Will be like
use_fast_cmp = coll->id == COLL_NONE
As a summary: we can store a collation in each *_field/part object
to simplify SQL source code, but it does not affect necessity of
a special COLL_NONE constant.
Also, I see now a new minor problem:
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index 64f74f9d3..a9525058b 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -396,8 +396,10 @@ local function create_collation_space()
> box.space._index:insert{_collation.id, 1, 'name', 'tree', {unique = true}, {{1, 'string'}}}
>
> log.info("create predefined collations")
> + box.space._collation:replace{0, "none", ADMIN, "BINARY", "", setmap{}}
> box.space._collation:replace{1, "unicode", ADMIN, "ICU", "", setmap{}}
> box.space._collation:replace{2, "unicode_ci", ADMIN, "ICU", "", {strength='primary'}}
> + box.space._collation:replace{3, "binary", ADMIN, "BINARY", "", setmap{}}
>
> local _priv = box.space[box.schema.PRIV_ID]
> _priv:insert{ADMIN, PUBLIC, 'space', _collation.id, box.priv.W}
Unfortunately, we can not change old update() functions. So the only way to add
these new collations is to add a new code to upgrade_to_2_1_0() with these two
new lines.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH v2 2/3] Introduce "none" and "binary" collations
2018-11-14 11:52 ` Vladislav Shpilevoy
@ 2018-11-14 20:59 ` n.pettik
2018-11-15 11:04 ` Vladislav Shpilevoy
0 siblings, 1 reply; 12+ messages in thread
From: n.pettik @ 2018-11-14 20:59 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
> On 14 Nov 2018, at 14:52, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
>>>>
>>>> + /*
>>>> + * In SQL all identifiers should be uppercased, so
>>>> + * to avoid mess lets simple search binary (since it is
>>>> + * sort of "special" collation) ignoring case at all.
>>>> + */
>>>
>>> 2. I am not sure if it should be so special. I think, we should
>>> treat it just like any other collation. If tests have BINARY
>>> uppercased, then they should be fixed, I guess.
>> Thing is in SQLite binary can be used without quotes, like:
>> … COLLATE BiNaRy …
>> And since ids are uppercased, this thing comes as “BINARY".
>> If I forced only lower case “binary”, a lot (>80 separate sub-tests,
>> 12 separate file tests) of tests would fail. Moreover, I see that similar
>> things take place at Lua-land (schema.cc):
>> local function update_index_parts(format, parts)
>> ...
>> elseif k == 'collation' then
>> -- find ID by name
>> local coll = box.space._collation.index.name:get{v}
>> if not coll then
>> coll = box.space._collation.index.name:get{v:lower()}
>> end
>> If you still think that we should allow only “binary” format,
>> I will do it alongside with tests in a separate commit.
>
> I still think that since 'binary' is even in _collation space
> now, it should be just a regular collation. If you do not want
> to change tests, then you can try to search any collation firstly
> as is, and if not found, then lowerify and search again, regardless
> of is it binary or not.
We can’t lower each collation’s name - I’m sure that Peter will
be strictly against this change (to be honest I am too), because
it contradicts our rules concerning identifications’ names.
Moreover, what if user create two collations with names like
“custom_collation” and “CuStom_COLLATION” and execute query like
SELECT … COLLATE “custom_COLLATION"
Which collation will be used in this case? Without lowering no one
is found, but after lowering both would be available.
I would better fix tests to make them use “binary” instead of
BINARY. I guess this is absolutely right solution.
Commit at the end of letter.
> My point is that we should not treat binary differently from
> others.
Yep, now I fully agree with you. I am going to add one more
commit to patchset.
> By the way, what if I write SQL like this? :
>
> SELECT ... WHERE ... COLLATE NONE …
>
> Looks like this code:
>
>> + if (strcasecmp(name, "binary") == 0)
>> + p = coll_by_name("binary", strlen("binary"));
>> + else
>> + p = coll_by_name(name, strlen(name));
>
> will find it ok, but maybe should not, is it?
Why? It exists in _collation and uses memcmp, so what is wrong with it?
I guess we simply should document its behaviour.
Also, in your example parser would fail to find it. Correct usage is like:
> SELECT ... WHERE ... COLLATE “none” …
:)
On the contrary, I've added tests on this case:
diff --git a/test/sql-tap/identifier_case.test.lua b/test/sql-tap/identifier_case.test.lua
index 4878ced65..9e8107aef 100755
--- a/test/sql-tap/identifier_case.test.lua
+++ b/test/sql-tap/identifier_case.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(67)
+test:plan(71)
local test_prefix = "identifier_case-"
@@ -207,7 +207,9 @@ data = {
{ 4, [["bInaRy"]], {1, "Collation 'bInaRy' does not exist"}},
{ 5, [["unicode"]], {0}},
{ 6, [[ unicode ]], {1,"Collation 'UNICODE' does not exist"}},
- { 7, [["UNICODE"]], {1,"Collation 'UNICODE' does not exist"}}
+ { 7, [["UNICODE"]], {1,"Collation 'UNICODE' does not exist"}},
+ { 8, [[NONE]], {1,"Collation 'NONE' does not exist"}},
+ { 9, [["none"]], {0}},
}
test:do_catchsql_test(
@@ -241,6 +243,8 @@ data = {
{ 5, [[ 5 < 'b' collate "unicode" ]], {0, {1}}},
{ 6, [[ 5 < 'b' collate unicode ]], {1,"Collation 'UNICODE' does not exist"}},
{ 7, [[ 5 < 'b' collate "unicode_ci" ]], {0, {1}}},
+ { 8, [[ 5 < 'b' collate NONE ]], {1, "Collation 'NONE' does not exist"}},
+ { 9, [[ 5 < 'b' collate "none" ]], {0, {1}}},
>> In other words, each field with type of STRING/SCALAR and mb ANY
>> would have real pointer to collation. However, in turn such change
>> would break too many tests, without significant refactoring of SQL
>> query planner (at least I failed to do it in several hours).
>> On the other hand, in this case we can remove COLL_NONE
>> and unify collation interface: each field or key part MUST feature
>> collation. And I mean not only valid ID (it is how it works now),
>> but also valid (i.e. != NULL) pointer to collation struct. So, it seems
>> that we would be able to remove any checks like:
>> if (coll != NULL)
>> *use collation*
>> else
>> *process without involving collations*
>> Would become:
>> If (type_is_string_like)
>> *always use collation*
>> To be honest I was sure that you would notice this pickle…
>> Or current approach is not dat bad?
>
> Yeah, I understand your aspiration, but I have some remarks
> about the draft patch above and about the concept as it is:
>
> * you said that you want to get rid of COLL_NONE, but in the draft
> above you still use it. Even if you had removed COLL_NONE
> usage from key_def builders, it would have stayed in alter.cc
> as a check that you can not drop 'none' collation. I think that
> COLL_NONE as a constant equal to 0 should not be removed from
> source code until it is not used at all. But it is a minor
> comment rather about coding style: I prefer code like
> 'id != UNDERSTANDABLE_NAME' to 'id != <strange_constant>'.
And still we use iid != 0 to check whether index primary or not…
But I like your argument.
> * the main flaw in storing a collation for each string is that it
> will dramatically slow down comparators. The awful problem is in
> virtual functions calling - it is much slower than regular
> functions.
Yep, I understand this point and I told the same to Kostja,
but he insisted on this “none” collation. To be honest, I thought
that he wanted to implement second variant, where pointers
to “none” collation != NULL.
> Now we have two levels of avoiding coll->cmp() calling.
> First in *_compare_create functions. If a key has no collations,
> we can use highly specified template recursive comparators
> (struct TupleCompare, struct TupleCompareWithKey) which call
> memcmp. Second, we are trying to do not call coll->cmp() in other
> comparators (see mp_compare_str).
>
> Yes, we still can follow you, store a collation object in each
> key_part, tuple_field, field_def, but in tuple_compare.cc we
> still will need a way how to check that a collation comparator
> can be inlined into memcmp. This way is check coll->id == COLL_NONE.
> Now it is like
>
> use_fast_cmp = coll == NULL
>
> Will be like
>
> use_fast_cmp = coll->id == COLL_NONE
>
> As a summary: we can store a collation in each *_field/part object
> to simplify SQL source code, but it does not affect necessity of
> a special COLL_NONE constant.
Thanks for your comments.
> Also, I see now a new minor problem:
>
>> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
>> index 64f74f9d3..a9525058b 100644
>> --- a/src/box/lua/upgrade.lua
>> +++ b/src/box/lua/upgrade.lua
>> @@ -396,8 +396,10 @@ local function create_collation_space()
>> box.space._index:insert{_collation.id, 1, 'name', 'tree', {unique = true}, {{1, 'string'}}}
>> log.info("create predefined collations")
>> + box.space._collation:replace{0, "none", ADMIN, "BINARY", "", setmap{}}
>> box.space._collation:replace{1, "unicode", ADMIN, "ICU", "", setmap{}}
>> box.space._collation:replace{2, "unicode_ci", ADMIN, "ICU", "", {strength='primary'}}
>> + box.space._collation:replace{3, "binary", ADMIN, "BINARY", "", setmap{}}
>> local _priv = box.space[box.schema.PRIV_ID]
>> _priv:insert{ADMIN, PUBLIC, 'space', _collation.id, box.priv.W}
>
> Unfortunately, we can not change old update() functions. So the only way to add
> these new collations is to add a new code to upgrade_to_2_1_0() with these two
> new lines.
Ops, indeed. Moved new collations to upgrade function and
updated bootstrap file:
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index a9525058b..4b0fd0345 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -396,10 +396,8 @@ local function create_collation_space()
box.space._index:insert{_collation.id, 1, 'name', 'tree', {unique = true}, {{1, 'string'}}}
log.info("create predefined collations")
- box.space._collation:replace{0, "none", ADMIN, "BINARY", "", setmap{}}
box.space._collation:replace{1, "unicode", ADMIN, "ICU", "", setmap{}}
box.space._collation:replace{2, "unicode_ci", ADMIN, "ICU", "", {strength='primary'}}
- box.space._collation:replace{3, "binary", ADMIN, "BINARY", "", setmap{}}
local _priv = box.space[box.schema.PRIV_ID]
_priv:insert{ADMIN, PUBLIC, 'space', _collation.id, box.priv.W}
@@ -608,6 +606,9 @@ local function upgrade_to_2_1_0()
format[2] = {type='any', name='value', is_nullable=true}
box.space._schema:format(format)
+ box.space._collation:replace{0, "none", ADMIN, "BINARY", "", setmap{}}
+ box.space._collation:replace{3, "binary", ADMIN, "BINARY", "", setmap{}}
+
upgrade_priv_to_2_1_0()
end
New commit in patch-set:
commit 854792f46d4ec87c3c2218c7645fe398e007c888
Author: Nikita Pettik <korablev@tarantool.org>
Date: Wed Nov 14 21:07:58 2018 +0300
sql: don't uppercase name of binary collation
Since now we don't have real binary collation, it was allowed to use its
name in different cases. For instance: BiNaRY, "bInArY", "BINARY" etc.
All these names were valid and accounted for binary collation.
However, we are going to introduce real entry in _collation space to
represent binary collation. Thus, for now we allow using only standard
"binary" name.
Needed for #3185
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 17c20480e..191eb0308 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2400,11 +2400,8 @@ index_fill_def(struct Parse *parse, struct index *index,
if (expr->op == TK_COLLATE) {
sql_get_coll_seq(parse, expr->u.zToken, &coll_id);
if (coll_id == COLL_NONE &&
- strcmp(expr->u.zToken, "binary") != 0) {
- diag_set(ClientError, ER_NO_SUCH_COLLATION,
- expr->u.zToken);
+ strcmp(expr->u.zToken, "binary") != 0)
goto tnt_error;
- }
diff --git a/src/box/sql/callback.c b/src/box/sql/callback.c
index 3cf3a835d..62904fd20 100644
--- a/src/box/sql/callback.c
+++ b/src/box/sql/callback.c
@@ -42,15 +42,16 @@
struct coll *
sql_get_coll_seq(Parse *parser, const char *name, uint32_t *coll_id)
{
- if (name == NULL || strcasecmp(name, "binary") == 0) {
+ if (name == NULL || strcmp(name, "binary") == 0) {
*coll_id = COLL_NONE;
return NULL;
}
struct coll_id *p = coll_by_name(name, strlen(name));
if (p == NULL) {
*coll_id = COLL_NONE;
- sqlite3ErrorMsg(parser, "no such collation sequence: %s",
- name);
+ diag_set(ClientError, ER_NO_SUCH_COLLATION, name);
+ parser->rc = SQL_TARANTOOL_ERROR;
+ parser->nErr++;
return NULL;
} else {
*coll_id = p->id;
diff --git a/test/sql-tap/collation.test.lua b/test/sql-tap/collation.test.lua
index eb4f43a90..9483632b2 100755
--- a/test/sql-tap/collation.test.lua
+++ b/test/sql-tap/collation.test.lua
@@ -155,7 +155,7 @@ local data_test_unicode_ci = {
local data_collations = {
-- default collation = binary
{"/*COLLATE DEFAULT*/", data_test_binary_1},
- {"COLLATE BINARY", data_test_binary_1},
+ {"COLLATE \"binary\"", data_test_binary_1},
{"COLLATE \"unicode\"", data_test_unicode},
{"COLLATE \"unicode_ci\"", data_test_unicode_ci},
}
diff --git a/test/sql-tap/distinct.test.lua b/test/sql-tap/distinct.test.lua
index 26a4aace2..8f6ca8ed9 100755
--- a/test/sql-tap/distinct.test.lua
+++ b/test/sql-tap/distinct.test.lua
@@ -124,9 +124,9 @@ local data = {
{"13.2", 0, "SELECT DISTINCT a, b, c COLLATE \"unicode_ci\" FROM t4"},
{"14.1", 0, "SELECT DISTINCT a, d COLLATE \"unicode_ci\" FROM t1"},
{"14.2", 1, "SELECT DISTINCT a, d COLLATE \"unicode_ci\" FROM t4"},
- {"15 ", 0, "SELECT DISTINCT a, d COLLATE binary FROM t1"},
- {"16.1", 0, "SELECT DISTINCT a, b, c COLLATE binary FROM t1"},
- {"16.2", 1, "SELECT DISTINCT a, b, c COLLATE binary FROM t4"},
+ {"15 ", 0, "SELECT DISTINCT a, d COLLATE \"binary\" FROM t1"},
+ {"16.1", 0, "SELECT DISTINCT a, b, c COLLATE \"binary\" FROM t1"},
+ {"16.2", 1, "SELECT DISTINCT a, b, c COLLATE \"binary\" FROM t4"},
{"17", 0, --{ \/* Technically, it would be possible to detect that DISTINCT\n ** is a no-op in cases like the following. But SQLite does not\n ** do so. *\/\n
"SELECT DISTINCT t1.id FROM t1, t2 WHERE t1.id=t2.x" },
{"18 ", 1, "SELECT DISTINCT c1, c2 FROM t3"},
@@ -173,7 +173,7 @@ data = {
{"a, b, c FROM t1", {"btree"}, {"A", "B", "C", "a", "b", "c"}},
{"a, b, c FROM t1 ORDER BY a, b, c", {"btree"}, {"A", "B", "C", "a", "b", "c"}},
{"b FROM t1 WHERE a = 'a'", {}, {"b"}},
- {"b FROM t1 ORDER BY +b COLLATE binary", {"btree", "btree"}, {"B", "b"}},
+ {"b FROM t1 ORDER BY +b COLLATE \"binary\"", {"btree", "btree"}, {"B", "b"}},
{"a FROM t1", {}, {"A", "a"}},
{"b COLLATE \"unicode_ci\" FROM t1", {}, {"b"}},
{"b COLLATE \"unicode_ci\" FROM t1 ORDER BY b COLLATE \"unicode_ci\"", {}, {"b"}},
diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
index 47fb7a809..1e8f096e1 100755
--- a/test/sql-tap/e_select1.test.lua
+++ b/test/sql-tap/e_select1.test.lua
@@ -508,11 +508,11 @@ if (0 > 0)
test:do_execsql_test(
"e_select-1.6.0",
[[
- CREATE TABLE t5(a TEXT COLLATE "unicode_ci", b TEXT COLLATE binary);
+ CREATE TABLE t5(a TEXT COLLATE "unicode_ci", b TEXT COLLATE "binary");
INSERT INTO t5 VALUES('AA', 'cc');
INSERT INTO t5 VALUES('BB', 'dd');
INSERT INTO t5 VALUES(NULL, NULL);
- CREATE TABLE t6(a TEXT COLLATE binary, b TEXT COLLATE "unicode_ci");
+ CREATE TABLE t6(a TEXT COLLATE "c", b TEXT COLLATE "unicode_ci");
INSERT INTO t6 VALUES('aa', 'cc');
INSERT INTO t6 VALUES('bb', 'DD');
INSERT INTO t6 VALUES(NULL, NULL);
@@ -997,7 +997,7 @@ test:do_execsql_test(
INSERT INTO b2 VALUES('abc', 3);
INSERT INTO b2 VALUES('xyz', 4);
- CREATE TABLE b3(id INT PRIMARY KEY, a TEXT COLLATE "unicode_ci", b TEXT COLLATE binary);
+ CREATE TABLE b3(id INT PRIMARY KEY, a TEXT COLLATE "unicode_ci", b TEXT COLLATE "binary");
INSERT INTO b3 VALUES(1, 'abc', 'abc');
INSERT INTO b3 VALUES(2, 'aBC', 'aBC');
INSERT INTO b3 VALUES(3, 'Def', 'Def');
@@ -1289,7 +1289,7 @@ test:do_select_tests(
{"1", "SELECT DISTINCT b FROM h1", {"one", "I", "i", "four", "IV", "iv"}},
{"2", "SELECT DISTINCT b COLLATE \"unicode_ci\" FROM h1", {"one", "I", "four", "IV"}},
{"3", "SELECT DISTINCT x FROM h2", {"One", "Two", "Three", "Four"}},
- {"4", "SELECT DISTINCT x COLLATE binary FROM h2", {
+ {"4", "SELECT DISTINCT x COLLATE \"binary\" FROM h2", {
"One", "Two", "Three", "Four", "one", "two", "three", "four"
}},
})
@@ -1574,7 +1574,7 @@ test:drop_all_tables()
test:do_execsql_test(
"e_select-7.10.0",
[[
- CREATE TABLE y1(a TEXT COLLATE "unicode_ci" PRIMARY KEY, b TEXT COLLATE binary, c TEXT );
+ CREATE TABLE y1(a TEXT COLLATE "unicode_ci" PRIMARY KEY, b TEXT COLLATE "binary", c TEXT );
INSERT INTO y1 VALUES('Abc', 'abc', 'aBC');
]], {
-- <e_select-7.10.0>
@@ -1588,12 +1588,12 @@ test:do_select_tests(
{"1", "SELECT 'abc' UNION SELECT 'ABC'", {"ABC", "abc"}},
{"2", "SELECT 'abc' COLLATE \"unicode_ci\" UNION SELECT 'ABC'", {"ABC" }},
{"3", "SELECT 'abc' UNION SELECT 'ABC' COLLATE \"unicode_ci\"", {"ABC" }},
- {"4", "SELECT 'abc' COLLATE binary UNION SELECT 'ABC' COLLATE \"unicode_ci\"", {"ABC", "abc"}},
- {"5", "SELECT 'abc' COLLATE \"unicode_ci\" UNION SELECT 'ABC' COLLATE binary", {"ABC" }},
+ {"4", "SELECT 'abc' COLLATE \"binary\" UNION SELECT 'ABC' COLLATE \"unicode_ci\"", {"ABC", "abc"}},
+ {"5", "SELECT 'abc' COLLATE \"unicode_ci\" UNION SELECT 'ABC' COLLATE \"binary\"", {"ABC" }},
{"6", "SELECT a FROM y1 UNION SELECT b FROM y1", {"abc" }},
{"7", "SELECT b FROM y1 UNION SELECT a FROM y1", {"Abc", "abc"}},
{"8", "SELECT a FROM y1 UNION SELECT c FROM y1", {"aBC" }},
- {"9", "SELECT a FROM y1 UNION SELECT c COLLATE binary FROM y1", {"aBC" }},
+ {"9", "SELECT a FROM y1 UNION SELECT c COLLATE \"binary\" FROM y1", {"aBC" }},
})
-- EVIDENCE-OF: R-32706-07403 No affinity transformations are applied to
@@ -1932,7 +1932,7 @@ test:do_execsql_test(
test:do_execsql_test(
"e_select-8.9.1",
[[
- SELECT x FROM d4 ORDER BY 1 COLLATE binary
+ SELECT x FROM d4 ORDER BY 1 COLLATE "binary"
]], {
-- <e_select-8.9.1>
"DEF", "JKL", "abc", "ghi"
@@ -1942,7 +1942,7 @@ test:do_execsql_test(
test:do_execsql_test(
"e_select-8.9.2",
[[
- SELECT x COLLATE binary FROM d4 ORDER BY 1 COLLATE "unicode_ci"
+ SELECT x COLLATE "binary" FROM d4 ORDER BY 1 COLLATE "unicode_ci"
]], {
-- <e_select-8.9.2>
"abc", "DEF", "ghi", "JKL"
@@ -1963,7 +1963,7 @@ test:do_execsql_test(
test:do_execsql_test(
"e_select-8.10.1",
[[
- SELECT x COLLATE binary FROM d4 ORDER BY 1
+ SELECT x COLLATE "binary" FROM d4 ORDER BY 1
]], {
-- <e_select-8.10.1>
"DEF", "JKL", "abc", "ghi"
@@ -1973,7 +1973,7 @@ test:do_execsql_test(
test:do_execsql_test(
"e_select-8.10.2",
[[
- SELECT x COLLATE binary FROM d4 ORDER BY x
+ SELECT x COLLATE "binary" FROM d4 ORDER BY x
]], {
-- <e_select-8.10.2>
"abc", "DEF", "ghi", "JKL"
@@ -1983,7 +1983,7 @@ test:do_execsql_test(
test:do_execsql_test(
"e_select-8.10.3",
[[
- SELECT x COLLATE binary AS x FROM d4 ORDER BY x
+ SELECT x COLLATE "binary" AS x FROM d4 ORDER BY x
]], {
-- <e_select-8.10.3>
"DEF", "JKL", "abc", "ghi"
diff --git a/test/sql-tap/identifier_case.test.lua b/test/sql-tap/identifier_case.test.lua
index 096130a52..4878ced65 100755
--- a/test/sql-tap/identifier_case.test.lua
+++ b/test/sql-tap/identifier_case.test.lua
@@ -201,10 +201,10 @@ end
-- Check that collaiton names work as identifiers
data = {
- { 1, [[ binary ]], {0}},
- { 2, [[ BINARY ]], {0}},
+ { 1, [[ binary ]], {1, "Collation 'BINARY' does not exist"}},
+ { 2, [[ BINARY ]], {1, "Collation 'BINARY' does not exist"}},
{ 3, [["binary"]], {0}},
- { 4, [["bInaRy"]], {0}},
+ { 4, [["bInaRy"]], {1, "Collation 'bInaRy' does not exist"}},
{ 5, [["unicode"]], {0}},
{ 6, [[ unicode ]], {1,"Collation 'UNICODE' does not exist"}},
{ 7, [["UNICODE"]], {1,"Collation 'UNICODE' does not exist"}}
@@ -234,12 +234,12 @@ test:do_catchsql_test(
{0})
data = {
- { 1, [[ 'a' < 'b' collate binary ]], {0, {1}}},
+ { 1, [[ 'a' < 'b' collate binary ]], {1, "Collation 'BINARY' does not exist"}},
{ 2, [[ 'a' < 'b' collate "binary" ]], {0, {1}}},
{ 3, [[ 'a' < 'b' collate 'binary' ]], {1, [[near "'binary'": syntax error]]}},
{ 4, [[ 'a' < 'b' collate "unicode" ]], {0, {1}}},
{ 5, [[ 5 < 'b' collate "unicode" ]], {0, {1}}},
- { 6, [[ 5 < 'b' collate unicode ]], {1,"no such collation sequence: UNICODE"}},
+ { 6, [[ 5 < 'b' collate unicode ]], {1,"Collation 'UNICODE' does not exist"}},
{ 7, [[ 5 < 'b' collate "unicode_ci" ]], {0, {1}}},
}
diff --git a/test/sql-tap/in3.test.lua b/test/sql-tap/in3.test.lua
index 83139a3e4..e6f0d415d 100755
--- a/test/sql-tap/in3.test.lua
+++ b/test/sql-tap/in3.test.lua
@@ -206,7 +206,7 @@ test:do_test(
test:do_test(
"in3-1.15",
function()
- return exec_neph(" SELECT a FROM t1 WHERE a COLLATE binary IN (SELECT a FROM t1) ")
+ return exec_neph(" SELECT a FROM t1 WHERE a COLLATE \"binary\" IN (SELECT a FROM t1) ")
end, {
-- <in3-1.15>
0, 1, 3, 5
diff --git a/test/sql-tap/in5.test.lua b/test/sql-tap/in5.test.lua
index 4e2cdcd24..ba21170ee 100755
--- a/test/sql-tap/in5.test.lua
+++ b/test/sql-tap/in5.test.lua
@@ -277,7 +277,7 @@ test:do_execsql_test(
test:do_execsql_test(
"6.1.2",
[[
- SELECT count(*) FROM t1 WHERE a COLLATE BINARY IN (SELECT DISTINCT a FROM t1)
+ SELECT count(*) FROM t1 WHERE a COLLATE "binary" IN (SELECT DISTINCT a FROM t1)
]], {
-- <6.1.2>
1
diff --git a/test/sql-tap/index3.test.lua b/test/sql-tap/index3.test.lua
index 4f950be09..222b895aa 100755
--- a/test/sql-tap/index3.test.lua
+++ b/test/sql-tap/index3.test.lua
@@ -59,7 +59,7 @@ test:do_execsql_test(
CREATE TABLE t1(a INT , b TEXT , c INT , d INT , e INT ,
PRIMARY KEY(a), UNIQUE(b COLLATE "unicode_ci" DESC));
CREATE INDEX t1c ON t1(c);
- CREATE INDEX t1d ON t1(d COLLATE binary ASC);
+ CREATE INDEX t1d ON t1(d COLLATE "binary" ASC);
WITH RECURSIVE c(x) AS (VALUES(1) UNION SELECT x+1 FROM c WHERE x<30)
INSERT INTO t1(a,b,c,d,e)
SELECT x, printf('ab%03xxy',x), x, x, x FROM c;
diff --git a/test/sql-tap/minmax3.test.lua b/test/sql-tap/minmax3.test.lua
index 1ddf39ff5..348275ffa 100755
--- a/test/sql-tap/minmax3.test.lua
+++ b/test/sql-tap/minmax3.test.lua
@@ -502,7 +502,7 @@ test:do_execsql_test(
test:do_execsql_test(
"minmax3-4.4",
[[
- SELECT max(x COLLATE binary), max(x COLLATE "unicode_ci") FROM t4;
+ SELECT max(x COLLATE "binary"), max(x COLLATE "unicode_ci") FROM t4;
]], {
-- <minmax3-4.4>
"abc", "BCD"
@@ -564,7 +564,7 @@ test:do_execsql_test(
test:do_execsql_test(
"minmax3-4.13",
[[
- SELECT min(x COLLATE binary), min(x COLLATE "unicode_ci") FROM t4;
+ SELECT min(x COLLATE "binary"), min(x COLLATE "unicode_ci") FROM t4;
]], {
-- <minmax3-4.13>
"BCD", "abc"
diff --git a/test/sql-tap/resolver01.test.lua b/test/sql-tap/resolver01.test.lua
index d08f95bf6..7be2184e5 100755
--- a/test/sql-tap/resolver01.test.lua
+++ b/test/sql-tap/resolver01.test.lua
@@ -236,7 +236,7 @@ test:do_test(
]])
local r = {}
table.insert(r, test:execsql("SELECT '1', substr(m,2) AS m FROM t4 ORDER BY m;"))
- table.insert(r, test:execsql("SELECT '2', substr(m,2) AS m FROM t4 ORDER BY m COLLATE binary;"))
+ table.insert(r, test:execsql("SELECT '2', substr(m,2) AS m FROM t4 ORDER BY m COLLATE \"binary\";"))
table.insert(r, test:execsql("SELECT '3', substr(m,2) AS m FROM t4 ORDER BY lower(m);"))
return r
end, {
diff --git a/test/sql-tap/selectE.test.lua b/test/sql-tap/selectE.test.lua
index 11b84711e..b379f5aa8 100755
--- a/test/sql-tap/selectE.test.lua
+++ b/test/sql-tap/selectE.test.lua
@@ -84,7 +84,7 @@ test:do_test(
function()
return test:execsql [[
SELECT a FROM t2 EXCEPT SELECT a FROM t3
- ORDER BY a COLLATE binary;
+ ORDER BY a COLLATE "binary";
]]
end, {
-- <selectE-1.2>
@@ -127,7 +127,7 @@ test:do_test(
function()
return test:execsql [[
SELECT a COLLATE "unicode_ci" FROM t2 EXCEPT SELECT a FROM t3
- ORDER BY 1 COLLATE binary
+ ORDER BY 1 COLLATE "binary"
]]
end, {
-- <selectE-2.2>
diff --git a/test/sql-tap/tkt2391.test.lua b/test/sql-tap/tkt2391.test.lua
index 7fa5e1634..b48998c53 100755
--- a/test/sql-tap/tkt2391.test.lua
+++ b/test/sql-tap/tkt2391.test.lua
@@ -20,7 +20,7 @@ test:plan(4)
test:do_execsql_test(
"tkt2391.1",
[[
- CREATE TABLE folders(folderid INT , parentid INT , foldername TEXT COLLATE binary primary key);
+ CREATE TABLE folders(folderid INT , parentid INT , foldername TEXT COLLATE "binary" primary key);
INSERT INTO folders VALUES(1, 3, 'FolderA');
INSERT INTO folders VALUES(1, 3, 'folderB');
INSERT INTO folders VALUES(4, 0, 'FolderC');
diff --git a/test/sql-tap/tkt3493.test.lua b/test/sql-tap/tkt3493.test.lua
index 26ca2271b..7e093b217 100755
--- a/test/sql-tap/tkt3493.test.lua
+++ b/test/sql-tap/tkt3493.test.lua
@@ -246,7 +246,7 @@ test:do_execsql_test(
test:do_execsql_test(
"tkt3493-3.1",
[[
- CREATE TABLE t2(a TEXT COLLATE "unicode_ci" PRIMARY KEY, b TEXT COLLATE BINARY);
+ CREATE TABLE t2(a TEXT COLLATE "unicode_ci" PRIMARY KEY, b TEXT COLLATE "binary");
INSERT INTO t2 VALUES('aBc', 'DeF');
]], {
-- <tkt3493-3.1>
@@ -291,7 +291,7 @@ test:do_execsql_test(
test:do_execsql_test(
"tkt3493-3.3.2",
[[
- SELECT a>b COLLATE BINARY FROM t2 GROUP BY a, b
+ SELECT a>b COLLATE "binary" FROM t2 GROUP BY a, b
]], {
-- <tkt3493-3.3.2>
1
diff --git a/test/sql-tap/with1.test.lua b/test/sql-tap/with1.test.lua
index faa99811c..8200d25f8 100755
--- a/test/sql-tap/with1.test.lua
+++ b/test/sql-tap/with1.test.lua
@@ -1056,7 +1056,7 @@ test:do_catchsql_test(13.3, [[
-- 2015-04-12
--
test:do_execsql_test(14.1, [[
- WITH x AS (SELECT * FROM t) SELECT 0 EXCEPT SELECT 0 ORDER BY 1 COLLATE binary;
+ WITH x AS (SELECT * FROM t) SELECT 0 EXCEPT SELECT 0 ORDER BY 1 COLLATE "binary";
]], {
-- <14.1>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH v2 2/3] Introduce "none" and "binary" collations
2018-11-14 20:59 ` n.pettik
@ 2018-11-15 11:04 ` Vladislav Shpilevoy
0 siblings, 0 replies; 12+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-15 11:04 UTC (permalink / raw)
To: n.pettik, tarantool-patches
Hi! Thanks for the fixes!
>>> In other words, each field with type of STRING/SCALAR and mb ANY
>>> would have real pointer to collation. However, in turn such change
>>> would break too many tests, without significant refactoring of SQL
>>> query planner (at least I failed to do it in several hours).
>>> On the other hand, in this case we can remove COLL_NONE
>>> and unify collation interface: each field or key part MUST feature
>>> collation. And I mean not only valid ID (it is how it works now),
>>> but also valid (i.e. != NULL) pointer to collation struct. So, it seems
>>> that we would be able to remove any checks like:
>>> if (coll != NULL)
>>> *use collation*
>>> else
>>> *process without involving collations*
>>> Would become:
>>> If (type_is_string_like)
>>> *always use collation*
>>> To be honest I was sure that you would notice this pickle…
>>> Or current approach is not dat bad?
>>
>> Yeah, I understand your aspiration, but I have some remarks
>> about the draft patch above and about the concept as it is:
>>
>> * you said that you want to get rid of COLL_NONE, but in the draft
>> above you still use it. Even if you had removed COLL_NONE
>> usage from key_def builders, it would have stayed in alter.cc
>> as a check that you can not drop 'none' collation. I think that
>> COLL_NONE as a constant equal to 0 should not be removed from
>> source code until it is not used at all. But it is a minor
>> comment rather about coding style: I prefer code like
>> 'id != UNDERSTANDABLE_NAME' to 'id != <strange_constant>'.
>
> And still we use iid != 0 to check whether index primary or not…
Decent, but I do not like it too. I tried to convince Kostja that it
is a bad pattern, but as usually he did not listen.
> diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
> index 47fb7a809..1e8f096e1 100755
> --- a/test/sql-tap/e_select1.test.lua
> +++ b/test/sql-tap/e_select1.test.lua
> @@ -508,11 +508,11 @@ if (0 > 0)
> test:do_execsql_test(
> "e_select-1.6.0",
> [[
> - CREATE TABLE t5(a TEXT COLLATE "unicode_ci", b TEXT COLLATE binary);
> + CREATE TABLE t5(a TEXT COLLATE "unicode_ci", b TEXT COLLATE "binary");
> INSERT INTO t5 VALUES('AA', 'cc');
> INSERT INTO t5 VALUES('BB', 'dd');
> INSERT INTO t5 VALUES(NULL, NULL);
> - CREATE TABLE t6(a TEXT COLLATE binary, b TEXT COLLATE "unicode_ci");
> + CREATE TABLE t6(a TEXT COLLATE "c", b TEXT COLLATE "unicode_ci");
Typo - binary turned into "c". I've force pushed a fix so as to allow
Kirill Y. merge the branch as soon as possible. Nevertheless, I've put
the fix at the end of the email, according to SOP.
> INSERT INTO t6 VALUES('aa', 'cc');
> INSERT INTO t6 VALUES('bb', 'DD');
> INSERT INTO t6 VALUES(NULL, NULL);
Also in the patch 'Introduce "none" and "binary" collations' I see one
of COLL_NONE substituted back to 0. I have force fixed it too, and put
at the end of the email.
==================================================================
Fix for "sql: don't uppercase name of binary collation".
diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
index 1e8f096e1..e1d814f47 100755
--- a/test/sql-tap/e_select1.test.lua
+++ b/test/sql-tap/e_select1.test.lua
@@ -512,7 +512,7 @@ if (0 > 0)
INSERT INTO t5 VALUES('AA', 'cc');
INSERT INTO t5 VALUES('BB', 'dd');
INSERT INTO t5 VALUES(NULL, NULL);
- CREATE TABLE t6(a TEXT COLLATE "c", b TEXT COLLATE "unicode_ci");
+ CREATE TABLE t6(a TEXT COLLATE "binary", b TEXT COLLATE "unicode_ci");
INSERT INTO t6 VALUES('aa', 'cc');
INSERT INTO t6 VALUES('bb', 'DD');
INSERT INTO t6 VALUES(NULL, NULL);
===================================================================
Fix for "Introduce "none" and "binary" collations".
diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index 8fc217f68..5bdf102e7 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -40,7 +40,7 @@ static uint32_t formats_size = 0, formats_capacity = 0;
static const struct tuple_field tuple_field_default = {
FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false,
- ON_CONFLICT_ACTION_NONE, NULL, 0,
+ ON_CONFLICT_ACTION_NONE, NULL, COLL_NONE,
};
/**
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH v2 0/3] Change collation compatibility rules according to ANSI SQL
2018-11-13 0:07 [tarantool-patches] [PATCH v2 0/3] Change collation compatibility rules according to ANSI SQL Nikita Pettik
` (2 preceding siblings ...)
2018-11-13 0:07 ` [tarantool-patches] [PATCH v2 3/3] sql: change collation compatibility rules Nikita Pettik
@ 2018-11-15 11:24 ` Kirill Yukhin
3 siblings, 0 replies; 12+ messages in thread
From: Kirill Yukhin @ 2018-11-15 11:24 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
Hello,
On 13 Nov 03:07, Nikita Pettik wrote:
> This patch-set introduces rules for collation compatibility in order to
> restrict usage of different collations within one comparison.
> Originally, SQLite uses doubtful approach: if collations of LHS and RHS
> can not be used together, then collation of LHS is chosen. Such
> behaviour quite misleading and results in the fact that swap of LHS and
> RHS values may change result of operation. We've decided to add
> restrictions on collations of LHS and RHS to disallow messing different
> collations as it is stated in ANSI SQL.
I've rebased and checked your patch set into 2.1 branch.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-11-15 11:24 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-13 0:07 [tarantool-patches] [PATCH v2 0/3] Change collation compatibility rules according to ANSI SQL Nikita Pettik
2018-11-13 0:07 ` [tarantool-patches] [PATCH v2 1/3] sql: do not add explicit <COLLATE "BINARY"> clause Nikita Pettik
2018-11-13 0:07 ` [tarantool-patches] [PATCH v2 2/3] Introduce "none" and "binary" collations Nikita Pettik
2018-11-13 12:02 ` [tarantool-patches] " Vladislav Shpilevoy
2018-11-13 22:37 ` n.pettik
2018-11-14 11:52 ` Vladislav Shpilevoy
2018-11-14 20:59 ` n.pettik
2018-11-15 11:04 ` Vladislav Shpilevoy
2018-11-13 0:07 ` [tarantool-patches] [PATCH v2 3/3] sql: change collation compatibility rules Nikita Pettik
2018-11-13 12:02 ` [tarantool-patches] " Vladislav Shpilevoy
2018-11-13 22:37 ` n.pettik
2018-11-15 11:24 ` [tarantool-patches] Re: [PATCH v2 0/3] Change collation compatibility rules according to ANSI SQL Kirill Yukhin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox