Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Roman Khabibov <roman.habibov@tarantool.org>,
	tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH 1/2] sql: fix collation node duplication in AST
Date: Sun, 12 May 2019 19:32:49 +0300	[thread overview]
Message-ID: <d15b9a9e-9e9a-8033-c89e-00da049011cd@tarantool.org> (raw)
In-Reply-To: <c87fe7d113b4c3710c3eca299824caea5346ec5b.1557312975.git.roman.habibov@tarantool.org>

Hi! Thanks for the patch! Looks like the problem in the patch
is much easier to reproduce, without 'EXCEPT':

    box.execute('SELECT s COLLATE "unicode_ci" FROM a ORDER BY 1 COLLATE "unicode_ci"')

Just use ORDER BY <number> + COLLATE.

But having got this simpler test I found that you broke another one.
Look at this schema:

    box.cfg{}
    box.execute('CREATE TABLE a (id INT PRIMARY KEY, s TEXT)')
    box.execute("INSERT INTO a VALUES (1, 'B'), (2, 'b')")

Before your patch:

    tarantool> box.execute('SELECT s COLLATE "unicode_ci" FROM a ORDER BY 1 COLLATE "unicode"')
    ---
    - metadata:
      - name: s COLLATE "unicode_ci"
        type: string
      rows:
      - ['b']
      - ['B']
    ...


After your patch:

    tarantool> box.execute('SELECT s COLLATE "unicode_ci" FROM a ORDER BY 1 COLLATE "unicode"')
    ---
    - metadata:
      - name: s COLLATE "unicode_ci"
        type: string
      rows:
      - ['B']
      - ['b']
    ...

Result set order has changed. This is because you ignore a new collation
regardless of a name of a previous one. It means, that the patch should not
be applied as is.

We should either

    1) Replace the old collation with a new one, as an optimization;
    2) Left as is now.

I vote for the second as the simplest, and add a test provided by me
above to ensure we will never break it accidentally.

This commit should consist of the test and a comment in resolveAlias.
And keep this nice ASCII schema.

  reply	other threads:[~2019-05-12 16:32 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-08 11:29 [tarantool-patches] [PATCH 0/2] sql: allow <COLLATE> only for string-like args Roman Khabibov
2019-05-08 11:29 ` [tarantool-patches] [PATCH 1/2] sql: fix collation node duplication in AST Roman Khabibov
2019-05-12 16:32   ` Vladislav Shpilevoy [this message]
2019-05-28 14:10     ` [tarantool-patches] " Roman Khabibov
2019-06-02 17:09       ` Vladislav Shpilevoy
2019-06-04 14:00         ` Roman Khabibov
2019-05-08 11:29 ` [tarantool-patches] [PATCH 2/2] sql: allow <COLLATE> only for string-like args Roman Khabibov
2019-05-12 16:32   ` [tarantool-patches] " Vladislav Shpilevoy
2019-05-28 14:08     ` Roman Khabibov
2019-06-02 17:09       ` Vladislav Shpilevoy
2019-06-04 14:27         ` Roman Khabibov
2019-06-04 19:49           ` Vladislav Shpilevoy
2019-06-07 15:01             ` Roman Khabibov
2019-06-09 16:55               ` Vladislav Shpilevoy

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=d15b9a9e-9e9a-8033-c89e-00da049011cd@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=roman.habibov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH 1/2] sql: fix collation node duplication in AST' \
    /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