From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Mergen Imeev <imeevma@tarantool.org>,
Nikita Pettik <korablev@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 1/1] sql: make NUMBER to be union of SQL numeric types
Date: Mon, 10 Feb 2020 00:29:32 +0300 [thread overview]
Message-ID: <20200209212932.bwo7bzovelxcog2z@tkn_work_nb> (raw)
In-Reply-To: <20200203095210.GA7696@tarantool.org>
On Mon, Feb 03, 2020 at 12:52:10PM +0300, Mergen Imeev wrote:
> On Tue, Jan 28, 2020 at 07:23:29PM +0300, Nikita Pettik wrote:
> > On 23 Jan 16:40, Mergen Imeev wrote:
> > > Hi! Thank you for review. My answers and new commit-message
> > > below.
> >
> > Hi,
> >
> > I've a bit reworked your patch: splitted it into consistent sequence of
> > independent patches and provided refactoring for sqlVdbeMemNumerify().
> > Also I found that CAST boolean AS NUMBER didn't work properly: cast resulted
> > in error meanshile cast boolean as integer is legal, so that as number should
> > be legal as well. There's no more functional changes except for mentioned one.
> > Please look at branch: https://github.com/tarantool/tarantool/commits/np/gh-4233-fix-number-field-type-in-sql
> >
> > If it is OK to you please reply and I will push patches to master bracnch.
> > Thanks.
> >
> Hi,
>
> I'm sorry, but I do not like it. Since now it is yours
> commits, I'll ask Sasha to move issues #4233 and #4463
> to you. If you want a review, please ask Vlad.
I guess Nikita expects that you'll agree with splitting of the commits
and show you the code to save a time on a discussion. Since you
disagreed (this fast path fails), I think Nikita should provide
objections against your way explicitly. (However 'I do not like' is very
weak argument, it is always better to provide some reasons explicitly.)
'CAST boolean AS NUMBER' does not look as part of tasks Mergen solving
here (#4233 and #4463). Aside of this, the commit that fixes it is named
as 'sql: refactor sqlVdbeMemNumerify()', but it changes a user visible
behaviour and so it is not pure refactoring. It is also strange that it
has no tests that will show how the behaviour is changed.
Changes related to Tarantool/SQL type system are very opaque now in the
sense that we have no formal type system description we want to
implement as result. Just 'fix that' and 'change this'. I think we
should design the type system first (we should did it a year ago) and
only then make some changes. CCed Kirill and Sergos to plan this
activity (or decide that the current way to design it is okay).
WBR, Alexander Turenko.
next prev parent reply other threads:[~2020-02-09 21:29 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-31 8:50 imeevma
2019-12-31 8:56 ` Nikita Pettik
2019-12-31 9:46 ` Mergen Imeev
2020-01-13 12:58 ` Nikita Pettik
2020-01-17 8:06 ` Mergen Imeev
2020-01-22 13:26 ` Nikita Pettik
2020-01-23 13:40 ` Mergen Imeev
2020-01-28 16:23 ` Nikita Pettik
2020-02-03 9:52 ` Mergen Imeev
2020-02-04 12:56 ` Nikita Pettik
2020-02-09 21:29 ` Alexander Turenko [this message]
2020-02-09 23:45 ` Nikita Pettik
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200209212932.bwo7bzovelxcog2z@tkn_work_nb \
--to=alexander.turenko@tarantool.org \
--cc=imeevma@tarantool.org \
--cc=korablev@tarantool.org \
--cc=tarantool-patches@dev.tarantool.org \
--cc=v.shpilevoy@tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH v2 1/1] sql: make NUMBER to be union of SQL numeric types' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox