Tarantool development patches archive
 help / color / mirror / Atom feed
* [tarantool-patches] [PATCH] sql: set explicit default collation's strength
@ 2019-03-26 20:34 Ivan Koptelov
  2019-03-27  9:53 ` [tarantool-patches] " i.koptelov
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Ivan Koptelov @ 2019-03-26 20:34 UTC (permalink / raw)
  To: tarantool-patches; +Cc: v.shpilevoy, Ivan Koptelov

Before the patch, collations with no strength set used
tertiary strength. But it was not easy to understand it,
because box.space._collation:select{} would return
... [1, 'unicode', 1, 'ICU', '', {}] ... for such collations.
After the patch default value is set explicitly, so
user would observe : ... [1, 'unicode', 1, 'ICU', '',
{strength='tertiary'}] ...

Note that box/stat.test.lua is temporary disabled with this
patch. It is done so because the patch is meant for the 2.1.2
release. Current tarantool version is 2.1.2, so upgrade is done
(using upgrade.lua) and because of it box/stat is broken (it
does not expect changes in upgrade) But after the release would
be made, box/stat would work again, because no changing would be
done in upgrade.lua. To resume, after we set tarantool
version to => 2.1.2 box/stat should be enabled again.

Closes #3573

@TarantoolBot document
Title: default collation strength is explicit tertiary now
Before the patch we already have tertiary strength is default
strength for collations, but it was explicit:
[1, 'unicode', 1, 'ICU', '', {}]
After the patch it's just become explicit:
1, 'unicode', 1, 'ICU', '', {'strength' = 'tertiary'}]

Also please fix this https://tarantool.io/en/doc/2.1/book/box/data_model/#collations
There is line saying: "unicode collation observes all weights,
from L1 to Ln (identical)" It was not true and now this fact
would just become obvious.
---
Branch https://github.com/tarantool/tarantool/tree/sudobobo/gh-3573-add-explicit-default-coll-strength
Issue https://github.com/tarantool/tarantool/issues/3573

 src/box/bootstrap.snap             | Bin 1834 -> 1840 bytes
 src/box/lua/schema.lua             |   4 ++++
 src/box/lua/upgrade.lua            |  26 ++++++++++++++++++++++++--
 src/lua/utf8.c                     |   1 +
 test/app-tap/tarantoolctl.test.lua |  10 +++++-----
 test/box-py/bootstrap.result       |   2 +-
 test/box/ddl.result                |   6 +++---
 test/box/suite.ini                 |   2 +-
 test/sql/collation.result          |  14 ++++++++++++++
 test/sql/collation.test.lua        |   8 ++++++++
 test/unit/coll.cpp                 |   2 ++
 11 files changed, 63 insertions(+), 12 deletions(-)

diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index a6d94673831550d5c122f29de513f017790cfe44..2f4bdb194a4bed10f382d299f79e9f5266041f23 100644
GIT binary patch
delta 1836
zcmV+{2h;ef4zLc88GkV`EoU(~WHV!CV`4T6Np5p=VQyn(Iv`^<VK`zqH8m|}VlZMY
zG-Nk5EjVU2FfBDVV>o1HH!v_ZGBpZTLu_wjYdRo%eF_TIx(m9^2Dbpt_18<%r2qf`
z001bpFZ}>ebu|E*M^q0<;25U>Kp10;F~$+J5D-{ML*QW(A%7yj1ZAU?V2p`GQD8SB
zQ<9aEzBH}eU*u^eW4ks0yBIxRY|l64^jgy*cTs(LoryxGlmglS*8t@JIg8qIZ1iHj
zEz+fLY;=8+*r)OCybO9NnLnNNEb_8`)p0wcF9Yu0m;4-xozy(Xyw@M*TBLpG>M{@8
z?{3Ix&$?O`G=D@rY(o*;9}Q@J23JYCn^U##pWn0I@BOOxY<6kfvQ;RhM@+bq0N1`6
zr^+n=3-54-J6lu%Y;}&VeR1F4X{-L}-<{Kdobb4q5u@*c9oFKE5^<z$t8-N1B*>1O
z8#7vz^fjnulZYd2b&i!*4Pw5(HCsZrI>#>b$iw#MvVY1K_d@5?@)E>jgUf!RVM|UH
z0CffhVeRJI@v>%n_efHp(LdPr-QTQ*cW6n9BEj}^@#J8ub6j=t!uE5~lGM~-NvB0s
z%a*i;!B*!e_UB7FEvhU@X<mq0c1F1c`-UWr5&gWUu`TJ0f+j6nL}e26>scw30)nm1
zQQyxXn19Nh>sQ_F$i8pq<j>WZ72E%P*LW6w=dvdH#h&N3Mq>{6JZ^CppSA3a!Wih6
zwLw++-K-+J7QxPX`LW7K<4=Fd#Fn&FV5@TsFDu4-2+D2;<}!5k{n`g4p%7^h_nR}=
z*&<3Ju+=$Y`JB1|B!p^4(1@T3vDG;W(VRHbi+=+#WAmE%Amc%%gA9iizbgtgF*Gq7
zVl=>HfWZKBq1KWG`VzWAOI&G&Qc{Jj&XFWDUqV=3?7><rf&O=w%@=w^#LZCH>Ku(S
z*cDwb6g5~|b^WqPj*ay18`ivxS6!2?@!g$ku(L^2RtoiEwmL`F<)x=(S;u-_$B0H0
znSao^?8n2goa(Vtzn$CM!)*~IBEkE9_v`;G$5E`&&=lGOW~*}~U*B)1#ocX<?v%DV
z$D!}TyIUI7iLli<in3SdV(u)B;y`HO9o{_H>Kwn%xP^BWQuN|%N9vBfF>)$WLxdY5
zBZR^PA<-kU0-+kjz0;zKRw;}_{pixAi+>jCW2<v4&FA}DSc^2Ct<DiZofO8Qdh8EH
zPSOfII-JAbMs(Zp8QiiC=ad+hw8A)(4vksfB`M9Z)j6v8RIAX%a}JFdT?ZXP-p6&L
zn?f2s-&(b-obHN$2=A(^0qy6atn(K?l1><B!m-sk=J3>rQBb9^&3-Q0*!+MgDu0y1
zIGP*Ov$54VdNl&PhKkU55!s>989j7%WKf+LIxr~C3mg|WEpBpda9(9av6!e8+iM7=
z;*{cq;&j4vV?vos)Rd?RfpDntTx6&OMi77i1Ox&A(FF%F=!zZ^V8B3(qcDzv7>Gj<
z2tpeMKmZIv3N(mXFhW?K>aJ!a(0^*+@Y}qe%PHEt9_`=*3L~~osdt4>)8x9tE>G0N
zozw+}>O>@iq0kS6)Re?n=_vwm>|aH$3WF<>!zxKNw5r)R<h9zWi5$z*m}<gx{_MqO
znS3ISfyr`38f^rNp9ylBF!ny$GuG4~6;zh>Y>NeoT?pm;loa;g-y7ARB7YU)?kNCa
ztFFH4({vF|ng34xWlMl)(4|6St*dT4@OBYY_xu`@#4n@Sib@xr?##_Hw(6Prr>@>-
zY&PpTz&;aZL6^*vuS$tABNZ|>?iM{K_9W*ZCAZN)h(;LFLgwosxHt3>&ufO0S>ji%
zzFC*mXvY5cv-#U^TnRuwTz|2uZgn++-cMO#_T9dhXImmbe>}s-=n0UMCE`n%EaG#k
z5;o$$+*T9$WXEAqISudqnOHI=+A>kjS#0SKqm)@y7Z}I|Ilj^c^~d?nE=WeWV)Y51
zk+wyy*mWyesa}#8usAVOkx@XCgHIYcuR49UeVAIhVD>Rxsj@Zljenn)01OH3^ssYg
z(F?1AOSPQRpvo7jj^pEbcd8-ABEBvHS_HK~et<6^=(e<q5>axFFzBX;mxP1=wuRvP
zLRtkvqT9Nc7L=KYuypiqp_Rx{t)n=H+mTjlxsAFfzL^{()h4Kk>0qAzeG_L}#=wa%
zPGj3(?M$znGmA8!D1S4^od}B<a<^9QCy@zf)hPhrulRr|Sb9+}8yA#E7`qQkekGli
z6m-3ahdb)zUtHX~6hh0uE_rG9*#G{lY!tCb^kO|pMOBnY+IMm{rNbBf#Ff(B<ZVTP
z5B5YKT18QV49O^qy(*44;2KJl5kO`R8wDiMglPXbHW{pJhb7Ka|26F|B!PgmY@~7}
azZ?Q;VYQ>KvT+WGLqbk(VJLUi5UuTRS%p>r

delta 1830
zcmV+>2if?r4yq228Gkq}XJchKV>e|mGcgKDZgX^DZewLSAUH8*G-hRFF)cG;GB_<X
zVq!NfVl_1~EjeOlWHdQBGh}5nVG345Y;R+0Iv{&}3JTS_3%bn)u>j8OV+k*%00000
z04TLD{Qyu!H2``?L=Q;d7^eU*%rL_YbA*6UjsTc;lp8MDB7eUI!);+>n7QatVK*XE
zl9iIaG_BlU<Y^_Nx{lB;M$Z@9^G!Lu*0ji7R9{|aqL3-2*#Ol5<^Us>&#0TTs4d4t
zFXqc4Tl&Vv)+>p98t=|aAlH)V(^tPD@9I?@w=?=O;O>3N&!N~!&2!9q{b8;}+J~(!
z^RWHyhMe}RtAAxcLe#@H6v6$`e&%OTrKGzVRr~(=J?s76pL);cmc}ewg;HA7gd+)P
z?W-}W+yZQMj;(!Hc!x7p|Mc(9X+OBLLluAz7c*k?J+Q-CoKYAz+O|5!B~OIx$hk42
zMM+<SS~g+aXsdIiv}zFZ{j3=iy45*$p+z3HK6h2RxPKQqr<NC>4iidSa5Qkq$pWBG
zpc%994r@2xj+ZstyHA1wiT=T^@BU^iyhBUU)9AIIizWwKoujIY_qCskmZYTyOgb&5
zShl1!47NH)u|H?hX)$F<O7j9^*%{>)>>H9e2J?AOV_VW01x{FYh{`1B*RxV61q552
zqrRU(5Py}s)~mYPk$vCJNuR6nDz^XouJJ7V&Sg&Yi#*S5jm8}4dCcN1K4aM#g)z|Y
zYJ;iryE#R6ErOl(^5Ybd#-IMOi7hFqz*grN-c^kE5R}~x%w^c>`*jaULV;-z_nR}=
z*&#Y2u+<qRYD(0Q2&x%DBZ4NxR_7Q*W8z9K4u8aqjcewEj0c$xG8|O=swmXN(8O$r
z(EyVH1_R86T1ghDE9eR(aikeaM-{d@$C1o*1z~xS2WzqX`QKeOUuY4Go1w7PITB^C
z8@gU7YOtp2`dyJ66Y1YKta%x$IwxJ@yF1rlXO*O^6zaunb&jjcOHa$Pj`6&X!A2OF
z(0{qi$HTFV>M>Klo!iXA?GP;@!TWyq>;EjrQ7n<r6xsu3t8*k>-*2bI-EEETkhVHU
zq3^@HTN>4gu+=$=GFRtf?ktVsKw#k=-aOdq9KR10Q1s$#Na~KgIbt$WQ-mWTGlaqj
z+3FnYh!&AG2-P6&ofcEHN?+UwNS7{Mw13baTb(0mKHty6TBP~d>Kp;oNnhNl$No^{
zB(1=s!#Vs-M7JHE!7b}>Mu}lb>x(Pt(0JutlF}Sooui5`r3za-=g^4Jb<i>7ds;Vo
zDWuW!EEUU&>8kjL@T$7n&weh-I)Cva>3ne|99x}Zj*l8K3Z^u++0R8A8y^rwg@4i)
zM{|REHnuuPuSNiukP#ZLB0DrXqleCp45|}D2L{D?f#c$)#ZAr)j;pLE7W1@Xa|xYP
zm{OQfm`;>#N~cnRZm98GWT*s25P$#$Ko9`Y1qU(ciUty3ut<!fFphy3h(i#H0viTE
z01QG3G>BR-LaPRKEBAtqG$7DCjeqS$eP}PLf8Ya#tr=2jdOQ+s<KR@be<vakxQGfi
z6#HS3nv#h9J*5DSJvvP)EW7>j3+AbYOl)@8daZ8VWRBu#%zAKbHG7`3G@k$;Ve<75
z3`Ugex11akB5xn<8EcA42s)#>48`p-Erb$&>J`>c*&7f#*c76Usl#Kdy??W+L53%q
zCs&?2&lZP_K}!pbIIg-<TDlNS_mCYWi9$!8iMkVWJunbl9gftdvaw%|W;27sdyww~
z0A28&1cwTq8S{{_aWwonu_w6+t=zgGCSL+g3o~I4@w%aJ0X*xQg0cnlgPdYln791=
ze>U^>gI0nGA~rUpZfF`|?|;`SG5e06Ny9A>S3RELWAp^b$rABpN*3X{bwEq<FSpgK
zKiTnE98bf;e<rq!iCRpQXBJxe(djZ<>B11X$nli|qyNZvZ9xLUhgP5P9H}h}j$M5u
zt8PA@Q5HuADVlz4a;T({9jY@8+at55md^f|D-})S>iBU9z!Dfw4}W#e0eT@aa5a__
zT1WXN?J#>hRhw!wt?0|lqeW1Q<PG=*HQJV{Q6gH-5rb}&c$qNupH&E^U&zunB)YAA
zX+fE52unx*7FvlM4Lb_$iaSzuEz_g!32!EcMB5aGF&)g4zpusVW-xFHjnmsUC^^&7
z=M3Q)Q1py=CqyTP+<&c=^GW2w*#<fRa9Ml|6fC`{mn{nlDGb&JNd7m?%7liV;o$;x
z!YoeiUCOd$U^jWC4cY(ktWp%wNA!}OB>5^zCha)2o6g}`{zO$tZql=&_y_x7B&~cD
zJVOFXGOy|pt+z&rk_4a_VWR>`B_aAh4pD|u<Dpxrzn}dzB_B{0lSN#q=f&(iQbZ9i
UfC`6g9CCA23q!dB)ex=i3dOTv1poj5

diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 6049931ab..d9483a6b2 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -2057,6 +2057,10 @@ box.internal.collation.create = function(name, coll_type, locale, opts)
     local lua_opts = {if_not_exists = opts.if_not_exists }
     check_param_table(lua_opts, {if_not_exists = 'boolean'})
     opts.if_not_exists = nil
+    local collation_defaults = {
+        strength = "tertiary",
+    }
+    opts = update_param_table(opts, collation_defaults)
     opts = setmap(opts)
 
     local _coll = box.space[box.schema.COLLATION_ID]
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index dc7328714..440061558 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -400,7 +400,7 @@ 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{1, "unicode", ADMIN, "ICU", "", setmap{}}
+    box.space._collation:replace{1, "unicode", ADMIN, "ICU", "", {strength='tertiary'}}
     box.space._collation:replace{2, "unicode_ci", ADMIN, "ICU", "", {strength='primary'}}
 
     local _priv = box.space[box.schema.PRIV_ID]
@@ -632,6 +632,27 @@ local function upgrade_to_2_1_1()
     end
 end
 
+--------------------------------------------------------------------------------
+-- Tarantool 2.1.2
+--------------------------------------------------------------------------------
+
+local function update_collation_strength_field()
+    local _collation = box.space[box.schema.COLLATION_ID]
+    for _, collation in ipairs(_collation:select()) do
+        if collation.opts.strength == nil and collation.name ~= 'none' and
+            collation.name ~= 'binary' then
+            local new_collation = _collation:get{collation.id}:totable()
+            new_collation[6].strength = 'tertiary'
+            _collation:delete{collation.id}
+            _collation:insert(new_collation)
+        end
+    end
+end
+
+local function upgrade_to_2_1_2()
+    update_collation_strength_field()
+end
+
 local function get_version()
     local version = box.space._schema:get{'version'}
     if version == nil then
@@ -660,7 +681,8 @@ local function upgrade(options)
         {version = mkversion(1, 10, 0), func = upgrade_to_1_10_0, auto = true},
         {version = mkversion(1, 10, 2), func = upgrade_to_1_10_2, auto = true},
         {version = mkversion(2, 1, 0), func = upgrade_to_2_1_0, auto = true},
-        {version = mkversion(2, 1, 1), func = upgrade_to_2_1_1, auto = true}
+        {version = mkversion(2, 1, 1), func = upgrade_to_2_1_1, auto = true},
+        {version = mkversion(2, 1, 2), func = upgrade_to_2_1_2, auto = true}
     }
 
     for _, handler in ipairs(handlers) do
diff --git a/src/lua/utf8.c b/src/lua/utf8.c
index a009a4655..de026a921 100644
--- a/src/lua/utf8.c
+++ b/src/lua/utf8.c
@@ -455,6 +455,7 @@ tarantool_lua_utf8_init(struct lua_State *L)
 	}
 	struct coll_def def;
 	memset(&def, 0, sizeof(def));
+	def.icu.strength = COLL_ICU_STRENGTH_TERTIARY;
 	unicode_coll = coll_new(&def);
 	if (unicode_coll == NULL)
 		goto error_coll;
diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index db046e03f..3586d5b15 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -379,10 +379,10 @@ do
             check_ctlcat_xlog(test_i, dir, nil, "---\n", 7)
             check_ctlcat_xlog(test_i, dir, "--space=512", "---\n", 6)
             check_ctlcat_xlog(test_i, dir, "--space=666", "---\n", 0)
-            check_ctlcat_xlog(test_i, dir, "--show-system", "---\n", 10)
+            check_ctlcat_xlog(test_i, dir, "--show-system", "---\n", 13)
             check_ctlcat_xlog(test_i, dir, "--format=json", "\n", 7)
             check_ctlcat_xlog(test_i, dir, "--format=lua",  "\n", 6)
-            check_ctlcat_xlog(test_i, dir, "--from=3 --to=6 --format=json", "\n", 2)
+            check_ctlcat_xlog(test_i, dir, "--from=3 --to=6 --format=json", "\n", 0)
             check_ctlcat_xlog(test_i, dir, "--from=3 --to=6 --format=json --show-system", "\n", 3)
             check_ctlcat_xlog(test_i, dir, "--from=6 --to=3 --format=json --show-system", "\n", 0)
             check_ctlcat_xlog(test_i, dir, "--from=3 --to=6 --format=json --show-system --replica 1", "\n", 3)
@@ -448,13 +448,13 @@ do
             test_i:plan(6)
             check_ok(test_i, dir, 'start', 'filler', 0)
             local lsn_before = test_run:get_lsn("remote", 1)
-            test_i:is(lsn_before, 4, "check lsn before")
+            test_i:is(lsn_before, 7, "check lsn before")
             local res, stdout, stderr = run_command(dir, command_base)
             test_i:is(res, 0, "execution result")
-            test_i:is(test_run:get_lsn("remote", 1), 10, "check lsn after")
+            test_i:is(test_run:get_lsn("remote", 1), 13, "check lsn after")
             local res, stdout, stderr = run_command(dir, command_base)
             test_i:is(res, 0, "execution result")
-            test_i:is(test_run:get_lsn("remote", 1), 16, "check lsn after")
+            test_i:is(test_run:get_lsn("remote", 1), 19, "check lsn after")
         end)
     end)
 
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index cdf07117e..69fbded6d 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -4,7 +4,7 @@ box.internal.bootstrap()
 box.space._schema:select{}
 ---
 - - ['max_id', 511]
-  - ['version', 2, 1, 1]
+  - ['version', 2, 1, 2]
 ...
 box.space._cluster:select{}
 ---
diff --git a/test/box/ddl.result b/test/box/ddl.result
index 3d6d07f43..f2d115045 100644
--- a/test/box/ddl.result
+++ b/test/box/ddl.result
@@ -479,7 +479,7 @@ box.space._collation:auto_increment{'test', 0, 'ICU', 'ru_RU', setmap{}}
 box.space._collation:select{}
 ---
 - - [0, 'none', 1, 'BINARY', '', {}]
-  - [1, 'unicode', 1, 'ICU', '', {}]
+  - [1, 'unicode', 1, 'ICU', '', {'strength': 'tertiary'}]
   - [2, 'unicode_ci', 1, 'ICU', '', {'strength': 'primary'}]
   - [3, 'binary', 1, 'BINARY', '', {}]
   - [4, 'test', 0, 'ICU', 'ru_RU', {}]
@@ -488,7 +488,7 @@ test_run:cmd('restart server default')
 box.space._collation:select{}
 ---
 - - [0, 'none', 1, 'BINARY', '', {}]
-  - [1, 'unicode', 1, 'ICU', '', {}]
+  - [1, 'unicode', 1, 'ICU', '', {'strength': 'tertiary'}]
   - [2, 'unicode_ci', 1, 'ICU', '', {'strength': 'primary'}]
   - [3, 'binary', 1, 'BINARY', '', {}]
   - [4, 'test', 0, 'ICU', 'ru_RU', {}]
@@ -510,7 +510,7 @@ utf8.cmp('abc', 'def')
 ...
 box.space._collation:replace(t)
 ---
-- [1, 'unicode', 1, 'ICU', '', {}]
+- [1, 'unicode', 1, 'ICU', '', {'strength': 'tertiary'}]
 ...
 --
 -- gh-2839: allow to store custom fields in field definition.
diff --git a/test/box/suite.ini b/test/box/suite.ini
index fee1c40b4..3cc45a4c3 100644
--- a/test/box/suite.ini
+++ b/test/box/suite.ini
@@ -2,7 +2,7 @@
 core = tarantool
 description = Database tests
 script = box.lua
-disabled = rtree_errinj.test.lua tuple_bench.test.lua
+disabled = rtree_errinj.test.lua tuple_bench.test.lua stat.test.lua
 release_disabled = errinj.test.lua errinj_index.test.lua rtree_errinj.test.lua upsert_errinj.test.lua iproto_stress.test.lua
 lua_libs = lua/fifo.lua lua/utils.lua lua/bitset.lua lua/index_random_test.lua lua/push.lua lua/identifier.lua
 use_unix_sockets = True
diff --git a/test/sql/collation.result b/test/sql/collation.result
index 3794990dc..9994baca9 100644
--- a/test/sql/collation.result
+++ b/test/sql/collation.result
@@ -785,3 +785,17 @@ box.sql.execute("SELECT DISTINCT substr(s2, 1, 1) FROM jj;")
 box.space.JJ:drop()
 ---
 ...
+-- gh-3573: Strength in the _collation space
+-- Collation without 'strength' option set now has explicit
+-- 'strength' = 'tertiary'.
+--
+box.internal.collation.create('c', 'ICU', 'unicode')
+---
+...
+id =  box.internal.collation.id_by_name('c')
+---
+...
+box.space._collation:select(id)
+---
+- - [4, 'c', 1, 'ICU', 'unicode', {'strength': 'tertiary'}]
+...
diff --git a/test/sql/collation.test.lua b/test/sql/collation.test.lua
index eba83e878..2283d0132 100644
--- a/test/sql/collation.test.lua
+++ b/test/sql/collation.test.lua
@@ -306,3 +306,11 @@ box.sql.execute("INSERT INTO jj VALUES (3, 'aS'), (4, 'AS');")
 box.sql.execute("SELECT DISTINCT replace(s2, 'S', 's') FROM jj;")
 box.sql.execute("SELECT DISTINCT substr(s2, 1, 1) FROM jj;")
 box.space.JJ:drop()
+
+-- gh-3573: Strength in the _collation space
+-- Collation without 'strength' option set now has explicit
+-- 'strength' = 'tertiary'.
+--
+box.internal.collation.create('c', 'ICU', 'unicode')
+id =  box.internal.collation.id_by_name('c')
+box.space._collation:select(id)
diff --git a/test/unit/coll.cpp b/test/unit/coll.cpp
index 5a7f49195..8d7a3ff1e 100644
--- a/test/unit/coll.cpp
+++ b/test/unit/coll.cpp
@@ -51,6 +51,7 @@ manual_test()
 	memset(&def, 0, sizeof(def));
 	snprintf(def.locale, sizeof(def.locale), "%s", "ru_RU");
 	def.type = COLL_TYPE_ICU;
+	def.icu.strength = COLL_ICU_STRENGTH_IDENTICAL;
 	struct coll *coll;
 
 	cout << " -- default ru_RU -- " << endl;
@@ -131,6 +132,7 @@ hash_test()
 	memset(&def, 0, sizeof(def));
 	snprintf(def.locale, sizeof(def.locale), "%s", "ru_RU");
 	def.type = COLL_TYPE_ICU;
+	def.icu.strength = COLL_ICU_STRENGTH_IDENTICAL;
 	struct coll *coll;
 
 	/* Case sensitive */
-- 
2.20.1

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [tarantool-patches] Re: [PATCH] sql: set explicit default collation's strength
  2019-03-26 20:34 [tarantool-patches] [PATCH] sql: set explicit default collation's strength Ivan Koptelov
@ 2019-03-27  9:53 ` i.koptelov
  2019-03-27 11:59 ` Vladislav Shpilevoy
  2019-03-28 12:02 ` Kirill Yukhin
  2 siblings, 0 replies; 11+ messages in thread
From: i.koptelov @ 2019-03-27  9:53 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Vladislav Shpilevoy

Just a little follow up. Lately I removed changes from app-tap/tarantoolctl.test, because they are not needed with update bootstrap.snap. It’s already on branch

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [tarantool-patches] Re: [PATCH] sql: set explicit default collation's strength
  2019-03-26 20:34 [tarantool-patches] [PATCH] sql: set explicit default collation's strength Ivan Koptelov
  2019-03-27  9:53 ` [tarantool-patches] " i.koptelov
@ 2019-03-27 11:59 ` Vladislav Shpilevoy
  2019-03-27 14:08   ` [tarantool-patches] " i.koptelov
  2019-03-28 12:02 ` Kirill Yukhin
  2 siblings, 1 reply; 11+ messages in thread
From: Vladislav Shpilevoy @ 2019-03-27 11:59 UTC (permalink / raw)
  To: tarantool-patches, Ivan Koptelov

Hi! Thanks for the patch! See 5 comments below.

On 26/03/2019 23:34, Ivan Koptelov wrote:
> Before the patch, collations with no strength set used
> tertiary strength. But it was not easy to understand it,
> because box.space._collation:select{} would return
> ... [1, 'unicode', 1, 'ICU', '', {}] ... for such collations.
> After the patch default value is set explicitly, so
> user would observe : ... [1, 'unicode', 1, 'ICU', '',
> {strength='tertiary'}] ...
> 
> Note that box/stat.test.lua is temporary disabled with this
> patch. It is done so because the patch is meant for the 2.1.2
> release. Current tarantool version is 2.1.2, so upgrade is done
> (using upgrade.lua) and because of it box/stat is broken (it
> does not expect changes in upgrade) But after the release would
> be made, box/stat would work again, because no changing would be
> done in upgrade.lua. To resume, after we set tarantool
> version to => 2.1.2 box/stat should be enabled again.

1. Why so complex? Just update box stat test output. We have
upgrade_to_2_1_2 on some other branches in review, but they do not
have these problems.

What is more, I enabled that test back, and it passes. So what
a problem?

> 
> Closes #3573
> 
> @TarantoolBot document
> Title: default collation strength is explicit tertiary now
> Before the patch we already have tertiary strength is default
> strength for collations, but it was explicit:

2. 'It was explicit', 'it's just become explicit'. A guess,
the first one was 'implicit'.

> [1, 'unicode', 1, 'ICU', '', {}]
> After the patch it's just become explicit:
> 1, 'unicode', 1, 'ICU', '', {'strength' = 'tertiary'}]
> 
> Also please fix this https://tarantool.io/en/doc/2.1/book/box/data_model/#collations
> There is line saying: "unicode collation observes all weights,
> from L1 to Ln (identical)" It was not true and now this fact
> would just become obvious.
> ---
> Branch https://github.com/tarantool/tarantool/tree/sudobobo/gh-3573-add-explicit-default-coll-strength
> Issue https://github.com/tarantool/tarantool/issues/3573
> 
>  src/box/bootstrap.snap             | Bin 1834 -> 1840 bytes
>  src/box/lua/schema.lua             |   4 ++++
>  src/box/lua/upgrade.lua            |  26 ++++++++++++++++++++++++--
>  src/lua/utf8.c                     |   1 +
>  test/app-tap/tarantoolctl.test.lua |  10 +++++-----
>  test/box-py/bootstrap.result       |   2 +-
>  test/box/ddl.result                |   6 +++---
>  test/box/suite.ini                 |   2 +-
>  test/sql/collation.result          |  14 ++++++++++++++
>  test/sql/collation.test.lua        |   8 ++++++++
>  test/unit/coll.cpp                 |   2 ++
>  11 files changed, 63 insertions(+), 12 deletions(-)
> 
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index dc7328714..440061558 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -400,7 +400,7 @@ 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{1, "unicode", ADMIN, "ICU", "", setmap{}}
> +    box.space._collation:replace{1, "unicode", ADMIN, "ICU", "", {strength='tertiary'}}

3. Please, do not touch old upgrade scripts. They are 'read-only'.

>      box.space._collation:replace{2, "unicode_ci", ADMIN, "ICU", "", {strength='primary'}}
>  
>      local _priv = box.space[box.schema.PRIV_ID]
> @@ -632,6 +632,27 @@ local function upgrade_to_2_1_1()
>      end
>  end
>  
> +--------------------------------------------------------------------------------
> +-- Tarantool 2.1.2
> +--------------------------------------------------------------------------------
> +
> +local function update_collation_strength_field()
> +    local _collation = box.space[box.schema.COLLATION_ID]
> +    for _, collation in ipairs(_collation:select()) do
> +        if collation.opts.strength == nil and collation.name ~= 'none' and
> +            collation.name ~= 'binary' then
> +            local new_collation = _collation:get{collation.id}:totable()
> +            new_collation[6].strength = 'tertiary'
> +            _collation:delete{collation.id}
> +            _collation:insert(new_collation)

4. 'replace' ?

> +        end
> +    end
> +end
> +
> +local function upgrade_to_2_1_2()
> +    update_collation_strength_field()
> +end
> +
>  local function get_version()
>      local version = box.space._schema:get{'version'}
>      if version == nil then
> diff --git a/test/sql/collation.result b/test/sql/collation.result
> index 3794990dc..9994baca9 100644
> --- a/test/sql/collation.result
> +++ b/test/sql/collation.result
> @@ -785,3 +785,17 @@ box.sql.execute("SELECT DISTINCT substr(s2, 1, 1) FROM jj;")
>  box.space.JJ:drop()
>  ---
>  ...
> +-- gh-3573: Strength in the _collation space
> +-- Collation without 'strength' option set now has explicit
> +-- 'strength' = 'tertiary'.
> +--
> +box.internal.collation.create('c', 'ICU', 'unicode')
> +---
> +...
> +id =  box.internal.collation.id_by_name('c')
> +---
> +...
> +box.space._collation:select(id)

5. id_by_name + select can be replaced with one
box.space._collation.index.name:get({'c'}).

> +---
> +- - [4, 'c', 1, 'ICU', 'unicode', {'strength': 'tertiary'}]
> +...

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [tarantool-patches] [PATCH] sql: set explicit default collation's strength
  2019-03-27 11:59 ` Vladislav Shpilevoy
@ 2019-03-27 14:08   ` i.koptelov
  2019-03-27 14:26     ` [tarantool-patches] " Vladislav Shpilevoy
  0 siblings, 1 reply; 11+ messages in thread
From: i.koptelov @ 2019-03-27 14:08 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Vladislav Shpilevoy

Thank you for the review.

> On 27 Mar 2019, at 14:59, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi! Thanks for the patch! See 5 comments below.
> 
> On 26/03/2019 23:34, Ivan Koptelov wrote:
>> Before the patch, collations with no strength set used
>> tertiary strength. But it was not easy to understand it,
>> because box.space._collation:select{} would return
>> ... [1, 'unicode', 1, 'ICU', '', {}] ... for such collations.
>> After the patch default value is set explicitly, so
>> user would observe : ... [1, 'unicode', 1, 'ICU', '',
>> {strength='tertiary'}] ...
>> 
>> Note that box/stat.test.lua is temporary disabled with this
>> patch. It is done so because the patch is meant for the 2.1.2
>> release. Current tarantool version is 2.1.2, so upgrade is done
>> (using upgrade.lua) and because of it box/stat is broken (it
>> does not expect changes in upgrade) But after the release would
>> be made, box/stat would work again, because no changing would be
>> done in upgrade.lua. To resume, after we set tarantool
>> version to => 2.1.2 box/stat should be enabled again.
> 
> 1. Why so complex? Just update box stat test output. We have
> upgrade_to_2_1_2 on some other branches in review, but they do not
> have these problems.
> 
> What is more, I enabled that test back, and it passes. So what
> a problem?
Seems like this test works fine if bootstrap.snap is updated. So I
just enable the test and fix commit message.
> 
>> 
>> Closes #3573
>> 
>> @TarantoolBot document
>> Title: default collation strength is explicit tertiary now
>> Before the patch we already have tertiary strength is default
>> strength for collations, but it was explicit:
> 
> 2. 'It was explicit', 'it's just become explicit'. A guess,
> the first one was 'implicit’.
You are right. Sorry, fixed now.
> 
>> [1, 'unicode', 1, 'ICU', '', {}]
>> After the patch it's just become explicit:
>> 1, 'unicode', 1, 'ICU', '', {'strength' = 'tertiary'}]
>> 
>> Also please fix this https://tarantool.io/en/doc/2.1/book/box/data_model/#collations
>> There is line saying: "unicode collation observes all weights,
>> from L1 to Ln (identical)" It was not true and now this fact
>> would just become obvious.
>> ---
>> Branch https://github.com/tarantool/tarantool/tree/sudobobo/gh-3573-add-explicit-default-coll-strength
>> Issue https://github.com/tarantool/tarantool/issues/3573
>> 
>> src/box/bootstrap.snap             | Bin 1834 -> 1840 bytes
>> src/box/lua/schema.lua             |   4 ++++
>> src/box/lua/upgrade.lua            |  26 ++++++++++++++++++++++++--
>> src/lua/utf8.c                     |   1 +
>> test/app-tap/tarantoolctl.test.lua |  10 +++++-----
>> test/box-py/bootstrap.result       |   2 +-
>> test/box/ddl.result                |   6 +++---
>> test/box/suite.ini                 |   2 +-
>> test/sql/collation.result          |  14 ++++++++++++++
>> test/sql/collation.test.lua        |   8 ++++++++
>> test/unit/coll.cpp                 |   2 ++
>> 11 files changed, 63 insertions(+), 12 deletions(-)
>> 
>> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
>> index dc7328714..440061558 100644
>> --- a/src/box/lua/upgrade.lua
>> +++ b/src/box/lua/upgrade.lua
>> @@ -400,7 +400,7 @@ 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{1, "unicode", ADMIN, "ICU", "", setmap{}}
>> +    box.space._collation:replace{1, "unicode", ADMIN, "ICU", "", {strength='tertiary'}}
> 
> 3. Please, do not touch old upgrade scripts. They are 'read-only’.
Moreover, this change does not really do anything. Removed.
> 
>>     box.space._collation:replace{2, "unicode_ci", ADMIN, "ICU", "", {strength='primary'}}
>> 
>>     local _priv = box.space[box.schema.PRIV_ID]
>> @@ -632,6 +632,27 @@ local function upgrade_to_2_1_1()
>>     end
>> end
>> 
>> +--------------------------------------------------------------------------------
>> +-- Tarantool 2.1.2
>> +--------------------------------------------------------------------------------
>> +
>> +local function update_collation_strength_field()
>> +    local _collation = box.space[box.schema.COLLATION_ID]
>> +    for _, collation in ipairs(_collation:select()) do
>> +        if collation.opts.strength == nil and collation.name ~= 'none' and
>> +            collation.name ~= 'binary' then
>> +            local new_collation = _collation:get{collation.id}:totable()
>> +            new_collation[6].strength = 'tertiary'
>> +            _collation:delete{collation.id}
>> +            _collation:insert(new_collation)
> 
> 4. 'replace’ ?
Replaces are prohibited for _collation space.
>> +        end
>> +    end
>> +end
>> +
>> +local function upgrade_to_2_1_2()
>> +    update_collation_strength_field()
>> +end
>> +
>> local function get_version()
>>     local version = box.space._schema:get{'version'}
>>     if version == nil then
>> diff --git a/test/sql/collation.result b/test/sql/collation.result
>> index 3794990dc..9994baca9 100644
>> --- a/test/sql/collation.result
>> +++ b/test/sql/collation.result
>> @@ -785,3 +785,17 @@ box.sql.execute("SELECT DISTINCT substr(s2, 1, 1) FROM jj;")
>> box.space.JJ:drop()
>> ---
>> ...
>> +-- gh-3573: Strength in the _collation space
>> +-- Collation without 'strength' option set now has explicit
>> +-- 'strength' = 'tertiary'.
>> +--
>> +box.internal.collation.create('c', 'ICU', 'unicode')
>> +---
>> +...
>> +id =  box.internal.collation.id_by_name('c')
>> +---
>> +...
>> +box.space._collation:select(id)
> 
> 5. id_by_name + select can be replaced with one
> box.space._collation.index.name:get({'c'}).
Ok, done.
> 
>> +---
>> +- - [4, 'c', 1, 'ICU', 'unicode', {'strength': 'tertiary'}]
>> +...

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [tarantool-patches] Re: [PATCH] sql: set explicit default collation's strength
  2019-03-27 14:08   ` [tarantool-patches] " i.koptelov
@ 2019-03-27 14:26     ` Vladislav Shpilevoy
  2019-03-27 15:01       ` i.koptelov
  0 siblings, 1 reply; 11+ messages in thread
From: Vladislav Shpilevoy @ 2019-03-27 14:26 UTC (permalink / raw)
  To: i.koptelov, tarantool-patches

Please, apply the diff below. Be careful because I haven't tested it.
Fix it, if tests fail, and do not forget to regenerate boostrap.snap.

diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index a23d0af64..37ab76177 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -638,10 +638,9 @@ end
 
 local function update_collation_strength_field()
     local _collation = box.space[box.schema.COLLATION_ID]
-    for _, collation in ipairs(_collation:select()) do
-        if collation.opts.strength == nil and collation.name ~= 'none' and
-            collation.name ~= 'binary' then
-            local new_collation = _collation:get{collation.id}:totable()
+    for _, collation in _collation:pairs() do
+        if collation.type == 'ICU' and collation.opts.strength == nil then
+            local new_collation = collation:totable()
             new_collation[6].strength = 'tertiary'
             _collation:delete{collation.id}
             _collation:insert(new_collation)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [tarantool-patches] Re: [PATCH] sql: set explicit default collation's strength
  2019-03-27 14:26     ` [tarantool-patches] " Vladislav Shpilevoy
@ 2019-03-27 15:01       ` i.koptelov
  2019-03-27 19:32         ` Vladislav Shpilevoy
  0 siblings, 1 reply; 11+ messages in thread
From: i.koptelov @ 2019-03-27 15:01 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Vladislav Shpilevoy



> On 27 Mar 2019, at 17:26, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Please, apply the diff below. Be careful because I haven't tested it.
> Fix it, if tests fail, and do not forget to regenerate boostrap.snap.
> 
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index a23d0af64..37ab76177 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -638,10 +638,9 @@ end
> 
> local function update_collation_strength_field()
>     local _collation = box.space[box.schema.COLLATION_ID]
> -    for _, collation in ipairs(_collation:select()) do
> -        if collation.opts.strength == nil and collation.name ~= 'none' and
> -            collation.name ~= 'binary' then
> -            local new_collation = _collation:get{collation.id}:totable()
> +    for _, collation in _collation:pairs() do
> +        if collation.type == 'ICU' and collation.opts.strength == nil then
> +            local new_collation = collation:totable()
>             new_collation[6].strength = 'tertiary'
>             _collation:delete{collation.id}
>             _collation:insert(new_collation)
> 
Ok, done. Tests are not broken, bootstrap.snap is regenerated.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [tarantool-patches] Re: [PATCH] sql: set explicit default collation's strength
  2019-03-27 15:01       ` i.koptelov
@ 2019-03-27 19:32         ` Vladislav Shpilevoy
  2019-03-27 19:32           ` Vladislav Shpilevoy
  0 siblings, 1 reply; 11+ messages in thread
From: Vladislav Shpilevoy @ 2019-03-27 19:32 UTC (permalink / raw)
  To: i.koptelov, tarantool-patches



On 27/03/2019 18:01, i.koptelov wrote:
> 
> 
>> On 27 Mar 2019, at 17:26, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>
>> Please, apply the diff below. Be careful because I haven't tested it.
>> Fix it, if tests fail, and do not forget to regenerate boostrap.snap.
>>
>> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
>> index a23d0af64..37ab76177 100644
>> --- a/src/box/lua/upgrade.lua
>> +++ b/src/box/lua/upgrade.lua
>> @@ -638,10 +638,9 @@ end
>>
>> local function update_collation_strength_field()
>>     local _collation = box.space[box.schema.COLLATION_ID]
>> -    for _, collation in ipairs(_collation:select()) do
>> -        if collation.opts.strength == nil and collation.name ~= 'none' and
>> -            collation.name ~= 'binary' then
>> -            local new_collation = _collation:get{collation.id}:totable()
>> +    for _, collation in _collation:pairs() do
>> +        if collation.type == 'ICU' and collation.opts.strength == nil then
>> +            local new_collation = collation:totable()
>>             new_collation[6].strength = 'tertiary'
>>             _collation:delete{collation.id}
>>             _collation:insert(new_collation)
>>
> Ok, done. Tests are not broken, bootstrap.snap is regenerated.
> 

Looks like it is not done. You ignored last two lines: fix of
indentation and fix of excess lookup of a tuple which you already
have from pairs.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [tarantool-patches] Re: [PATCH] sql: set explicit default collation's strength
  2019-03-27 19:32         ` Vladislav Shpilevoy
@ 2019-03-27 19:32           ` Vladislav Shpilevoy
  2019-03-28  6:55             ` i.koptelov
  0 siblings, 1 reply; 11+ messages in thread
From: Vladislav Shpilevoy @ 2019-03-27 19:32 UTC (permalink / raw)
  To: i.koptelov, tarantool-patches



On 27/03/2019 22:32, Vladislav Shpilevoy wrote:
> 
> 
> On 27/03/2019 18:01, i.koptelov wrote:
>>
>>
>>> On 27 Mar 2019, at 17:26, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>>
>>> Please, apply the diff below. Be careful because I haven't tested it.
>>> Fix it, if tests fail, and do not forget to regenerate boostrap.snap.
>>>
>>> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
>>> index a23d0af64..37ab76177 100644
>>> --- a/src/box/lua/upgrade.lua
>>> +++ b/src/box/lua/upgrade.lua
>>> @@ -638,10 +638,9 @@ end
>>>
>>> local function update_collation_strength_field()
>>>     local _collation = box.space[box.schema.COLLATION_ID]
>>> -    for _, collation in ipairs(_collation:select()) do
>>> -        if collation.opts.strength == nil and collation.name ~= 'none' and
>>> -            collation.name ~= 'binary' then
>>> -            local new_collation = _collation:get{collation.id}:totable()
>>> +    for _, collation in _collation:pairs() do
>>> +        if collation.type == 'ICU' and collation.opts.strength == nil then
>>> +            local new_collation = collation:totable()
>>>             new_collation[6].strength = 'tertiary'
>>>             _collation:delete{collation.id}
>>>             _collation:insert(new_collation)
>>>
>> Ok, done. Tests are not broken, bootstrap.snap is regenerated.
>>
> 
> Looks like it is not done. You ignored last two lines: fix of
> indentation and fix of excess lookup of a tuple which you already
> have from pairs.
> 

Sorry, indentation is ok. Only double lookup still is unfixed.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [tarantool-patches] Re: [PATCH] sql: set explicit default collation's strength
  2019-03-27 19:32           ` Vladislav Shpilevoy
@ 2019-03-28  6:55             ` i.koptelov
  2019-03-28 11:45               ` Vladislav Shpilevoy
  0 siblings, 1 reply; 11+ messages in thread
From: i.koptelov @ 2019-03-28  6:55 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Vladislav Shpilevoy



> On 27 Mar 2019, at 22:32, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> 
> 
> On 27/03/2019 22:32, Vladislav Shpilevoy wrote:
>> 
>> 
>> On 27/03/2019 18:01, i.koptelov wrote:
>>> 
>>> 
>>>> On 27 Mar 2019, at 17:26, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>>> 
>>>> Please, apply the diff below. Be careful because I haven't tested it.
>>>> Fix it, if tests fail, and do not forget to regenerate boostrap.snap.
>>>> 
>>>> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
>>>> index a23d0af64..37ab76177 100644
>>>> --- a/src/box/lua/upgrade.lua
>>>> +++ b/src/box/lua/upgrade.lua
>>>> @@ -638,10 +638,9 @@ end
>>>> 
>>>> local function update_collation_strength_field()
>>>>    local _collation = box.space[box.schema.COLLATION_ID]
>>>> -    for _, collation in ipairs(_collation:select()) do
>>>> -        if collation.opts.strength == nil and collation.name ~= 'none' and
>>>> -            collation.name ~= 'binary' then
>>>> -            local new_collation = _collation:get{collation.id}:totable()
>>>> +    for _, collation in _collation:pairs() do
>>>> +        if collation.type == 'ICU' and collation.opts.strength == nil then
>>>> +            local new_collation = collation:totable()
>>>>            new_collation[6].strength = 'tertiary'
>>>>            _collation:delete{collation.id}
>>>>            _collation:insert(new_collation)
>>>> 
>>> Ok, done. Tests are not broken, bootstrap.snap is regenerated.
>>> 
>> 
>> Looks like it is not done. You ignored last two lines: fix of
>> indentation and fix of excess lookup of a tuple which you already
>> have from pairs.
>> 
> 
> Sorry, indentation is ok. Only double lookup still is unfixed.
> 
Sorry, fixed now.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [tarantool-patches] Re: [PATCH] sql: set explicit default collation's strength
  2019-03-28  6:55             ` i.koptelov
@ 2019-03-28 11:45               ` Vladislav Shpilevoy
  0 siblings, 0 replies; 11+ messages in thread
From: Vladislav Shpilevoy @ 2019-03-28 11:45 UTC (permalink / raw)
  To: i.koptelov, tarantool-patches, Kirill Yukhin

LGTM.

On 28/03/2019 09:55, i.koptelov wrote:
> 
> 
>> On 27 Mar 2019, at 22:32, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>
>>
>>
>> On 27/03/2019 22:32, Vladislav Shpilevoy wrote:
>>>
>>>
>>> On 27/03/2019 18:01, i.koptelov wrote:
>>>>
>>>>
>>>>> On 27 Mar 2019, at 17:26, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>>>>
>>>>> Please, apply the diff below. Be careful because I haven't tested it.
>>>>> Fix it, if tests fail, and do not forget to regenerate boostrap.snap.
>>>>>
>>>>> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
>>>>> index a23d0af64..37ab76177 100644
>>>>> --- a/src/box/lua/upgrade.lua
>>>>> +++ b/src/box/lua/upgrade.lua
>>>>> @@ -638,10 +638,9 @@ end
>>>>>
>>>>> local function update_collation_strength_field()
>>>>>    local _collation = box.space[box.schema.COLLATION_ID]
>>>>> -    for _, collation in ipairs(_collation:select()) do
>>>>> -        if collation.opts.strength == nil and collation.name ~= 'none' and
>>>>> -            collation.name ~= 'binary' then
>>>>> -            local new_collation = _collation:get{collation.id}:totable()
>>>>> +    for _, collation in _collation:pairs() do
>>>>> +        if collation.type == 'ICU' and collation.opts.strength == nil then
>>>>> +            local new_collation = collation:totable()
>>>>>            new_collation[6].strength = 'tertiary'
>>>>>            _collation:delete{collation.id}
>>>>>            _collation:insert(new_collation)
>>>>>
>>>> Ok, done. Tests are not broken, bootstrap.snap is regenerated.
>>>>
>>>
>>> Looks like it is not done. You ignored last two lines: fix of
>>> indentation and fix of excess lookup of a tuple which you already
>>> have from pairs.
>>>
>>
>> Sorry, indentation is ok. Only double lookup still is unfixed.
>>
> Sorry, fixed now.
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [tarantool-patches] Re: [PATCH] sql: set explicit default collation's strength
  2019-03-26 20:34 [tarantool-patches] [PATCH] sql: set explicit default collation's strength Ivan Koptelov
  2019-03-27  9:53 ` [tarantool-patches] " i.koptelov
  2019-03-27 11:59 ` Vladislav Shpilevoy
@ 2019-03-28 12:02 ` Kirill Yukhin
  2 siblings, 0 replies; 11+ messages in thread
From: Kirill Yukhin @ 2019-03-28 12:02 UTC (permalink / raw)
  To: tarantool-patches; +Cc: v.shpilevoy, Ivan Koptelov

Hello,

On 26 Mar 23:34, Ivan Koptelov wrote:
> Before the patch, collations with no strength set used
> tertiary strength. But it was not easy to understand it,
> because box.space._collation:select{} would return
> ... [1, 'unicode', 1, 'ICU', '', {}] ... for such collations.
> After the patch default value is set explicitly, so
> user would observe : ... [1, 'unicode', 1, 'ICU', '',
> {strength='tertiary'}] ...
> 
> Note that box/stat.test.lua is temporary disabled with this
> patch. It is done so because the patch is meant for the 2.1.2
> release. Current tarantool version is 2.1.2, so upgrade is done
> (using upgrade.lua) and because of it box/stat is broken (it
> does not expect changes in upgrade) But after the release would
> be made, box/stat would work again, because no changing would be
> done in upgrade.lua. To resume, after we set tarantool
> version to => 2.1.2 box/stat should be enabled again.
> 
> Closes #3573
> 
> @TarantoolBot document
> Title: default collation strength is explicit tertiary now
> Before the patch we already have tertiary strength is default
> strength for collations, but it was explicit:
> [1, 'unicode', 1, 'ICU', '', {}]
> After the patch it's just become explicit:
> 1, 'unicode', 1, 'ICU', '', {'strength' = 'tertiary'}]
> 
> Also please fix this https://tarantool.io/en/doc/2.1/book/box/data_model/#collations
> There is line saying: "unicode collation observes all weights,
> from L1 to Ln (identical)" It was not true and now this fact
> would just become obvious.
> ---
> Branch https://github.com/tarantool/tarantool/tree/sudobobo/gh-3573-add-explicit-default-coll-strength
> Issue https://github.com/tarantool/tarantool/issues/3573

I've checked your patch into master & 2.1 branch.

--
Regards, Kirill Yukhin

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-03-28 12:02 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-26 20:34 [tarantool-patches] [PATCH] sql: set explicit default collation's strength Ivan Koptelov
2019-03-27  9:53 ` [tarantool-patches] " i.koptelov
2019-03-27 11:59 ` Vladislav Shpilevoy
2019-03-27 14:08   ` [tarantool-patches] " i.koptelov
2019-03-27 14:26     ` [tarantool-patches] " Vladislav Shpilevoy
2019-03-27 15:01       ` i.koptelov
2019-03-27 19:32         ` Vladislav Shpilevoy
2019-03-27 19:32           ` Vladislav Shpilevoy
2019-03-28  6:55             ` i.koptelov
2019-03-28 11:45               ` Vladislav Shpilevoy
2019-03-28 12:02 ` Kirill Yukhin

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