Hello! Thank you for review! New patch at the end of the letter.
Sorry for quite late respond, I sincerely regret it.Rebased, fixed.Patch LGTM (except for several minor nits).diff --git a/src/box/sql.c b/src/box/sql.c
index b158c50..87f2088 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1217,21 +1217,21 @@ void tarantoolSqlite3LoadSchema(struct init_data *init)
sql_init_callback(init, TARANTOOL_SYS_SQL_STAT1_NAME,
BOX_SQL_STAT1_ID, 0,
"CREATE TABLE \""TARANTOOL_SYS_SQL_STAT1_NAME
- "\"(\"tbl\" text,"
- "\"idx\" text,"
+ "\"(\"space_id\" INT,"
+ "\"index_id\" INT,"
"\"stat\" not null,"
- "PRIMARY KEY(\"tbl\", \"idx\"))");
+ "PRIMARY KEY(\"space_id\", \"index_id\"))");
sql_init_callback(init, TARANTOOL_SYS_SQL_STAT4_NAME,
BOX_SQL_STAT4_ID, 0,
"CREATE TABLE \""TARANTOOL_SYS_SQL_STAT4_NAME
- "\"(\"tbl\" text,"
- "\"idx\" text,"
+ "\"(\"space_id\" INT,"
+ "\"index_id\" INT,"
"\"neq\" text,"
"\"nlt\" text,"
"\"ndlt\" text,"
"\"sample\","
- "PRIMARY KEY(\"tbl\", \"idx\", \"sample\"))");
+ "PRIMARY KEY(\"space_id\", \"index_id\", \"sample\"))”);Note: after you rebase on fresh 2.0, these fixes will disappear, sincewe don’t add system spaces to separate hash anymore.
Fixed.diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 76ae153..6938dcb 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -38,8 +38,8 @@
*
* The following system tables are or have been supported:
*
- * CREATE TABLE _sql_stat1(tbl, idx, stat);
- * CREATE TABLE _sql_stat4(tbl, idx, nEq, nLt, nDLt, sample);
+ * CREATE TABLE _sql_stat1(space_id, index_id, stat);
+ * CREATE TABLE _sql_stat4(space_id, index_id, nEq, nLt, nDLt, sample);
*
* For most applications, _sql_stat1 provides all the statistics required
* for the query planner to make good choices.
@@ -47,7 +47,7 @@
* Format of _sql_stat1:
*
* There is normally one row per index, with the index identified by the
- * name in the idx column. The tbl column is the name of the table to
+ * name in the index_id column. The space_id column is the name of the table toNit: * id in the index_id column. The space_id column is the id of the space to
Fixed.@@ -1410,27 +1405,14 @@ load_stat_from_space(struct sqlite3 *db, const char *sql_select_prepare,
goto finalize;
uint32_t current_idx_count = 0;
while (sqlite3_step(stmt) == SQLITE_ROW) {
- const char *space_name = (char *)sqlite3_column_text(stmt, 0);
- if (space_name == NULL)
- continue;
- const char *index_name = (char *)sqlite3_column_text(stmt, 1);
- if (index_name == NULL)
- continue;
- uint32_t sample_count = sqlite3_column_int(stmt, 2);
- uint32_t space_id = box_space_id_by_name(space_name,
- strlen(space_name));
- assert(space_id != BOX_ID_NIL);
+ uint32_t space_id = sqlite3_column_int(stmt, 0);
+ assert(space_id != BOX_ID_NIL && space_id != 0);Above you have mentioned that space_id now can’t be equal to BOX_ID_NIL:Actually now we cannot get BOX_ID_NIL, but we can get 0. Changed "if"@@ -1246,24 +1246,14 @@ analysis_loader(void *data, int argc, char **argv, char **unused)I would add assertion like:
struct analysis_index_info *info = (struct analysis_index_info *) data;
assert(info->stats != NULL);
struct index_stat *stat = &info->stats[info->index_count++];
-uint32_t space_id = box_space_id_by_name(argv[0], strlen(argv[0]));
+uint32_t space_id = atoll(argv[0]);
assert(space_id != 0);
Since atoll in case of fail returns 0 and BOX_ID_NIL != 0.
that placed next line after "atoll”.So, why do you check within assertion that condition? I mean there is no crime,but does it make sense? The same for other similar checks.
Removed space id/name from result of executed queries.--- a/test/sql/sql-statN-index-drop.test.lua
+++ b/test/sql/sql-statN-index-drop.test.lua
@@ -14,15 +14,15 @@ box.sql.execute("INSERT INTO t2 VALUES(1, 2);")
box.sql.execute("ANALYZE;")
-- Checking the data.
-box.sql.execute("SELECT * FROM \"_sql_stat4\";")
-box.sql.execute("SELECT * FROM \"_sql_stat1\";")
+box.sql.execute('SELECT "name","index_id","neq","nlt","ndlt","sample" FROM "_sql_stat4" join "_space" on "_sql_stat4"."space_id" = "_space"."id";')
+box.sql.execute('SELECT "name","index_id","stat" FROM "_sql_stat1" join "_space" on "_sql_stat1"."space_id" = "_space"."id";’)Nit: I don’t think that in this test (and the rest below) we need to fetchname of space instead of its id. Lets leave this test as it was before.
-- Dropping an index.
box.sql.execute("DROP INDEX i1 ON t1;")
-- Checking the DROP INDEX results.
-box.sql.execute("SELECT * FROM \"_sql_stat4\";")
-box.sql.execute("SELECT * FROM \"_sql_stat1\";")
+box.sql.execute('SELECT "name","index_id","neq","nlt","ndlt","sample" FROM "_sql_stat4" join "_space" on "_sql_stat4"."space_id" = "_space"."id";')
+box.sql.execute('SELECT "name","index_id","stat" FROM "_sql_stat1" join "_space" on "_sql_stat1"."space_id" = "_space"."id";')
--Cleaning up.
box.sql.execute("DROP TABLE t1;")
@@ -41,15 +41,15 @@ box.sql.execute("INSERT INTO t2 VALUES(1, 2);")
box.sql.execute("ANALYZE;")
-- Checking the data.
-box.sql.execute("SELECT * FROM \"_sql_stat4\";")
-box.sql.execute("SELECT * FROM \"_sql_stat1\";")
+box.sql.execute('SELECT "name","index_id","neq","nlt","ndlt","sample" FROM "_sql_stat4" join "_space" on "_sql_stat4"."space_id" = "_space"."id";')
+box.sql.execute('SELECT "name","index_id","stat" FROM "_sql_stat1" join "_space" on "_sql_stat1"."space_id" = "_space"."id";')
-- Dropping an index.
box.sql.execute("DROP INDEX i1 ON t2;")
-- Checking the DROP INDEX results.
-box.sql.execute("SELECT * FROM \"_sql_stat4\";")
-box.sql.execute("SELECT * FROM \"_sql_stat1\";")
+box.sql.execute('SELECT "name","index_id","neq","nlt","ndlt","sample" FROM "_sql_stat4" join "_space" on "_sql_stat4"."space_id" = "_space"."id";')
+box.sql.execute('SELECT "name","index_id","stat" FROM "_sql_stat1" join "_space" on "_sql_stat1"."space_id" = "_space"."id";')