[tarantool-patches] Re: [PATCH v8.5] sql: add index_def to struct Index

  • From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx, Ivan Koptelov <ivan.koptelov@xxxxxxxxxxxxx>, Nikita Pettik <korablev@xxxxxxxxxxxxx>
  • Date: Tue, 3 Jul 2018 15:13:06 +0300

Hello. Thanks for the fixes!

On 03/07/2018 12:46, Ivan Koptelov wrote:

Thank for the review. Previous mail contains patch with an error. Here it is 
fixed.
Hello. Thanks for the fixes! See my 6 comments below.

And I have pushed more fixes on the branch. Please,
look and squash.

Now every sqlite struct Index is created with tnt struct
index_def inside. This allows us to use tnt index_def
in work with sqlite indexes in the same manner as with
tnt index and is a step to remove sqlite Index with
tnt index.
Fields coll_array, coll_id_array, aiColumn, sort_order
and zName are removed from Index. All usages of this
fields changed to usage of corresponding index_def
fields.
index_is_unique(), sql_index_collation() and
index_column_count() are removed with calls of
index_def corresponding fields.

Closes: #3369

---
Branch:
https://github.com/tarantool/tarantool/tree/sb/gh-3369-use-index-def-in-select-and-where
Issue:https://github.com/tarantool/tarantool/issues/3369

  src/box/sql.c                        |  54 ++-
  src/box/sql/analyze.c                |  85 ++---
  src/box/sql/build.c                  | 713 +++++++++++++++++------------------
  src/box/sql/delete.c                 |  10 +-
  src/box/sql/expr.c                   |  61 +--
  src/box/sql/fkey.c                   | 132 +++----
  src/box/sql/insert.c                 | 145 ++++---
  src/box/sql/pragma.c                 |  30 +-
  src/box/sql/select.c                 |   2 +-
  src/box/sql/sqliteInt.h              | 111 ++----
  src/box/sql/update.c                 |  39 +-
  src/box/sql/vdbeaux.c                |   2 +-
  src/box/sql/vdbemem.c                |  21 +-
  src/box/sql/where.c                  | 180 ++++-----
  src/box/sql/wherecode.c              | 102 ++---
  test/sql-tap/colname.test.lua        |   4 +-
  test/sql/message-func-indexes.result |   8 +-
  17 files changed, 821 insertions(+), 878 deletions(-)

-
-/** Return true if given index is unique and not nullable. */
-bool
-index_is_unique_not_null(const Index *idx)

4. Same as on the previous review. Still is used in a pair of places.
Are you sure? I searched through the whole project and didn't find it.
There is only a variable with the same name in one place.

Sorry, my fault. It is just a variable.

diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index 85143ed20..7ca02095f 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -3058,6 +3064,8 @@ whereLoopAddBtree(WhereLoopBuilder * pBuilder,    /* 
WHERE clause information */
                if (pSrc->pIBIndex)
                        break;
        }
+       if (&sPk == pProbe && sPk.def != NULL)
+               index_def_delete(sPk.def);

It will not be deleted since pProbe is changed in the cycle above.
I have checked it by putting assert(false) under this 'if'.
And I have already fixed it on the branch.

        return rc;
  }

I have pushed my last fixes on the branch. Please, look, squash and then
LGTM. Nikita, please, review after the squash.

Other related posts: