Re: [Tarantool-patches] [PATCH 2/3] sql: enabled ANY as target for explicit conversions

[Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

: From: Mergen Imeev <imeevma@tarantool.org>
: Sent: Tuesday, June 1, 2021 5:03 PM
: To: Timur Safin <tsafin@tarantool.org>
: Cc: tarantool-patches@dev.tarantool.org
: Subject: Re: [PATCH 2/3] sql: enabled ANY as target for explicit conversions
: 
: Thank you for the patch. See 10 comments below.
: 
: 1. I believe we do not have field type ANY in SQL. Also, I believe that
: to add
: it to explicit cast we have to add it to SQL.

Yes, there is no way to declare field of ANY type, or create such literal. But
As result of discussion, for consistency sake (with SCALAR) it was decided to still allow 
explicit cast to ANY, just as plain noop.

: 
: 2. What this patch actually does? I mean, after this patch I still not able
: to cast values to ANY:
: 
: tarantool> box.execute('SELECT CAST(1 AS ANY);')
: ---
: - null
: - 'At line 1 at or near position 18: keyword ''ANY'' is reserved. Please
: use double
:    quotes if ''ANY'' is an identifier.'
: ...

That's odd - I've seen such behavior when there was no renamed %wildcard ANY and 
Old generated parser code. It was working for me with that old patch, but will check.


: 
: On Tue, May 25, 2021 at 12:01:01PM +0300, Timur Safin wrote:
: > For consistency sake it was decided to provide
: > `CAST(anything TO ANY)` as allowed, but still noop.
: >
: > Fuller explicit conversions table is described in the
: > /doc/rfc/5910-consistent-sql-lua-types.md and here we
: > represent only relevant to ANY.
: >

: 3. I still think that it is better to put RFC name instead of  this relative
: link.

Whatever.

: 
: > Furthermore, there is no direct way in SQL to create literal and
: > expression of ANY type, thus there is no defined CAST from ANY
: > to anything else.
: >

: 4. There is no way to create literal, however you can use space with
: field type
: ANY, right?

Nope, SQL is strict-typed language at runtime. ANY is NoSQL type, and
there should be no way to declare untyped field. But if, by any chance,
we have  received data of `any` type from Lua side, then we should have
some means to at least cast to it expression.

: 
: >               | 0 | 1 | 2 | 4 | 5 | 6 | 7 | 3 | 9 |10 |11 |12 | 8 |
: >               +---+---+---+---+---+---+---+---+---+---+---+---+---+
: >  0.       any |   |   |   |   |   |   |   |   |   |   |   |   |   |
: >  1.  unsigned |   | . | . | . | . |   |   | . |   |   |   |   | Y |
: >  2.    string |   | . | . | . | . | . | . | . |   |   |   |   | Y |
: >  4.    double |   | . | . | . | . |   |   | . |   |   |   |   | Y |
: >  5.   integer |   | . | . | . | . |   |   | . |   |   |   |   | Y |
: >  6.   boolean |   |   | . |   |   | . |   |   |   |   |   |   | Y |
: >  7. varbinary |   |   | . |   |   |   | . |   |   |   |   |   | Y |
: >  3.    number |   | . | . | . | . |   |   | . |   |   |   |   | Y |
: >  9.   decimal |   |   |   |   |   |   |   |   |   |   |   |   |   |
: > 10.      uuid |   |   |   |   |   |   |   |   |   |   |   |   |   |
: > 11.     array |   |   |   |   |   |   |   |   |   |   |   |   |   |
: > 12.       map |   |   |   |   |   |   |   |   |   |   |   |   |   |
: >  8.    scalar |   | . | . | . | . | . | . | . |   |   |   |   | Y |
: >               +---+---+---+---+---+---+---+---+---+---+---+---+---+
: >
: 5. This strange order again.

This order is from illustrations of the spec. It's too late to redraw
all pictures there, but reordering here is easy. So we need to live 
with this order forever :(

: 
: 6. Again, do you really need this table in commit-message? This time it
: is also
: a lot simpler to just describe this explicit conversion rules using words.

Picture worth thousand words. You tend to prefer wording, many prefer
pictures.

: 
: > * We introduced new token ANY as known lexem to the Lemon parser.
: >   While renamed prior %wildcard to ANYTHING;
: > * Modified explicit conversion rules to allow ANY as _target_ in
: >   conversion pair.
: >
: > Relates to #5910, #6009
: > Part of #4407
: 7. Wrong issue number. Also, I am again not sure that you should include
: "Relates to" here.

Relates will allow us to keep them linked with parent specification.
Even if there will be multiple steps and commits.

: 
: > ---
: >  extra/mkkeywordhash.c          | 3 ++-
: >  src/box/sql/mem.c              | 2 ++
: >  src/box/sql/parse.y            | 3 ++-
: >  test/sql-tap/keyword1.test.lua | 2 +-
: >  4 files changed, 7 insertions(+), 3 deletions(-)
: >
: > diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
: > index 7480c0211..d343cf706 100644
: > --- a/extra/mkkeywordhash.c
: > +++ b/extra/mkkeywordhash.c
: > @@ -60,6 +60,7 @@ static Keyword aKeywordTable[] = {
: >    { "ALL",                    "TK_ALL",         true  },
: >    { "ALTER",                  "TK_ALTER",       true  },
: >    { "ANALYZE",                "TK_STANDARD",    true  },
: > +  { "ANY",                    "TK_ID",          true  },
: 8. Why some types describes as TK_ID, and some as TK_<type name>?
: 
: >    { "AND",                    "TK_AND",         true  },
: >    { "AS",                     "TK_AS",          true  },
: >    { "ASC",                    "TK_ASC",         true  },
: > @@ -178,7 +179,7 @@ static Keyword aKeywordTable[] = {
: >    { "WITH",                   "TK_WITH",        true  },
: >    { "WHEN",                   "TK_WHEN",        true  },
: >    { "WHERE",                  "TK_WHERE",       true  },
: > -  { "ANY",                    "TK_STANDARD",    true  },
: > +  { "ANYTHING",               "TK_STANDARD",    true  },
: 9. Why not name it as TK_WILDCARD to avoid further misunderstanding?
: 
: >    { "ASENSITIVE",             "TK_STANDARD",    true  },
: >    { "BLOB",                   "TK_STANDARD",    true  },
: >    { "BINARY",                 "TK_ID",          true  },
: > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
: > index d40366754..b4f1078ae 100644
: > --- a/src/box/sql/mem.c
: > +++ b/src/box/sql/mem.c
: > @@ -874,6 +874,8 @@ mem_cast_explicit(struct Mem *mem, enum field_type
: type)
: >  		    (mem->flags & MEM_Subtype) != 0)
: >  			return -1;
: >  		return 0;
: > +	case FIELD_TYPE_ANY:
: > +		return 0;
: >  	default:
: >  		break;
: >  	}
: > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
: > index abc363951..e586d0181 100644
: > --- a/src/box/sql/parse.y
: > +++ b/src/box/sql/parse.y
: > @@ -270,7 +270,7 @@ columnlist ::= tcons.
: >    QUERY KEY OFFSET RAISE RELEASE REPLACE RESTRICT
: >    RENAME CTIME_KW IF ENABLE DISABLE
: >    .
: > -%wildcard ANY.
: > +%wildcard ANYTHING.
: >
: >
: >  // And "ids" is an identifer-or-string.
: > @@ -1834,6 +1834,7 @@ typedef(A) ::= SCALAR . { A.type =
: FIELD_TYPE_SCALAR; }
: >  typedef(A) ::= BOOL . { A.type = FIELD_TYPE_BOOLEAN; }
: >  typedef(A) ::= BOOLEAN . { A.type = FIELD_TYPE_BOOLEAN; }
: >  typedef(A) ::= VARBINARY . { A.type = FIELD_TYPE_VARBINARY; }
: > +typedef(A) ::= ANY . { A.type = FIELD_TYPE_ANY; }
: >
: >  /**
: >   * Time-like types are temporary disabled, until they are
: > diff --git a/test/sql-tap/keyword1.test.lua b/test/sql-
: tap/keyword1.test.lua
: > index f9a9c6865..4855f3ebc 100755
: > --- a/test/sql-tap/keyword1.test.lua
: > +++ b/test/sql-tap/keyword1.test.lua
: > @@ -242,7 +242,7 @@ for _, kw in ipairs(bannedkws) do
: >      local query = 'CREATE TABLE '..kw..'(a INT PRIMARY KEY);'
: >      if kw == 'end' or kw == 'match' or kw == 'release' or kw == 'rename'
: or
: >         kw == 'replace' or kw == 'binary' or kw == 'character' or
: > -       kw == 'smallint' then
: > +       kw == 'smallint' or kw == 'any' then
: >          test:do_catchsql_test(
: >          "bannedkw1-"..kw..".1",
: >          query, {
: > --
: > 2.29.2
: >


: 10. Why there is no tests?

All newly introduced modifications extensively covered in e_casts.test.lua
test you see the other patch. [And I really mean "extensively", because test matrix
there covers hundreds and hundreds of test cases, which is impossible to cover 
with manual result-based test. Regardless how hard you try and how careful you are]

I'll merge all explicit conversion code changes with corresponding part of e_casts.test.lua
if we had to have test in the same commit.

Timur