* [PATCH] lua: fix decimal comparison with box.NULL @ 2019-08-27 11:22 Serge Petrenko 2019-08-27 12:04 ` Vladimir Davydov 0 siblings, 1 reply; 5+ messages in thread From: Serge Petrenko @ 2019-08-27 11:22 UTC (permalink / raw) To: vdavydov.dev; +Cc: tarantool-patches, Serge Petrenko This problem is similar to the one fixed in commit 3c6c1cc96a1a510493c21c472565d4063e403ba2 (lua:fix decimal comparison with nil) We should handle box.NULL the same way. Closes #4454 --- https://github.com/tarantool/tarantool/issues/4454 https://github.com/tarantool/tarantool/tree/sp/gh-4454-decimal-cmp-null src/lua/decimal.c | 3 ++- test/app/decimal.result | 30 ++++++++++++++++++++++++++++++ test/app/decimal.test.lua | 8 ++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/lua/decimal.c b/src/lua/decimal.c index 23e50ba68..8905a0b9d 100644 --- a/src/lua/decimal.c +++ b/src/lua/decimal.c @@ -235,7 +235,8 @@ static int ldecimal_eq(struct lua_State *L) { assert(lua_gettop(L) == 2); - if (lua_isnil(L, 1) || lua_isnil(L, 2)) { + if (lua_isnil(L, 1) || lua_isnil(L, 2) || + luaL_isnull(L, 1) || luaL_isnull(L, 2)) { lua_pushboolean(L, false); return 1; } diff --git a/test/app/decimal.result b/test/app/decimal.result index 8251e13d8..90d0984b0 100644 --- a/test/app/decimal.result +++ b/test/app/decimal.result @@ -248,6 +248,36 @@ a <= nil | --- | - error: '[string "return a <= nil "]:1: attempt to compare decimal with nil' | ... +-- and with box.NULL +-- +a == box.NULL + | --- + | - false + | ... +a ~= box.NULL + | --- + | - true + | ... +a > box.NULL + | --- + | - error: '[string "return a > box.NULL "]:1: expected decimal, number or string as + | 1 argument' + | ... +a < box.NULL + | --- + | - error: '[string "return a < box.NULL "]:1: expected decimal, number or string as + | 2 argument' + | ... +a >= box.NULL + | --- + | - error: '[string "return a >= box.NULL "]:1: expected decimal, number or string as + | 1 argument' + | ... +a <= box.NULL + | --- + | - error: '[string "return a <= box.NULL "]:1: expected decimal, number or string as + | 2 argument' + | ... decimal.sqrt(a) | --- diff --git a/test/app/decimal.test.lua b/test/app/decimal.test.lua index d83522b45..79189a2f5 100644 --- a/test/app/decimal.test.lua +++ b/test/app/decimal.test.lua @@ -69,6 +69,14 @@ a > nil a < nil a >= nil a <= nil +-- and with box.NULL +-- +a == box.NULL +a ~= box.NULL +a > box.NULL +a < box.NULL +a >= box.NULL +a <= box.NULL decimal.sqrt(a) decimal.ln(a) -- 2.20.1 (Apple Git-117) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] lua: fix decimal comparison with box.NULL 2019-08-27 11:22 [PATCH] lua: fix decimal comparison with box.NULL Serge Petrenko @ 2019-08-27 12:04 ` Vladimir Davydov 2019-08-27 12:10 ` [tarantool-patches] " Serge Petrenko 0 siblings, 1 reply; 5+ messages in thread From: Vladimir Davydov @ 2019-08-27 12:04 UTC (permalink / raw) To: Serge Petrenko; +Cc: tarantool-patches On Tue, Aug 27, 2019 at 02:22:09PM +0300, Serge Petrenko wrote: > This problem is similar to the one fixed in commit > 3c6c1cc96a1a510493c21c472565d4063e403ba2 (lua:fix decimal comparison > with nil) > We should handle box.NULL the same way. > > Closes #4454 > --- > https://github.com/tarantool/tarantool/issues/4454 > https://github.com/tarantool/tarantool/tree/sp/gh-4454-decimal-cmp-null > > src/lua/decimal.c | 3 ++- > test/app/decimal.result | 30 ++++++++++++++++++++++++++++++ > test/app/decimal.test.lua | 8 ++++++++ > 3 files changed, 40 insertions(+), 1 deletion(-) > > diff --git a/src/lua/decimal.c b/src/lua/decimal.c > index 23e50ba68..8905a0b9d 100644 > --- a/src/lua/decimal.c > +++ b/src/lua/decimal.c > @@ -235,7 +235,8 @@ static int > ldecimal_eq(struct lua_State *L) > { > assert(lua_gettop(L) == 2); > - if (lua_isnil(L, 1) || lua_isnil(L, 2)) { > + if (lua_isnil(L, 1) || lua_isnil(L, 2) || > + luaL_isnull(L, 1) || luaL_isnull(L, 2)) { > lua_pushboolean(L, false); > return 1; > } > diff --git a/test/app/decimal.result b/test/app/decimal.result > index 8251e13d8..90d0984b0 100644 > --- a/test/app/decimal.result > +++ b/test/app/decimal.result > @@ -248,6 +248,36 @@ a <= nil > | --- > | - error: '[string "return a <= nil "]:1: attempt to compare decimal with nil' > | ... > +-- and with box.NULL > +-- > +a == box.NULL > + | --- > + | - false > + | ... > +a ~= box.NULL > + | --- > + | - true > + | ... > +a > box.NULL > + | --- > + | - error: '[string "return a > box.NULL "]:1: expected decimal, number or string as > + | 1 argument' > + | ... 1 argument? > +a < box.NULL > + | --- > + | - error: '[string "return a < box.NULL "]:1: expected decimal, number or string as > + | 2 argument' > + | ... 2 argument? Why does the error message differ for the two seemingly equivalent cases? What's "1 argument"? Note, in case of comparison with nil the error message is different: | - error: '[string "return a > nil "]:1: attempt to compare decimal with nil' | - error: '[string "return a < nil "]:1: attempt to compare decimal with nil' ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [tarantool-patches] Re: [PATCH] lua: fix decimal comparison with box.NULL 2019-08-27 12:04 ` Vladimir Davydov @ 2019-08-27 12:10 ` Serge Petrenko 2019-08-27 12:25 ` Vladimir Davydov 0 siblings, 1 reply; 5+ messages in thread From: Serge Petrenko @ 2019-08-27 12:10 UTC (permalink / raw) To: Vladimir Davydov; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 2805 bytes --] > 27 авг. 2019 г., в 15:04, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а): > > On Tue, Aug 27, 2019 at 02:22:09PM +0300, Serge Petrenko wrote: >> This problem is similar to the one fixed in commit >> 3c6c1cc96a1a510493c21c472565d4063e403ba2 (lua:fix decimal comparison >> with nil) >> We should handle box.NULL the same way. >> >> Closes #4454 >> --- >> https://github.com/tarantool/tarantool/issues/4454 >> https://github.com/tarantool/tarantool/tree/sp/gh-4454-decimal-cmp-null >> >> src/lua/decimal.c | 3 ++- >> test/app/decimal.result | 30 ++++++++++++++++++++++++++++++ >> test/app/decimal.test.lua | 8 ++++++++ >> 3 files changed, 40 insertions(+), 1 deletion(-) >> >> diff --git a/src/lua/decimal.c b/src/lua/decimal.c >> index 23e50ba68..8905a0b9d 100644 >> --- a/src/lua/decimal.c >> +++ b/src/lua/decimal.c >> @@ -235,7 +235,8 @@ static int >> ldecimal_eq(struct lua_State *L) >> { >> assert(lua_gettop(L) == 2); >> - if (lua_isnil(L, 1) || lua_isnil(L, 2)) { >> + if (lua_isnil(L, 1) || lua_isnil(L, 2) || >> + luaL_isnull(L, 1) || luaL_isnull(L, 2)) { >> lua_pushboolean(L, false); >> return 1; >> } >> diff --git a/test/app/decimal.result b/test/app/decimal.result >> index 8251e13d8..90d0984b0 100644 >> --- a/test/app/decimal.result >> +++ b/test/app/decimal.result >> @@ -248,6 +248,36 @@ a <= nil >> | --- >> | - error: '[string "return a <= nil "]:1: attempt to compare decimal with nil' >> | ... >> +-- and with box.NULL >> +-- >> +a == box.NULL >> + | --- >> + | - false >> + | ... >> +a ~= box.NULL >> + | --- >> + | - true >> + | ... >> +a > box.NULL >> + | --- >> + | - error: '[string "return a > box.NULL "]:1: expected decimal, number or string as >> + | 1 argument' >> + | ... > > 1 argument? 1st argument > >> +a < box.NULL >> + | --- >> + | - error: '[string "return a < box.NULL "]:1: expected decimal, number or string as >> + | 2 argument' >> + | ... > > 2 argument? 2nd argument > > Why does the error message differ for the two seemingly equivalent > cases? What's "1 argument"? > > Note, in case of comparison with nil the error message is different: > > | - error: '[string "return a > nil "]:1: attempt to compare decimal with nil' > | - error: '[string "return a < nil "]:1: attempt to compare decimal with nil’ When you say ‘5 < nil’ you get an error ‘attempt to compare number with nil’ However, when you say ‘5 < box.NULL’ you get attempt to compare number with ‘void *’. So I tried to be consistent. Kind of. And just leave the message as it is. We can say ‘attempt to compare decimal with box.NULL` or with `void *` if you wish. -- Serge Petrenko sergepetrenko@tarantool.org [-- Attachment #2: Type: text/html, Size: 13266 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [tarantool-patches] Re: [PATCH] lua: fix decimal comparison with box.NULL 2019-08-27 12:10 ` [tarantool-patches] " Serge Petrenko @ 2019-08-27 12:25 ` Vladimir Davydov 2019-08-27 12:37 ` Serge Petrenko 0 siblings, 1 reply; 5+ messages in thread From: Vladimir Davydov @ 2019-08-27 12:25 UTC (permalink / raw) To: Serge Petrenko; +Cc: tarantool-patches On Tue, Aug 27, 2019 at 03:10:47PM +0300, Serge Petrenko wrote: > > > 27 авг. 2019 г., в 15:04, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а): > > > > On Tue, Aug 27, 2019 at 02:22:09PM +0300, Serge Petrenko wrote: > >> This problem is similar to the one fixed in commit > >> 3c6c1cc96a1a510493c21c472565d4063e403ba2 (lua:fix decimal comparison > >> with nil) > >> We should handle box.NULL the same way. > >> > >> Closes #4454 > >> --- > >> https://github.com/tarantool/tarantool/issues/4454 > >> https://github.com/tarantool/tarantool/tree/sp/gh-4454-decimal-cmp-null > >> > >> src/lua/decimal.c | 3 ++- > >> test/app/decimal.result | 30 ++++++++++++++++++++++++++++++ > >> test/app/decimal.test.lua | 8 ++++++++ > >> 3 files changed, 40 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/lua/decimal.c b/src/lua/decimal.c > >> index 23e50ba68..8905a0b9d 100644 > >> --- a/src/lua/decimal.c > >> +++ b/src/lua/decimal.c > >> @@ -235,7 +235,8 @@ static int > >> ldecimal_eq(struct lua_State *L) > >> { > >> assert(lua_gettop(L) == 2); > >> - if (lua_isnil(L, 1) || lua_isnil(L, 2)) { > >> + if (lua_isnil(L, 1) || lua_isnil(L, 2) || > >> + luaL_isnull(L, 1) || luaL_isnull(L, 2)) { > >> lua_pushboolean(L, false); > >> return 1; > >> } > >> diff --git a/test/app/decimal.result b/test/app/decimal.result > >> index 8251e13d8..90d0984b0 100644 > >> --- a/test/app/decimal.result > >> +++ b/test/app/decimal.result > >> @@ -248,6 +248,36 @@ a <= nil > >> | --- > >> | - error: '[string "return a <= nil "]:1: attempt to compare decimal with nil' > >> | ... > >> +-- and with box.NULL > >> +-- > >> +a == box.NULL > >> + | --- > >> + | - false > >> + | ... > >> +a ~= box.NULL > >> + | --- > >> + | - true > >> + | ... > >> +a > box.NULL > >> + | --- > >> + | - error: '[string "return a > box.NULL "]:1: expected decimal, number or string as > >> + | 1 argument' > >> + | ... > > > > 1 argument? > > 1st argument > > > > >> +a < box.NULL > >> + | --- > >> + | - error: '[string "return a < box.NULL "]:1: expected decimal, number or string as > >> + | 2 argument' > >> + | ... > > > > 2 argument? > > 2nd argument But in both cases box.NULL is the second argument, isn't it? Anyway, looks like it's a different issue, because I get the same confusing error message while trying to compare a decimal with, say, a string. We can fix it in a separate patch if we need to. Pushed the patch to the master branch as is. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [tarantool-patches] Re: [PATCH] lua: fix decimal comparison with box.NULL 2019-08-27 12:25 ` Vladimir Davydov @ 2019-08-27 12:37 ` Serge Petrenko 0 siblings, 0 replies; 5+ messages in thread From: Serge Petrenko @ 2019-08-27 12:37 UTC (permalink / raw) To: Vladimir Davydov; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 3433 bytes --] > 27 авг. 2019 г., в 15:25, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а): > > On Tue, Aug 27, 2019 at 03:10:47PM +0300, Serge Petrenko wrote: >> >>> 27 авг. 2019 г., в 15:04, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а): >>> >>> On Tue, Aug 27, 2019 at 02:22:09PM +0300, Serge Petrenko wrote: >>>> This problem is similar to the one fixed in commit >>>> 3c6c1cc96a1a510493c21c472565d4063e403ba2 (lua:fix decimal comparison >>>> with nil) >>>> We should handle box.NULL the same way. >>>> >>>> Closes #4454 >>>> --- >>>> https://github.com/tarantool/tarantool/issues/4454 >>>> https://github.com/tarantool/tarantool/tree/sp/gh-4454-decimal-cmp-null >>>> >>>> src/lua/decimal.c | 3 ++- >>>> test/app/decimal.result | 30 ++++++++++++++++++++++++++++++ >>>> test/app/decimal.test.lua | 8 ++++++++ >>>> 3 files changed, 40 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/src/lua/decimal.c b/src/lua/decimal.c >>>> index 23e50ba68..8905a0b9d 100644 >>>> --- a/src/lua/decimal.c >>>> +++ b/src/lua/decimal.c >>>> @@ -235,7 +235,8 @@ static int >>>> ldecimal_eq(struct lua_State *L) >>>> { >>>> assert(lua_gettop(L) == 2); >>>> - if (lua_isnil(L, 1) || lua_isnil(L, 2)) { >>>> + if (lua_isnil(L, 1) || lua_isnil(L, 2) || >>>> + luaL_isnull(L, 1) || luaL_isnull(L, 2)) { >>>> lua_pushboolean(L, false); >>>> return 1; >>>> } >>>> diff --git a/test/app/decimal.result b/test/app/decimal.result >>>> index 8251e13d8..90d0984b0 100644 >>>> --- a/test/app/decimal.result >>>> +++ b/test/app/decimal.result >>>> @@ -248,6 +248,36 @@ a <= nil >>>> | --- >>>> | - error: '[string "return a <= nil "]:1: attempt to compare decimal with nil' >>>> | ... >>>> +-- and with box.NULL >>>> +-- >>>> +a == box.NULL >>>> + | --- >>>> + | - false >>>> + | ... >>>> +a ~= box.NULL >>>> + | --- >>>> + | - true >>>> + | ... >>>> +a > box.NULL >>>> + | --- >>>> + | - error: '[string "return a > box.NULL "]:1: expected decimal, number or string as >>>> + | 1 argument' >>>> + | ... >>> >>> 1 argument? >> >> 1st argument >> >>> >>>> +a < box.NULL >>>> + | --- >>>> + | - error: '[string "return a < box.NULL "]:1: expected decimal, number or string as >>>> + | 2 argument' >>>> + | ... >>> >>> 2 argument? >> >> 2nd argument > > But in both cases box.NULL is the second argument, isn't it? > > Anyway, looks like it's a different issue, because I get the same > confusing error message while trying to compare a decimal with, say, > a string. We can fix it in a separate patch if we need to. Ah, I see. Sorry for the misunderstanding. Yes, here’s the reason for it: (an extract from https://www.lua.org/manual/5.1/manual.html <https://www.lua.org/manual/5.1/manual.html>) `a >= b is equivalent to b <= a. Note that, in the absence of a "le" metamethod, Lua tries the "lt", assuming that a <= b is equivalent to not (b < a).` `a > b is equivalent to b < a.` So, a > box.NULL actually calls `decimal_lt(box.NULL, a)`. That’s why argument numbers get messed up. AFAIR you don’t have `__gt` and `__ge` metamethods in lua. I guess we should just get rid of argument numbers, in error messages here, because I don’t see how this can be fixed otherwise. > > Pushed the patch to the master branch as is. Thanks! -- Serge Petrenko sergepetrenko@tarantool.org [-- Attachment #2: Type: text/html, Size: 11029 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-08-27 12:37 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-27 11:22 [PATCH] lua: fix decimal comparison with box.NULL Serge Petrenko 2019-08-27 12:04 ` Vladimir Davydov 2019-08-27 12:10 ` [tarantool-patches] " Serge Petrenko 2019-08-27 12:25 ` Vladimir Davydov 2019-08-27 12:37 ` Serge Petrenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox