From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tarantool-patches-bounce@freelists.org>
Received: from localhost (localhost [127.0.0.1])
	by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D14302C72A
	for <tarantool-patches@freelists.org>; Sun, 12 May 2019 12:32:52 -0400 (EDT)
Received: from turing.freelists.org ([127.0.0.1])
	by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024)
	with ESMTP id S-kQ0lWGvto2 for <tarantool-patches@freelists.org>;
	Sun, 12 May 2019 12:32:52 -0400 (EDT)
Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(No client certificate requested)
	by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 8C6042C702
	for <tarantool-patches@freelists.org>; Sun, 12 May 2019 12:32:52 -0400 (EDT)
Subject: [tarantool-patches] Re: [PATCH 1/2] sql: fix collation node
 duplication in AST
References: <cover.1557312975.git.roman.habibov@tarantool.org>
 <c87fe7d113b4c3710c3eca299824caea5346ec5b.1557312975.git.roman.habibov@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <d15b9a9e-9e9a-8033-c89e-00da049011cd@tarantool.org>
Date: Sun, 12 May 2019 19:32:49 +0300
MIME-Version: 1.0
In-Reply-To: <c87fe7d113b4c3710c3eca299824caea5346ec5b.1557312975.git.roman.habibov@tarantool.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: tarantool-patches-bounce@freelists.org
Errors-to: tarantool-patches-bounce@freelists.org
Reply-To: tarantool-patches@freelists.org
List-Help: <mailto:ecartis@freelists.org?Subject=help>
List-Unsubscribe: <mailto:tarantool-patches-request@freelists.org?Subject=unsubscribe>
List-software: Ecartis version 1.0.0
List-Id: tarantool-patches <tarantool-patches.freelists.org>
List-Subscribe: <mailto:tarantool-patches-request@freelists.org?Subject=subscribe>
List-Owner: <mailto:>
List-post: <mailto:tarantool-patches@freelists.org>
List-Archive: <http://www.freelists.org/archives/tarantool-patches>
To: Roman Khabibov <roman.habibov@tarantool.org>, tarantool-patches@freelists.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.