* [Tarantool-patches] [PATCH v2 1/6] sql: refactor resulting set metadata
[not found] ` <cover.1576071711.git.korablev@tarantool.org>
@ 2019-12-11 13:44 ` Nikita Pettik
2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 2/6] sql: fix possible null dereference in sql_expr_coll() Nikita Pettik
` (4 subsequent siblings)
5 siblings, 0 replies; 25+ messages in thread
From: Nikita Pettik @ 2019-12-11 13:44 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
Move names and types of resulting set to a separate structure. Simplify
their storage by introducing separate members for name and type
(previously names and types were stored in one char * array). It will
allow us to add new metadata properties with ease.
Needed for #4407
---
src/box/sql/delete.c | 6 ++--
src/box/sql/insert.c | 5 ++--
src/box/sql/legacy.c | 2 +-
src/box/sql/pragma.c | 14 ++++-----
src/box/sql/prepare.c | 9 +++---
src/box/sql/select.c | 81 ++++++++++++++++++++++++++-------------------------
src/box/sql/update.c | 6 ++--
src/box/sql/vdbe.h | 28 ++++++++++--------
src/box/sql/vdbeInt.h | 8 ++++-
src/box/sql/vdbeapi.c | 81 +++++++++------------------------------------------
src/box/sql/vdbeaux.c | 81 +++++++++++++++++++++++++++------------------------
11 files changed, 137 insertions(+), 184 deletions(-)
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 91c2157ac..169814a2e 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -418,10 +418,8 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
parse->triggered_space != NULL) {
sqlVdbeAddOp2(v, OP_ResultRow, reg_count, 1);
sqlVdbeSetNumCols(v, 1);
- sqlVdbeSetColName(v, 0, COLNAME_NAME, "rows deleted",
- SQL_STATIC);
- sqlVdbeSetColName(v, 0, COLNAME_DECLTYPE, "integer",
- SQL_STATIC);
+ vdbe_metadata_set_col_name(v, 0, "rows deleted");
+ vdbe_metadata_set_col_type(v, 0, "integer");
}
delete_from_cleanup:
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 70504c800..f1290e01c 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -785,9 +785,8 @@ sqlInsert(Parse * pParse, /* Parser context */
column_name = "rows replaced";
else
column_name = "rows inserted";
- sqlVdbeSetColName(v, 0, COLNAME_NAME, column_name, SQL_STATIC);
- sqlVdbeSetColName(v, 0, COLNAME_DECLTYPE, "integer",
- SQL_STATIC);
+ vdbe_metadata_set_col_name(v, 0, column_name);
+ vdbe_metadata_set_col_type(v, 0, "integer");
}
insert_cleanup:
diff --git a/src/box/sql/legacy.c b/src/box/sql/legacy.c
index 0b1370f4a..8eec096bf 100644
--- a/src/box/sql/legacy.c
+++ b/src/box/sql/legacy.c
@@ -103,7 +103,7 @@ sql_exec(sql * db, /* The database on which the SQL executes */
(char *)
sql_column_name(pStmt,
i);
- /* sqlVdbeSetColName() installs column names as UTF8
+ /* vdbe_metadata_set_col_name() installs column names as UTF8
* strings so there is no way for sql_column_name() to fail.
*/
assert(azCols[i] != 0);
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 92bcf4e68..00ecde0a9 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -120,10 +120,8 @@ vdbe_set_pragma_result_columns(struct Vdbe *v, const struct PragmaName *pragma)
assert(n > 0);
sqlVdbeSetNumCols(v, n);
for (int i = 0, j = pragma->iPragCName; i < n; ++i) {
- sqlVdbeSetColName(v, i, COLNAME_NAME, pragCName[j++],
- SQL_STATIC);
- sqlVdbeSetColName(v, i, COLNAME_DECLTYPE, pragCName[j++],
- SQL_STATIC);
+ vdbe_metadata_set_col_name(v, i, pragCName[j++]);
+ vdbe_metadata_set_col_type(v, i, pragCName[j++]);
}
}
@@ -168,10 +166,10 @@ vdbe_emit_pragma_status(struct Parse *parse)
struct session *user_session = current_session();
sqlVdbeSetNumCols(v, 2);
- sqlVdbeSetColName(v, 0, COLNAME_NAME, "pragma_name", SQL_STATIC);
- sqlVdbeSetColName(v, 0, COLNAME_DECLTYPE, "text", SQL_STATIC);
- sqlVdbeSetColName(v, 1, COLNAME_NAME, "pragma_value", SQL_STATIC);
- sqlVdbeSetColName(v, 1, COLNAME_DECLTYPE, "integer", SQL_STATIC);
+ vdbe_metadata_set_col_name(v, 0, "pragma_name");
+ vdbe_metadata_set_col_type(v, 0, "text");
+ vdbe_metadata_set_col_name(v, 1, "pragma_value");
+ vdbe_metadata_set_col_type(v, 1, "integer");
parse->nMem = 2;
for (int i = 0; i < ArraySize(aPragmaName); ++i) {
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 0ecc676e2..39e897ba3 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -146,11 +146,10 @@ sqlPrepare(sql * db, /* Database handle. */
sqlVdbeSetNumCols(sParse.pVdbe, name_count);
for (int i = 0; i < name_count; i++) {
int name_index = 2 * i + name_first;
- sqlVdbeSetColName(sParse.pVdbe, i, COLNAME_NAME,
- azColName[name_index], SQL_STATIC);
- sqlVdbeSetColName(sParse.pVdbe, i, COLNAME_DECLTYPE,
- azColName[name_index + 1],
- SQL_STATIC);
+ vdbe_metadata_set_col_name(sParse.pVdbe, i,
+ azColName[name_index]);
+ vdbe_metadata_set_col_type(sParse.pVdbe, i,
+ azColName[name_index + 1]);
}
}
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 8f93edd16..e4768121e 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1747,15 +1747,18 @@ generateSortTail(Parse * pParse, /* Parsing context */
sqlVdbeResolveLabel(v, addrBreak);
}
-/*
+/**
* Generate code that will tell the VDBE the names of columns
- * in the result set. This information is used to provide the
- * azCol[] values in the callback.
+ * in the result set. This information is used to provide the
+ * metadata during/after statement execution.
+ *
+ * @param pParse Parsing context.
+ * @param pTabList List of tables.
+ * @param pEList Expressions defining the result set.
*/
static void
-generateColumnNames(Parse * pParse, /* Parser context */
- SrcList * pTabList, /* List of tables */
- ExprList * pEList) /* Expressions defining the result set */
+generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
+ struct ExprList *pEList)
{
Vdbe *v = pParse->pVdbe;
int i, j;
@@ -1789,13 +1792,9 @@ generateColumnNames(Parse * pParse, /* Parser context */
continue;
if (p->op == TK_VARIABLE)
var_pos[var_count++] = i;
- sqlVdbeSetColName(v, i, COLNAME_DECLTYPE,
- field_type_strs[sql_expr_type(p)], SQL_TRANSIENT);
- if (pEList->a[i].zName) {
- char *zName = pEList->a[i].zName;
- sqlVdbeSetColName(v, i, COLNAME_NAME, zName,
- SQL_TRANSIENT);
- } else if (p->op == TK_COLUMN || p->op == TK_AGG_COLUMN) {
+ vdbe_metadata_set_col_type(v, i,
+ field_type_strs[sql_expr_type(p)]);
+ if (p->op == TK_COLUMN || p->op == TK_AGG_COLUMN) {
char *zCol;
int iCol = p->iColumn;
for (j = 0; ALWAYS(j < pTabList->nSrc); j++) {
@@ -1806,28 +1805,30 @@ generateColumnNames(Parse * pParse, /* Parser context */
struct space_def *space_def = pTabList->a[j].space->def;
assert(iCol >= 0 && iCol < (int)space_def->field_count);
zCol = space_def->fields[iCol].name;
- if (!shortNames && !fullNames) {
- sqlVdbeSetColName(v, i, COLNAME_NAME,
- sqlDbStrDup(db,
- pEList->a[i].zSpan),
- SQL_DYNAMIC);
- } else if (fullNames) {
- char *zName = 0;
- zName = sqlMPrintf(db, "%s.%s",
- space_def->name, zCol);
- sqlVdbeSetColName(v, i, COLNAME_NAME, zName,
- SQL_DYNAMIC);
+ const char *name = NULL;
+ if (pEList->a[i].zName != NULL) {
+ name = pEList->a[i].zName;
} else {
- sqlVdbeSetColName(v, i, COLNAME_NAME, zCol,
- SQL_TRANSIENT);
+ if (!shortNames && !fullNames) {
+ name = pEList->a[i].zSpan;
+ } else if (fullNames) {
+ name = tt_sprintf("%s.%s",
+ space_def->name,
+ zCol);
+ } else {
+ name = zCol;
+ }
}
+ vdbe_metadata_set_col_name(v, i, name);
} else {
- const char *z = pEList->a[i].zSpan;
- z = z == 0 ? sqlMPrintf(db, "column%d",
- i + 1) : sqlDbStrDup(db,
- z);
- sqlVdbeSetColName(v, i, COLNAME_NAME, z,
- SQL_DYNAMIC);
+ const char *z = NULL;
+ if (pEList->a[i].zName != NULL)
+ z = pEList->a[i].zName;
+ else if (pEList->a[i].zSpan != NULL)
+ z = pEList->a[i].zSpan;
+ else
+ z = tt_sprintf("column%d", i + 1);
+ vdbe_metadata_set_col_name(v, i, z);
}
}
if (var_count == 0)
@@ -2828,9 +2829,9 @@ multiSelect(Parse * pParse, /* Parsing context */
Select *pFirst = p;
while (pFirst->pPrior)
pFirst = pFirst->pPrior;
- generateColumnNames(pParse,
- pFirst->pSrc,
- pFirst->pEList);
+ generate_column_metadata(pParse,
+ pFirst->pSrc,
+ pFirst->pEList);
}
iBreak = sqlVdbeMakeLabel(v);
iCont = sqlVdbeMakeLabel(v);
@@ -2927,9 +2928,9 @@ multiSelect(Parse * pParse, /* Parsing context */
Select *pFirst = p;
while (pFirst->pPrior)
pFirst = pFirst->pPrior;
- generateColumnNames(pParse,
- pFirst->pSrc,
- pFirst->pEList);
+ generate_column_metadata(pParse,
+ pFirst->pSrc,
+ pFirst->pEList);
}
iBreak = sqlVdbeMakeLabel(v);
iCont = sqlVdbeMakeLabel(v);
@@ -3575,7 +3576,7 @@ multiSelectOrderBy(Parse * pParse, /* Parsing context */
Select *pFirst = pPrior;
while (pFirst->pPrior)
pFirst = pFirst->pPrior;
- generateColumnNames(pParse, pFirst->pSrc, pFirst->pEList);
+ generate_column_metadata(pParse, pFirst->pSrc, pFirst->pEList);
}
/* Reassembly the compound query so that it will be freed correctly
@@ -6433,7 +6434,7 @@ sqlSelect(Parse * pParse, /* The parser context */
/* Identify column names if results of the SELECT are to be output.
*/
if (rc == 0 && pDest->eDest == SRT_Output) {
- generateColumnNames(pParse, pTabList, pEList);
+ generate_column_metadata(pParse, pTabList, pEList);
}
sqlDbFree(db, sAggInfo.aCol);
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 6d69b7252..c08777a2d 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -498,10 +498,8 @@ sqlUpdate(Parse * pParse, /* The parser context */
pParse->triggered_space == NULL) {
sqlVdbeAddOp2(v, OP_ResultRow, regRowCount, 1);
sqlVdbeSetNumCols(v, 1);
- sqlVdbeSetColName(v, 0, COLNAME_NAME, "rows updated",
- SQL_STATIC);
- sqlVdbeSetColName(v, 0, COLNAME_DECLTYPE, "integer",
- SQL_STATIC);
+ vdbe_metadata_set_col_name(v, 0, "rows updated");
+ vdbe_metadata_set_col_type(v, 0, "integer");
}
update_cleanup:
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 582d48a1f..ddced20e1 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -148,17 +148,6 @@ struct SubProgram {
#define P5_ConstraintUnique 2
#define P5_ConstraintFK 4
-/*
- * The Vdbe.aColName array contains 5n Mem structures, where n is the
- * number of columns of data returned by the statement.
- */
-#define COLNAME_NAME 0
-#define COLNAME_DECLTYPE 1
-#define COLNAME_DATABASE 2
-#define COLNAME_TABLE 3
-#define COLNAME_COLUMN 4
-#define COLNAME_N 2 /* Store the name and decltype */
-
/*
* The following macro converts a relative address in the p2 field
* of a VdbeOp structure into a negative number.
@@ -238,6 +227,10 @@ sql_vdbe_set_p4_key_def(struct Parse *parse, struct key_def *key_def);
VdbeOp *sqlVdbeGetOp(Vdbe *, int);
int sqlVdbeMakeLabel(Vdbe *);
void sqlVdbeRunOnlyOnce(Vdbe *);
+
+void
+vdbe_metadata_delete(struct Vdbe *v);
+
void sqlVdbeDelete(Vdbe *);
void sqlVdbeClearObject(sql *, Vdbe *);
void sqlVdbeMakeReady(Vdbe *, Parse *);
@@ -248,7 +241,18 @@ void sqlVdbeResetStepResult(Vdbe *);
void sqlVdbeRewind(Vdbe *);
int sqlVdbeReset(Vdbe *);
void sqlVdbeSetNumCols(Vdbe *, int);
-int sqlVdbeSetColName(Vdbe *, int, int, const char *, void (*)(void *));
+
+/**
+ * Set the name of the idx'th column to be returned by the SQL
+ * statement. @name must be a pointer to a nul terminated string.
+ * This call must be made after a call to sqlVdbeSetNumCols().
+ */
+int
+vdbe_metadata_set_col_name(struct Vdbe *v, int col_idx, const char *name);
+
+int
+vdbe_metadata_set_col_type(struct Vdbe *v, int col_idx, const char *type);
+
void sqlVdbeCountChanges(Vdbe *);
sql *sqlVdbeDb(Vdbe *);
void sqlVdbeSetSql(Vdbe *, const char *z, int n, int);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 0f32b4cd6..64250bee2 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -346,6 +346,11 @@ struct ScanStatus {
char *zName; /* Name of table or index */
};
+struct sql_column_metadata {
+ char *name;
+ char *type;
+};
+
/*
* An instance of the virtual machine. This structure contains the complete
* state of the virtual machine.
@@ -394,7 +399,8 @@ struct Vdbe {
Op *aOp; /* Space to hold the virtual machine's program */
Mem *aMem; /* The memory locations */
Mem **apArg; /* Arguments to currently executing user function */
- Mem *aColName; /* Column names to return */
+ /** SQL metadata for DML/DQL queries. */
+ struct sql_column_metadata *metadata;
Mem *pResultSet; /* Pointer to an array of results */
VdbeCursor **apCsr; /* One element of this array for each open cursor */
Mem *aVar; /* Values for the OP_Variable opcode. */
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 685212d91..48db0bf43 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -725,77 +725,24 @@ sql_column_subtype(struct sql_stmt *stmt, int i)
return sql_value_subtype(columnMem(stmt, i));
}
-/*
- * Convert the N-th element of pStmt->pColName[] into a string using
- * xFunc() then return that string. If N is out of range, return 0.
- *
- * There are up to 5 names for each column. useType determines which
- * name is returned. Here are the names:
- *
- * 0 The column name as it should be displayed for output
- * 1 The datatype name for the column
- * 2 The name of the database that the column derives from
- * 3 The name of the table that the column derives from
- * 4 The name of the table column that the result column derives from
- *
- * If the result is not a simple column reference (if it is an expression
- * or a constant) then useTypes 2, 3, and 4 return NULL.
- */
-static const void *
-columnName(sql_stmt * pStmt,
- int N, const void *(*xFunc) (Mem *), int useType)
-{
- const void *ret;
- Vdbe *p;
- int n;
- sql *db;
- ret = 0;
- p = (Vdbe *) pStmt;
- db = p->db;
- assert(db != 0);
- n = sql_column_count(pStmt);
- if (N < n && N >= 0) {
- N += useType * n;
- assert(db->mallocFailed == 0);
- ret = xFunc(&p->aColName[N]);
- /* A malloc may have failed inside of the xFunc() call. If this
- * is the case, clear the mallocFailed flag and return NULL.
- */
- if (db->mallocFailed) {
- sqlOomClear(db);
- ret = 0;
- }
- }
- return ret;
-}
-
/*
* Return the name of the Nth column of the result set returned by SQL
* statement pStmt.
*/
const char *
-sql_column_name(sql_stmt * pStmt, int N)
-{
- return columnName(pStmt, N, (const void *(*)(Mem *))sql_value_text,
- COLNAME_NAME);
-}
-
-const char *
-sql_column_datatype(sql_stmt *pStmt, int N)
+sql_column_name(sql_stmt *stmt, int n)
{
- return columnName(pStmt, N, (const void *(*)(Mem *))sql_value_text,
- COLNAME_DECLTYPE);
+ struct Vdbe *p = (struct Vdbe *) stmt;
+ assert(n < sql_column_count(stmt) && n >= 0);
+ return p->metadata[n].name;
}
-/*
- * Return the column declaration type (if applicable) of the 'i'th column
- * of the result set of SQL statement pStmt.
- */
const char *
-sql_column_decltype(sql_stmt * pStmt, int N)
+sql_column_datatype(sql_stmt *stmt, int n)
{
- return columnName(pStmt, N, (const void *(*)(Mem *))sql_value_text,
- COLNAME_DECLTYPE);
+ struct Vdbe *p = (struct Vdbe *) stmt;
+ assert(n < sql_column_count(stmt) && n >= 0);
+ return p->metadata[n].type;
}
/******************************* sql_bind_ **************************
@@ -853,17 +800,15 @@ sql_bind_type(struct Vdbe *v, uint32_t position, const char *type)
if (v->res_var_count < position)
return 0;
int rc = 0;
- if (sqlVdbeSetColName(v, v->var_pos[position - 1], COLNAME_DECLTYPE,
- type, SQL_TRANSIENT) != 0)
+ if (vdbe_metadata_set_col_type(v, v->var_pos[position - 1], type) != 0)
rc = -1;
- const char *bind_name = v->aColName[position - 1].z;
+ const char *bind_name = v->metadata[position - 1].name;
if (strcmp(bind_name, "?") == 0)
return rc;
for (uint32_t i = position; i < v->res_var_count; ++i) {
- if (strcmp(bind_name, v->aColName[i].z) == 0) {
- if (sqlVdbeSetColName(v, v->var_pos[i],
- COLNAME_DECLTYPE, type,
- SQL_TRANSIENT) != 0)
+ if (strcmp(bind_name, v->metadata[i].name) == 0) {
+ if (vdbe_metadata_set_col_type(v, v->var_pos[i],
+ type) != 0)
return -1;
}
}
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index a1d658648..f2cf386bb 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1827,6 +1827,18 @@ Cleanup(Vdbe * p)
p->pResultSet = 0;
}
+void
+vdbe_metadata_delete(struct Vdbe *v)
+{
+ if (v->metadata != NULL) {
+ for (int i = 0; i < v->nResColumn; ++i) {
+ free(v->metadata[i].name);
+ free(v->metadata[i].type);
+ }
+ free(v->metadata);
+ }
+}
+
/*
* Set the number of result columns that will be returned by this SQL
* statement. This is now set at compile time, rather than during
@@ -1836,50 +1848,44 @@ Cleanup(Vdbe * p)
void
sqlVdbeSetNumCols(Vdbe * p, int nResColumn)
{
- int n;
- sql *db = p->db;
-
- releaseMemArray(p->aColName, p->nResColumn * COLNAME_N);
- sqlDbFree(db, p->aColName);
- n = nResColumn * COLNAME_N;
+ vdbe_metadata_delete(p);
p->nResColumn = (u16) nResColumn;
- p->aColName = (Mem *) sqlDbMallocRawNN(db, sizeof(Mem) * n);
- if (p->aColName == 0)
+ p->metadata = (struct sql_column_metadata *)
+ calloc(nResColumn, sizeof(struct sql_column_metadata));
+ if (p->metadata == NULL) {
+ diag_set(OutOfMemory,
+ nResColumn * sizeof(struct sql_column_metadata),
+ "calloc", "metadata");
return;
- initMemArray(p->aColName, n, p->db, MEM_Null);
+ }
}
-/*
- * Set the name of the idx'th column to be returned by the SQL statement.
- * zName must be a pointer to a nul terminated string.
- *
- * This call must be made after a call to sqlVdbeSetNumCols().
- *
- * The final parameter, xDel, must be one of SQL_DYNAMIC, SQL_STATIC
- * or SQL_TRANSIENT. If it is SQL_DYNAMIC, then the buffer pointed
- * to by zName will be freed by sqlDbFree() when the vdbe is destroyed.
- */
int
-sqlVdbeSetColName(Vdbe * p, /* Vdbe being configured */
- int idx, /* Index of column zName applies to */
- int var, /* One of the COLNAME_* constants */
- const char *zName, /* Pointer to buffer containing name */
- void (*xDel) (void *)) /* Memory management strategy for zName */
-{
- int rc;
- Mem *pColName;
+vdbe_metadata_set_col_name(struct Vdbe *p, int idx, const char *name)
+{
assert(idx < p->nResColumn);
- assert(var < COLNAME_N);
- if (p->db->mallocFailed) {
- assert(!zName || xDel != SQL_DYNAMIC);
+ if (p->metadata[idx].name != NULL)
+ free(p->metadata[idx].name);
+ p->metadata[idx].name = strdup(name);
+ if (p->metadata[idx].name == NULL) {
+ diag_set(OutOfMemory, strlen(name) + 1, "strdup", "name");
return -1;
}
- assert(p->aColName != 0);
- assert(var == COLNAME_NAME || var == COLNAME_DECLTYPE);
- pColName = &(p->aColName[idx + var * p->nResColumn]);
- rc = sqlVdbeMemSetStr(pColName, zName, -1, 1, xDel);
- assert(rc != 0 || !zName || (pColName->flags & MEM_Term) != 0);
- return rc;
+ return 0;
+}
+
+int
+vdbe_metadata_set_col_type(struct Vdbe *p, int idx, const char *type)
+{
+ assert(idx < p->nResColumn);
+ if (p->metadata[idx].type != NULL)
+ free(p->metadata[idx].type);
+ p->metadata[idx].type = strdup(type);
+ if (p->metadata[idx].type == NULL) {
+ diag_set(OutOfMemory, strlen(type) + 1, "strdup", "type");
+ return -1;
+ }
+ return 0;
}
/*
@@ -2230,7 +2236,7 @@ sqlVdbeClearObject(sql * db, Vdbe * p)
{
SubProgram *pSub, *pNext;
assert(p->db == 0 || p->db == db);
- releaseMemArray(p->aColName, p->nResColumn * COLNAME_N);
+ vdbe_metadata_delete(p);
for (pSub = p->pProgram; pSub; pSub = pNext) {
pNext = pSub->pNext;
vdbeFreeOpArray(db, pSub->aOp, pSub->nOp);
@@ -2242,7 +2248,6 @@ sqlVdbeClearObject(sql * db, Vdbe * p)
sqlDbFree(db, p->pFree);
}
vdbeFreeOpArray(db, p->aOp, p->nOp);
- sqlDbFree(db, p->aColName);
sqlDbFree(db, p->zSql);
}
--
2.15.1
^ permalink raw reply [flat|nested] 25+ messages in thread* [Tarantool-patches] [PATCH v2 2/6] sql: fix possible null dereference in sql_expr_coll()
[not found] ` <cover.1576071711.git.korablev@tarantool.org>
2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 1/6] sql: refactor resulting set metadata Nikita Pettik
@ 2019-12-11 13:44 ` Nikita Pettik
2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 3/6] sql: extend result set with collation Nikita Pettik
` (3 subsequent siblings)
5 siblings, 0 replies; 25+ messages in thread
From: Nikita Pettik @ 2019-12-11 13:44 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
Some built-in functions can accept different number of arguments.
Check of argument count for such functions takes place right before
its execution. So it is possible that expression-list representing
arguments for built-in function is NULL. On the other hand, in
sql_expr_coll() (which returns collation of expression) it is assumed
that if function features SQL_FUNC_DERIVEDCOLL flag (it implies that
resulting collation depends on collation of arguments) then it has at
least one argument. The last assumption is wrong considering for example
SUBSTR() function: it may have 1 or 2 arguments, so check of argument
count doesn't occur during compilation. Hence, if it is required to
calculate collation for SUBSTR() function and there's no arguments,
Tarantool crashes due to null-dereference.
This patch fixes this bug with one additional check in sql_expr_coll().
---
src/box/sql/expr.c | 3 ++-
test/sql/collation.result | 9 +++++++++
test/sql/collation.test.lua | 5 +++++
3 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 648b7170e..0bdcfe576 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -332,7 +332,8 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
sql_func_by_signature(p->u.zToken, arg_count);
if (func == NULL)
break;
- if (sql_func_flag_is_set(func, SQL_FUNC_DERIVEDCOLL)) {
+ if (sql_func_flag_is_set(func, SQL_FUNC_DERIVEDCOLL) &&
+ arg_count > 0) {
/*
* Now we use quite straightforward
* approach assuming that resulting
diff --git a/test/sql/collation.result b/test/sql/collation.result
index 11962ef47..fbc7ce9aa 100644
--- a/test/sql/collation.result
+++ b/test/sql/collation.result
@@ -292,6 +292,15 @@ box.execute("SELECT * FROM t WHERE a COLLATE \"binary\" = c COLLATE \"unicode\";
- null
- Illegal mix of collations
...
+-- Make sure that using function featuring variable arguemnts
+-- length and resulting collation which depends on arguments
+-- is processed correctly.
+--
+box.execute("SELECT * FROM t WHERE a COLLATE \"binary\" = substr();")
+---
+- null
+- 'Wrong number of arguments is passed to SUBSTR(): expected 1 or 2, got 0'
+...
-- Compound queries perform implicit comparisons between values.
-- Hence, rules for collations compatibilities are the same.
--
diff --git a/test/sql/collation.test.lua b/test/sql/collation.test.lua
index 1be28b3ff..a013253cd 100644
--- a/test/sql/collation.test.lua
+++ b/test/sql/collation.test.lua
@@ -80,6 +80,11 @@ box.execute("SELECT * FROM t WHERE b = c;")
box.execute("SELECT * FROM t WHERE b COLLATE \"binary\" = c;")
box.execute("SELECT * FROM t WHERE a = c;")
box.execute("SELECT * FROM t WHERE a COLLATE \"binary\" = c COLLATE \"unicode\";")
+-- Make sure that using function featuring variable arguemnts
+-- length and resulting collation which depends on arguments
+-- is processed correctly.
+--
+box.execute("SELECT * FROM t WHERE a COLLATE \"binary\" = substr();")
-- Compound queries perform implicit comparisons between values.
-- Hence, rules for collations compatibilities are the same.
--
2.15.1
^ permalink raw reply [flat|nested] 25+ messages in thread* [Tarantool-patches] [PATCH v2 3/6] sql: extend result set with collation
[not found] ` <cover.1576071711.git.korablev@tarantool.org>
2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 1/6] sql: refactor resulting set metadata Nikita Pettik
2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 2/6] sql: fix possible null dereference in sql_expr_coll() Nikita Pettik
@ 2019-12-11 13:44 ` Nikita Pettik
2019-12-18 0:29 ` Vladislav Shpilevoy
2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 4/6] sql: extend result set with nullability Nikita Pettik
` (2 subsequent siblings)
5 siblings, 1 reply; 25+ messages in thread
From: Nikita Pettik @ 2019-12-11 13:44 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
If resulting set column is of STRING type and features collation (no
matter explicit or implicit) different from "none", then metadata will
contain its name.
This patch also introduces new pragma: full_metadata. By default it is
not set. If it is turned on, then optional metadata (like collation) is
pushed to Lua stack. Note that via IProto protocol always full metadata
is send, but its decoding depends on session SQL settings.
Part of #4407
---
src/box/execute.c | 32 +++++++++---
src/box/iproto_constants.h | 1 +
src/box/lua/execute.c | 8 ++-
src/box/lua/net_box.c | 23 +++++++--
src/box/sql/pragma.h | 8 +++
src/box/sql/select.c | 19 +++++++
src/box/sql/sqlInt.h | 7 +++
src/box/sql/vdbe.h | 4 ++
src/box/sql/vdbeInt.h | 1 +
src/box/sql/vdbeapi.c | 9 ++++
src/box/sql/vdbeaux.c | 16 ++++++
test/sql/engine.cfg | 4 ++
test/sql/full_metadata.result | 106 ++++++++++++++++++++++++++++++++++++++++
test/sql/full_metadata.test.lua | 43 ++++++++++++++++
test/sql/sql-debug.result | 1 +
15 files changed, 272 insertions(+), 10 deletions(-)
create mode 100644 test/sql/full_metadata.result
create mode 100644 test/sql/full_metadata.test.lua
diff --git a/src/box/execute.c b/src/box/execute.c
index e8b012e5b..72300235a 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -268,6 +268,24 @@ error:
return -1;
}
+static size_t
+metadata_map_sizeof(const char *name, const char *type, const char *coll)
+{
+ uint32_t members_count = 2;
+ size_t map_size = 0;
+ if (coll != NULL) {
+ members_count++;
+ map_size += mp_sizeof_uint(IPROTO_FIELD_COLL);
+ map_size += mp_sizeof_str(strlen(coll));
+ }
+ map_size += mp_sizeof_uint(IPROTO_FIELD_NAME);
+ map_size += mp_sizeof_uint(IPROTO_FIELD_TYPE);
+ map_size += mp_sizeof_str(strlen(name));
+ map_size += mp_sizeof_str(strlen(type));
+ map_size += mp_sizeof_map(members_count);
+ return map_size;
+}
+
/**
* Serialize a description of the prepared statement.
* @param stmt Prepared statement.
@@ -291,9 +309,7 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
pos = mp_encode_uint(pos, IPROTO_METADATA);
pos = mp_encode_array(pos, column_count);
for (int i = 0; i < column_count; ++i) {
- size_t size = mp_sizeof_map(2) +
- mp_sizeof_uint(IPROTO_FIELD_NAME) +
- mp_sizeof_uint(IPROTO_FIELD_TYPE);
+ const char *coll = sql_column_coll(stmt, i);
const char *name = sql_column_name(stmt, i);
const char *type = sql_column_datatype(stmt, i);
/*
@@ -303,18 +319,22 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
*/
assert(name != NULL);
assert(type != NULL);
- size += mp_sizeof_str(strlen(name));
- size += mp_sizeof_str(strlen(type));
+ size = metadata_map_sizeof(name, type, coll);
char *pos = (char *) obuf_alloc(out, size);
if (pos == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "pos");
return -1;
}
- pos = mp_encode_map(pos, 2);
+ uint32_t map_sz = 2 + (coll != NULL);
+ pos = mp_encode_map(pos, map_sz);
pos = mp_encode_uint(pos, IPROTO_FIELD_NAME);
pos = mp_encode_str(pos, name, strlen(name));
pos = mp_encode_uint(pos, IPROTO_FIELD_TYPE);
pos = mp_encode_str(pos, type, strlen(type));
+ if (coll != NULL) {
+ pos = mp_encode_uint(pos, IPROTO_FIELD_COLL);
+ pos = mp_encode_str(pos, coll, strlen(coll));
+ }
}
return 0;
}
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index 5e8a7d483..fa9c029a2 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -131,6 +131,7 @@ enum iproto_key {
enum iproto_metadata_key {
IPROTO_FIELD_NAME = 0,
IPROTO_FIELD_TYPE = 1,
+ IPROTO_FIELD_COLL = 2,
};
enum iproto_ballot_key {
diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index ffa3d4d2e..55b8a8fef 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -20,9 +20,11 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L,
assert(column_count > 0);
lua_createtable(L, column_count, 0);
for (int i = 0; i < column_count; ++i) {
- lua_createtable(L, 0, 2);
+ const char *coll = sql_column_coll(stmt, i);
const char *name = sql_column_name(stmt, i);
const char *type = sql_column_datatype(stmt, i);
+ size_t table_sz = 2 + (coll != NULL);
+ lua_createtable(L, 0, table_sz);
/*
* Can not fail, since all column names are
* preallocated during prepare phase and the
@@ -34,6 +36,10 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L,
lua_setfield(L, -2, "name");
lua_pushstring(L, type);
lua_setfield(L, -2, "type");
+ if (coll != NULL) {
+ lua_pushstring(L, coll);
+ lua_setfield(L, -2, "collation");
+ }
lua_rawseti(L, -2, i + 1);
}
}
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 001af95dc..025143307 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -638,6 +638,23 @@ netbox_decode_select(struct lua_State *L)
return 2;
}
+/** Decode optional (i.e. may be present in response) metadata fields. */
+static void
+decode_metadata_optional(struct lua_State *L, const char **data,
+ uint32_t map_size)
+{
+ /* 2 is default metadata map size (field name + field size). */
+ while (map_size-- > 2) {
+ uint32_t key = mp_decode_uint(data);
+ uint32_t len;
+ if (key == IPROTO_FIELD_COLL) {
+ const char *coll = mp_decode_str(data, &len);
+ lua_pushlstring(L, coll, len);
+ lua_setfield(L, -2, "collation");
+ }
+ }
+}
+
/**
* Decode IPROTO_METADATA into array of maps.
* @param L Lua stack to push result on.
@@ -650,12 +667,11 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
lua_createtable(L, count, 0);
for (uint32_t i = 0; i < count; ++i) {
uint32_t map_size = mp_decode_map(data);
- assert(map_size == 2);
- (void) map_size;
+ assert(map_size == 2 || map_size == 3);
uint32_t key = mp_decode_uint(data);
assert(key == IPROTO_FIELD_NAME);
(void) key;
- lua_createtable(L, 0, 1);
+ lua_createtable(L, 0, map_size);
uint32_t len;
const char *str = mp_decode_str(data, &len);
lua_pushlstring(L, str, len);
@@ -665,6 +681,7 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
const char *type = mp_decode_str(data, &len);
lua_pushlstring(L, type, len);
lua_setfield(L, -2, "type");
+ decode_metadata_optional(L, data, map_size);
lua_rawseti(L, -2, i + 1);
}
}
diff --git a/src/box/sql/pragma.h b/src/box/sql/pragma.h
index 30a1fc41a..aa9377a13 100644
--- a/src/box/sql/pragma.h
+++ b/src/box/sql/pragma.h
@@ -144,6 +144,9 @@ static const char *const pragCName[] = {
/* Used by: where_trace */
/* 90 */ "where_trace",
/* 91 */ "integer",
+ /* Used by: full_metadata */
+ /* 92 */ "full_metadata",
+ /* 93 */ "integer",
};
/* Definitions of all built-in pragmas */
@@ -186,6 +189,11 @@ static const PragmaName aPragmaName[] = {
/* ePragFlg: */ PragFlg_Result0 | PragFlg_NoColumns1,
/* ColNames: */ 64, 1,
/* iArg: */ SQL_FullColNames},
+ { /* zName: */ "full_metadata",
+ /* ePragTyp: */ PragTyp_FLAG,
+ /* ePragFlg: */ PragFlg_Result0 | PragFlg_NoColumns1,
+ /* ColNames: */ 92, 1,
+ /* iArg: */ SQL_FullMetadata},
{ /* zName: */ "index_info",
/* ePragTyp: */ PragTyp_INDEX_INFO,
/* ePragFlg: */
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index e4768121e..506c1a7c7 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1783,6 +1783,7 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
pParse->colNamesSet = 1;
fullNames = (pParse->sql_flags & SQL_FullColNames) != 0;
shortNames = (pParse->sql_flags & SQL_ShortColNames) != 0;
+ bool is_full_meta = (pParse->sql_flags & SQL_FullMetadata) != 0;
sqlVdbeSetNumCols(v, pEList->nExpr);
uint32_t var_count = 0;
for (i = 0; i < pEList->nExpr; i++) {
@@ -1794,6 +1795,24 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
var_pos[var_count++] = i;
vdbe_metadata_set_col_type(v, i,
field_type_strs[sql_expr_type(p)]);
+ if (is_full_meta && sql_expr_type(p) == FIELD_TYPE_STRING) {
+ bool unused;
+ uint32_t id;
+ struct coll *coll = NULL;
+ /*
+ * If sql_expr_coll fails then it fails somewhere
+ * above the call stack.
+ */
+ int rc = sql_expr_coll(pParse, p, &unused, &id, &coll);
+ assert(rc == 0);
+ (void) rc;
+ if (id != COLL_NONE) {
+ struct coll_id *coll_id = coll_by_id(id);
+ vdbe_metadata_set_col_collation(v, i,
+ coll_id->name,
+ coll_id->name_len);
+ }
+ }
if (p->op == TK_COLUMN || p->op == TK_AGG_COLUMN) {
char *zCol;
int iCol = p->iColumn;
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 2594b73e0..e9ad068f8 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -576,6 +576,9 @@ sql_column_name(sql_stmt *, int N);
const char *
sql_column_datatype(sql_stmt *, int N);
+const char *
+sql_column_coll(sql_stmt *stmt, int n);
+
int
sql_initialize(void);
@@ -1181,6 +1184,10 @@ struct sql {
#define SQL_EnableTrigger 0x01000000 /* True to enable triggers */
#define SQL_DeferFKs 0x02000000 /* Defer all FK constraints */
#define SQL_VdbeEQP 0x08000000 /* Debug EXPLAIN QUERY PLAN */
+#define SQL_FullMetadata 0x04000000 /* Display optional properties
+ * (nullability, autoincrement, alias)
+ * in metadata.
+ */
/* Bits of the sql.dbOptFlags field. */
#define SQL_QueryFlattener 0x0001 /* Query flattening */
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index ddced20e1..22ba0b756 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -253,6 +253,10 @@ vdbe_metadata_set_col_name(struct Vdbe *v, int col_idx, const char *name);
int
vdbe_metadata_set_col_type(struct Vdbe *v, int col_idx, const char *type);
+int
+vdbe_metadata_set_col_collation(struct Vdbe *p, int idx, const char *coll,
+ size_t coll_len);
+
void sqlVdbeCountChanges(Vdbe *);
sql *sqlVdbeDb(Vdbe *);
void sqlVdbeSetSql(Vdbe *, const char *z, int n, int);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 64250bee2..d3de5770b 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -349,6 +349,7 @@ struct ScanStatus {
struct sql_column_metadata {
char *name;
char *type;
+ char *collation;
};
/*
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 48db0bf43..e57a80334 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -745,6 +745,15 @@ sql_column_datatype(sql_stmt *stmt, int n)
return p->metadata[n].type;
}
+const char *
+sql_column_coll(sql_stmt *stmt, int n)
+{
+ struct Vdbe *p = (struct Vdbe *) stmt;
+ assert(n < sql_column_count(stmt) && n >= 0);
+ return p->metadata[n].collation;
+}
+
+
/******************************* sql_bind_ **************************
*
* Routines used to attach values to wildcards in a compiled SQL statement.
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index f2cf386bb..02d9fc6ce 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1834,6 +1834,7 @@ vdbe_metadata_delete(struct Vdbe *v)
for (int i = 0; i < v->nResColumn; ++i) {
free(v->metadata[i].name);
free(v->metadata[i].type);
+ free(v->metadata[i].collation);
}
free(v->metadata);
}
@@ -1888,6 +1889,21 @@ vdbe_metadata_set_col_type(struct Vdbe *p, int idx, const char *type)
return 0;
}
+int
+vdbe_metadata_set_col_collation(struct Vdbe *p, int idx, const char *coll,
+ size_t coll_len)
+{
+ assert(idx < p->nResColumn);
+ if (p->metadata[idx].collation != NULL)
+ free((void *)p->metadata[idx].collation);
+ p->metadata[idx].collation = strndup(coll, coll_len);
+ if (p->metadata[idx].collation == NULL) {
+ diag_set(OutOfMemory, coll_len + 1, "strndup", "collation");
+ return -1;
+ }
+ return 0;
+}
+
/*
* This routine checks that the sql.nVdbeActive count variable
* matches the number of vdbe's in the list sql.pVdbe that are
diff --git a/test/sql/engine.cfg b/test/sql/engine.cfg
index 284c42082..efc42a114 100644
--- a/test/sql/engine.cfg
+++ b/test/sql/engine.cfg
@@ -9,6 +9,10 @@
"remote": {"remote": "true"},
"local": {"remote": "false"}
},
+ "full_metadata.test.lua": {
+ "remote": {"remote": "true"},
+ "local": {"remote": "false"}
+ },
"*": {
"memtx": {"engine": "memtx"},
"vinyl": {"engine": "vinyl"}
diff --git a/test/sql/full_metadata.result b/test/sql/full_metadata.result
new file mode 100644
index 000000000..aebb938fe
--- /dev/null
+++ b/test/sql/full_metadata.result
@@ -0,0 +1,106 @@
+-- test-run result file version 2
+netbox = require('net.box')
+ | ---
+ | ...
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+box.execute("CREATE TABLE t (id INT PRIMARY KEY AUTOINCREMENT, a INT NOT NULL, c TEXT COLLATE \"unicode_ci\");")
+ | ---
+ | - row_count: 1
+ | ...
+box.execute("INSERT INTO t VALUES (1, 1, 'aSd');")
+ | ---
+ | - row_count: 1
+ | ...
+
+remote = test_run:get_cfg('remote') == 'true'
+ | ---
+ | ...
+execute = nil
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+if remote then
+ box.schema.user.grant('guest','read, write, execute', 'universe')
+ box.schema.user.grant('guest', 'create', 'space')
+ cn = netbox.connect(box.cfg.listen)
+ execute = function(...) return cn:execute(...) end
+else
+ execute = function(...)
+ local res, err = box.execute(...)
+ if err ~= nil then
+ error(err)
+ end
+ return res
+ end
+end;
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+execute("PRAGMA full_metadata = true;")
+ | ---
+ | - row_count: 0
+ | ...
+-- Make sure collation is presented in extended metadata.
+--
+execute("SELECT 'aSd' COLLATE \"unicode_ci\";")
+ | ---
+ | - metadata:
+ | - type: string
+ | name: '''aSd'' COLLATE "unicode_ci"'
+ | collation: unicode_ci
+ | rows:
+ | - ['aSd']
+ | ...
+execute("SELECT c FROM t;")
+ | ---
+ | - metadata:
+ | - type: string
+ | name: C
+ | collation: unicode_ci
+ | rows:
+ | - ['aSd']
+ | ...
+execute("SELECT c COLLATE \"unicode\" FROM t;")
+ | ---
+ | - metadata:
+ | - type: string
+ | name: c COLLATE "unicode"
+ | collation: unicode
+ | rows:
+ | - ['aSd']
+ | ...
+
+execute("PRAGMA full_metadata = false;")
+ | ---
+ | - row_count: 0
+ | ...
+
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+if remote then
+ cn:close()
+ box.schema.user.revoke('guest', 'read, write, execute', 'universe')
+ box.schema.user.revoke('guest', 'create', 'space')
+end;
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+box.space.T:drop()
+ | ---
+ | ...
diff --git a/test/sql/full_metadata.test.lua b/test/sql/full_metadata.test.lua
new file mode 100644
index 000000000..4aa2492a1
--- /dev/null
+++ b/test/sql/full_metadata.test.lua
@@ -0,0 +1,43 @@
+netbox = require('net.box')
+test_run = require('test_run').new()
+
+box.execute("CREATE TABLE t (id INT PRIMARY KEY AUTOINCREMENT, a INT NOT NULL, c TEXT COLLATE \"unicode_ci\");")
+box.execute("INSERT INTO t VALUES (1, 1, 'aSd');")
+
+remote = test_run:get_cfg('remote') == 'true'
+execute = nil
+test_run:cmd("setopt delimiter ';'")
+if remote then
+ box.schema.user.grant('guest','read, write, execute', 'universe')
+ box.schema.user.grant('guest', 'create', 'space')
+ cn = netbox.connect(box.cfg.listen)
+ execute = function(...) return cn:execute(...) end
+else
+ execute = function(...)
+ local res, err = box.execute(...)
+ if err ~= nil then
+ error(err)
+ end
+ return res
+ end
+end;
+test_run:cmd("setopt delimiter ''");
+
+execute("PRAGMA full_metadata = true;")
+-- Make sure collation is presented in extended metadata.
+--
+execute("SELECT 'aSd' COLLATE \"unicode_ci\";")
+execute("SELECT c FROM t;")
+execute("SELECT c COLLATE \"unicode\" FROM t;")
+
+execute("PRAGMA full_metadata = false;")
+
+test_run:cmd("setopt delimiter ';'")
+if remote then
+ cn:close()
+ box.schema.user.revoke('guest', 'read, write, execute', 'universe')
+ box.schema.user.revoke('guest', 'create', 'space')
+end;
+test_run:cmd("setopt delimiter ''");
+
+box.space.T:drop()
diff --git a/test/sql/sql-debug.result b/test/sql/sql-debug.result
index b19075366..a1b97663c 100644
--- a/test/sql/sql-debug.result
+++ b/test/sql/sql-debug.result
@@ -41,6 +41,7 @@ box.execute('PRAGMA')
- ['count_changes', 0]
- ['defer_foreign_keys', 0]
- ['full_column_names', 0]
+ - ['full_metadata', 0]
- ['parser_trace', 0]
- ['recursive_triggers', 1]
- ['reverse_unordered_selects', 0]
--
2.15.1
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 3/6] sql: extend result set with collation
2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 3/6] sql: extend result set with collation Nikita Pettik
@ 2019-12-18 0:29 ` Vladislav Shpilevoy
2019-12-24 0:26 ` Nikita Pettik
0 siblings, 1 reply; 25+ messages in thread
From: Vladislav Shpilevoy @ 2019-12-18 0:29 UTC (permalink / raw)
To: Nikita Pettik, tarantool-patches
Hi! Thanks for the patch!
See 3 comments below.
On 11/12/2019 14:44, Nikita Pettik wrote:
> If resulting set column is of STRING type and features collation (no
> matter explicit or implicit) different from "none", then metadata will
> contain its name.
>
> This patch also introduces new pragma: full_metadata. By default it is
> not set. If it is turned on, then optional metadata (like collation) is
> pushed to Lua stack. Note that via IProto protocol always full metadata
> is send, but its decoding depends on session SQL settings.
>
> Part of #4407
> diff --git a/src/box/execute.c b/src/box/execute.c
> index e8b012e5b..72300235a 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -268,6 +268,24 @@ error:
> return -1;
> }
>
> +static size_t
> +metadata_map_sizeof(const char *name, const char *type, const char *coll)
1. Since you moved sizeof to a separate function, maybe it is
worth doing the same with encoding part.
> +{
> + uint32_t members_count = 2;
> + size_t map_size = 0;
> + if (coll != NULL) {
> + members_count++;
> + map_size += mp_sizeof_uint(IPROTO_FIELD_COLL);
> + map_size += mp_sizeof_str(strlen(coll));
> + }
> + map_size += mp_sizeof_uint(IPROTO_FIELD_NAME);
> + map_size += mp_sizeof_uint(IPROTO_FIELD_TYPE);
> + map_size += mp_sizeof_str(strlen(name));
> + map_size += mp_sizeof_str(strlen(type));
> + map_size += mp_sizeof_map(members_count);
> + return map_size;
> +}
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index e4768121e..506c1a7c7 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1794,6 +1795,24 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> var_pos[var_count++] = i;
> vdbe_metadata_set_col_type(v, i,
> field_type_strs[sql_expr_type(p)]);
> + if (is_full_meta && sql_expr_type(p) == FIELD_TYPE_STRING) {
> + bool unused;
> + uint32_t id;
2. I would initialize id with 0, because a compiler may complain
about usage of not initialized id below. Now it does not, but
it can start, after any change in the code or in the compiler
version.
> + struct coll *coll = NULL;
> + /*
> + * If sql_expr_coll fails then it fails somewhere
> + * above the call stack.
> + */
> + int rc = sql_expr_coll(pParse, p, &unused, &id, &coll);
> + assert(rc == 0);
> + (void) rc;
> + if (id != COLL_NONE) {
> + struct coll_id *coll_id = coll_by_id(id);
> + vdbe_metadata_set_col_collation(v, i,
> + coll_id->name,
> + coll_id->name_len);
> + }
> + }
> if (p->op == TK_COLUMN || p->op == TK_AGG_COLUMN) {
> char *zCol;
> int iCol = p->iColumn;
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index f2cf386bb..02d9fc6ce 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -1888,6 +1889,21 @@ vdbe_metadata_set_col_type(struct Vdbe *p, int idx, const char *type)
> return 0;
> }
>
> +int
> +vdbe_metadata_set_col_collation(struct Vdbe *p, int idx, const char *coll,
> + size_t coll_len)
> +{
> + assert(idx < p->nResColumn);
> + if (p->metadata[idx].collation != NULL)
> + free((void *)p->metadata[idx].collation);
3. Please, remove (void *) cast. You don't need it since you
dropped 'const' from sql_column_metadata members. In the next
patches in similar places too.
> + p->metadata[idx].collation = strndup(coll, coll_len);
> + if (p->metadata[idx].collation == NULL) {
> + diag_set(OutOfMemory, coll_len + 1, "strndup", "collation");
> + return -1;
> + }
> + return 0;
> +}
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 3/6] sql: extend result set with collation
2019-12-18 0:29 ` Vladislav Shpilevoy
@ 2019-12-24 0:26 ` Nikita Pettik
2019-12-24 15:30 ` Vladislav Shpilevoy
0 siblings, 1 reply; 25+ messages in thread
From: Nikita Pettik @ 2019-12-24 0:26 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On 18 Dec 01:29, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
>
> See 3 comments below.
>
> On 11/12/2019 14:44, Nikita Pettik wrote:
> > If resulting set column is of STRING type and features collation (no
> > matter explicit or implicit) different from "none", then metadata will
> > contain its name.
> >
> > This patch also introduces new pragma: full_metadata. By default it is
> > not set. If it is turned on, then optional metadata (like collation) is
> > pushed to Lua stack. Note that via IProto protocol always full metadata
> > is send, but its decoding depends on session SQL settings.
> >
> > Part of #4407
> > diff --git a/src/box/execute.c b/src/box/execute.c
> > index e8b012e5b..72300235a 100644
> > --- a/src/box/execute.c
> > +++ b/src/box/execute.c
> > @@ -268,6 +268,24 @@ error:
> > return -1;
> > }
> >
> > +static size_t
> > +metadata_map_sizeof(const char *name, const char *type, const char *coll)
>
> 1. Since you moved sizeof to a separate function, maybe it is
> worth doing the same with encoding part.
>
> > +{
> > + uint32_t members_count = 2;
> > + size_t map_size = 0;
> > + if (coll != NULL) {
> > + members_count++;
> > + map_size += mp_sizeof_uint(IPROTO_FIELD_COLL);
> > + map_size += mp_sizeof_str(strlen(coll));
> > + }
> > + map_size += mp_sizeof_uint(IPROTO_FIELD_NAME);
> > + map_size += mp_sizeof_uint(IPROTO_FIELD_TYPE);
> > + map_size += mp_sizeof_str(strlen(name));
> > + map_size += mp_sizeof_str(strlen(type));
> > + map_size += mp_sizeof_map(members_count);
> > + return map_size;
> > +}
> > diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> > index e4768121e..506c1a7c7 100644
> > --- a/src/box/sql/select.c
> > +++ b/src/box/sql/select.c
> > @@ -1794,6 +1795,24 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> > var_pos[var_count++] = i;
> > vdbe_metadata_set_col_type(v, i,
> > field_type_strs[sql_expr_type(p)]);
> > + if (is_full_meta && sql_expr_type(p) == FIELD_TYPE_STRING) {
> > + bool unused;
> > + uint32_t id;
>
> 2. I would initialize id with 0, because a compiler may complain
> about usage of not initialized id below. Now it does not, but
> it can start, after any change in the code or in the compiler
> version.
>
> > + struct coll *coll = NULL;
> > + /*
> > + * If sql_expr_coll fails then it fails somewhere
> > + * above the call stack.
> > + */
> > + int rc = sql_expr_coll(pParse, p, &unused, &id, &coll);
> > + assert(rc == 0);
> > + (void) rc;
> > + if (id != COLL_NONE) {
> > + struct coll_id *coll_id = coll_by_id(id);
> > + vdbe_metadata_set_col_collation(v, i,
> > + coll_id->name,
> > + coll_id->name_len);
> > + }
> > + }
> > if (p->op == TK_COLUMN || p->op == TK_AGG_COLUMN) {
> > char *zCol;
> > int iCol = p->iColumn;
> > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> > index f2cf386bb..02d9fc6ce 100644
> > --- a/src/box/sql/vdbeaux.c
> > +++ b/src/box/sql/vdbeaux.c
> > @@ -1888,6 +1889,21 @@ vdbe_metadata_set_col_type(struct Vdbe *p, int idx, const char *type)
> > return 0;
> > }
> >
> > +int
> > +vdbe_metadata_set_col_collation(struct Vdbe *p, int idx, const char *coll,
> > + size_t coll_len)
> > +{
> > + assert(idx < p->nResColumn);
> > + if (p->metadata[idx].collation != NULL)
> > + free((void *)p->metadata[idx].collation);
>
> 3. Please, remove (void *) cast. You don't need it since you
> dropped 'const' from sql_column_metadata members. In the next
> patches in similar places too.
>
> > + p->metadata[idx].collation = strndup(coll, coll_len);
> > + if (p->metadata[idx].collation == NULL) {
> > + diag_set(OutOfMemory, coll_len + 1, "strndup", "collation");
> > + return -1;
> > + }
> > + return 0;
> > +}
Fixed all nits:
diff --git a/src/box/execute.c b/src/box/execute.c
index 72300235a..887e05eb8 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -286,6 +286,22 @@ metadata_map_sizeof(const char *name, const char *type, const char *coll)
return map_size;
}
+static inline void
+metadata_map_encode(char *buf, const char *name, const char *type,
+ const char *coll)
+{
+ uint32_t map_sz = 2 + (coll != NULL);
+ buf = mp_encode_map(buf, map_sz);
+ buf = mp_encode_uint(buf, IPROTO_FIELD_NAME);
+ buf = mp_encode_str(buf, name, strlen(name));
+ buf = mp_encode_uint(buf, IPROTO_FIELD_TYPE);
+ buf = mp_encode_str(buf, type, strlen(type));
+ if (coll != NULL) {
+ buf = mp_encode_uint(buf, IPROTO_FIELD_COLL);
+ buf = mp_encode_str(buf, coll, strlen(coll));
+ }
+}
+
/**
* Serialize a description of the prepared statement.
* @param stmt Prepared statement.
@@ -325,16 +341,7 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
diag_set(OutOfMemory, size, "obuf_alloc", "pos");
return -1;
}
- uint32_t map_sz = 2 + (coll != NULL);
- pos = mp_encode_map(pos, map_sz);
- pos = mp_encode_uint(pos, IPROTO_FIELD_NAME);
- pos = mp_encode_str(pos, name, strlen(name));
- pos = mp_encode_uint(pos, IPROTO_FIELD_TYPE);
- pos = mp_encode_str(pos, type, strlen(type));
- if (coll != NULL) {
- pos = mp_encode_uint(pos, IPROTO_FIELD_COLL);
- pos = mp_encode_str(pos, coll, strlen(coll));
- }
+ metadata_map_encode(pos, name, type, coll);
}
return 0;
}
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 506c1a7c7..a23d2692e 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1797,7 +1797,7 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
field_type_strs[sql_expr_type(p)]);
if (is_full_meta && sql_expr_type(p) == FIELD_TYPE_STRING) {
bool unused;
- uint32_t id;
+ uint32_t id = 0;
struct coll *coll = NULL;
/*
* If sql_expr_coll fails then it fails somewhere
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 02d9fc6ce..bc9aed81a 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1895,7 +1895,7 @@ vdbe_metadata_set_col_collation(struct Vdbe *p, int idx, const char *coll,
{
assert(idx < p->nResColumn);
if (p->metadata[idx].collation != NULL)
- free((void *)p->metadata[idx].collation);
+ free(p->metadata[idx].collation);
p->metadata[idx].collation = strndup(coll, coll_len);
if (p->metadata[idx].collation == NULL) {
diag_set(OutOfMemory, coll_len + 1, "strndup", "collation");
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 3/6] sql: extend result set with collation
2019-12-24 0:26 ` Nikita Pettik
@ 2019-12-24 15:30 ` Vladislav Shpilevoy
0 siblings, 0 replies; 25+ messages in thread
From: Vladislav Shpilevoy @ 2019-12-24 15:30 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
Hi! Thanks for the patch!
I've pushed my review fixes on top of this commit. See it below
and on the branch.
> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index 48db0bf43..e57a80334 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -745,6 +745,15 @@ sql_column_datatype(sql_stmt *stmt, int n)
> return p->metadata[n].type;
> }
>
> +const char *
> +sql_column_coll(sql_stmt *stmt, int n)
> +{
> + struct Vdbe *p = (struct Vdbe *) stmt;
> + assert(n < sql_column_count(stmt) && n >= 0);
> + return p->metadata[n].collation;
> +}
> +
> +
1. Double empty line.
> /******************************* sql_bind_ **************************
> *
> * Routines used to attach values to wildcards in a compiled SQL statement.
> diff --git a/test/sql/full_metadata.test.lua b/test/sql/full_metadata.test.lua
> new file mode 100644
> index 000000000..4aa2492a1
> --- /dev/null
> +++ b/test/sql/full_metadata.test.lua
2. Seems like you use tabs in that test file. According to
our code style, we use 4-space blocks in Lua files instead
of tabs.
================================================================================
commit 635cd1999708d0d731e46ddda64a97e4da55d082
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date: Tue Dec 24 15:46:28 2019 +0100
Review fix 3/6
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index e57a80334..61b98e0c5 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -753,7 +753,6 @@ sql_column_coll(sql_stmt *stmt, int n)
return p->metadata[n].collation;
}
-
/******************************* sql_bind_ **************************
*
* Routines used to attach values to wildcards in a compiled SQL statement.
diff --git a/test/sql/full_metadata.result b/test/sql/full_metadata.result
index aebb938fe..76d6107a1 100644
--- a/test/sql/full_metadata.result
+++ b/test/sql/full_metadata.result
@@ -26,18 +26,18 @@ test_run:cmd("setopt delimiter ';'")
| - true
| ...
if remote then
- box.schema.user.grant('guest','read, write, execute', 'universe')
- box.schema.user.grant('guest', 'create', 'space')
- cn = netbox.connect(box.cfg.listen)
- execute = function(...) return cn:execute(...) end
+ box.schema.user.grant('guest','read, write, execute', 'universe')
+ box.schema.user.grant('guest', 'create', 'space')
+ cn = netbox.connect(box.cfg.listen)
+ execute = function(...) return cn:execute(...) end
else
- execute = function(...)
- local res, err = box.execute(...)
- if err ~= nil then
- error(err)
- end
- return res
- end
+ execute = function(...)
+ local res, err = box.execute(...)
+ if err ~= nil then
+ error(err)
+ end
+ return res
+ end
end;
| ---
| ...
@@ -90,9 +90,9 @@ test_run:cmd("setopt delimiter ';'")
| - true
| ...
if remote then
- cn:close()
- box.schema.user.revoke('guest', 'read, write, execute', 'universe')
- box.schema.user.revoke('guest', 'create', 'space')
+ cn:close()
+ box.schema.user.revoke('guest', 'read, write, execute', 'universe')
+ box.schema.user.revoke('guest', 'create', 'space')
end;
| ---
| ...
diff --git a/test/sql/full_metadata.test.lua b/test/sql/full_metadata.test.lua
index 4aa2492a1..7b4637b1e 100644
--- a/test/sql/full_metadata.test.lua
+++ b/test/sql/full_metadata.test.lua
@@ -8,18 +8,18 @@ remote = test_run:get_cfg('remote') == 'true'
execute = nil
test_run:cmd("setopt delimiter ';'")
if remote then
- box.schema.user.grant('guest','read, write, execute', 'universe')
- box.schema.user.grant('guest', 'create', 'space')
- cn = netbox.connect(box.cfg.listen)
- execute = function(...) return cn:execute(...) end
+ box.schema.user.grant('guest','read, write, execute', 'universe')
+ box.schema.user.grant('guest', 'create', 'space')
+ cn = netbox.connect(box.cfg.listen)
+ execute = function(...) return cn:execute(...) end
else
- execute = function(...)
- local res, err = box.execute(...)
- if err ~= nil then
- error(err)
- end
- return res
- end
+ execute = function(...)
+ local res, err = box.execute(...)
+ if err ~= nil then
+ error(err)
+ end
+ return res
+ end
end;
test_run:cmd("setopt delimiter ''");
@@ -34,9 +34,9 @@ execute("PRAGMA full_metadata = false;")
test_run:cmd("setopt delimiter ';'")
if remote then
- cn:close()
- box.schema.user.revoke('guest', 'read, write, execute', 'universe')
- box.schema.user.revoke('guest', 'create', 'space')
+ cn:close()
+ box.schema.user.revoke('guest', 'read, write, execute', 'universe')
+ box.schema.user.revoke('guest', 'create', 'space')
end;
test_run:cmd("setopt delimiter ''");
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Tarantool-patches] [PATCH v2 4/6] sql: extend result set with nullability
[not found] ` <cover.1576071711.git.korablev@tarantool.org>
` (2 preceding siblings ...)
2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 3/6] sql: extend result set with collation Nikita Pettik
@ 2019-12-11 13:44 ` Nikita Pettik
2019-12-18 0:29 ` Vladislav Shpilevoy
2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 5/6] sql: extend result set with autoincrement Nikita Pettik
2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 6/6] sql: extend result set with alias Nikita Pettik
5 siblings, 1 reply; 25+ messages in thread
From: Nikita Pettik @ 2019-12-11 13:44 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
If member of result set is (solely) column identifier, then metadata
will contain its corresponding field nullability as boolean property.
Note that indicating nullability for other expressions (like x + 1)
may make sense but it requires derived nullability calculation which in
turn seems to be overkill (at least in scope of current patch).
Part of #4407
---
src/box/execute.c | 17 ++++++++++++++---
src/box/iproto_constants.h | 1 +
src/box/lua/execute.c | 7 ++++++-
src/box/lua/net_box.c | 7 ++++++-
src/box/sql/delete.c | 1 +
src/box/sql/insert.c | 1 +
src/box/sql/pragma.c | 3 +++
src/box/sql/prepare.c | 1 +
src/box/sql/select.c | 7 +++++++
src/box/sql/sqlInt.h | 3 +++
src/box/sql/update.c | 1 +
src/box/sql/vdbe.h | 3 +++
src/box/sql/vdbeInt.h | 5 +++++
src/box/sql/vdbeapi.c | 7 +++++++
src/box/sql/vdbeaux.c | 7 +++++++
test/sql/full_metadata.result | 36 ++++++++++++++++++++++++++++++++++++
test/sql/full_metadata.test.lua | 5 +++++
17 files changed, 107 insertions(+), 5 deletions(-)
diff --git a/src/box/execute.c b/src/box/execute.c
index 72300235a..2e43cdb22 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -269,7 +269,8 @@ error:
}
static size_t
-metadata_map_sizeof(const char *name, const char *type, const char *coll)
+metadata_map_sizeof(const char *name, const char *type, const char *coll,
+ int nullable)
{
uint32_t members_count = 2;
size_t map_size = 0;
@@ -278,6 +279,11 @@ metadata_map_sizeof(const char *name, const char *type, const char *coll)
map_size += mp_sizeof_uint(IPROTO_FIELD_COLL);
map_size += mp_sizeof_str(strlen(coll));
}
+ if (nullable != -1) {
+ members_count++;
+ map_size += mp_sizeof_uint(IPROTO_FIELD_IS_NULLABLE);
+ map_size += mp_sizeof_bool(nullable);
+ }
map_size += mp_sizeof_uint(IPROTO_FIELD_NAME);
map_size += mp_sizeof_uint(IPROTO_FIELD_TYPE);
map_size += mp_sizeof_str(strlen(name));
@@ -312,6 +318,7 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
const char *coll = sql_column_coll(stmt, i);
const char *name = sql_column_name(stmt, i);
const char *type = sql_column_datatype(stmt, i);
+ int nullable = sql_column_nullable(stmt, i);
/*
* Can not fail, since all column names and types
* are preallocated during prepare phase and the
@@ -319,13 +326,13 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
*/
assert(name != NULL);
assert(type != NULL);
- size = metadata_map_sizeof(name, type, coll);
+ size = metadata_map_sizeof(name, type, coll, nullable);
char *pos = (char *) obuf_alloc(out, size);
if (pos == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "pos");
return -1;
}
- uint32_t map_sz = 2 + (coll != NULL);
+ uint32_t map_sz = 2 + (coll != NULL) + (nullable != -1);
pos = mp_encode_map(pos, map_sz);
pos = mp_encode_uint(pos, IPROTO_FIELD_NAME);
pos = mp_encode_str(pos, name, strlen(name));
@@ -335,6 +342,10 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
pos = mp_encode_uint(pos, IPROTO_FIELD_COLL);
pos = mp_encode_str(pos, coll, strlen(coll));
}
+ if (nullable != -1) {
+ pos = mp_encode_uint(pos, IPROTO_FIELD_IS_NULLABLE);
+ pos = mp_encode_bool(pos, nullable);
+ }
}
return 0;
}
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index fa9c029a2..53d014b60 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -132,6 +132,7 @@ enum iproto_metadata_key {
IPROTO_FIELD_NAME = 0,
IPROTO_FIELD_TYPE = 1,
IPROTO_FIELD_COLL = 2,
+ IPROTO_FIELD_IS_NULLABLE = 3,
};
enum iproto_ballot_key {
diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index 55b8a8fef..debe94cb9 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -23,7 +23,8 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L,
const char *coll = sql_column_coll(stmt, i);
const char *name = sql_column_name(stmt, i);
const char *type = sql_column_datatype(stmt, i);
- size_t table_sz = 2 + (coll != NULL);
+ int nullable = sql_column_nullable(stmt, i);
+ size_t table_sz = 2 + (coll != NULL) + (nullable != -1);
lua_createtable(L, 0, table_sz);
/*
* Can not fail, since all column names are
@@ -40,6 +41,10 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L,
lua_pushstring(L, coll);
lua_setfield(L, -2, "collation");
}
+ if (nullable != -1) {
+ lua_pushboolean(L, nullable);
+ lua_setfield(L, -2, "is_nullable");
+ }
lua_rawseti(L, -2, i + 1);
}
}
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 025143307..644b373c9 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -651,6 +651,11 @@ decode_metadata_optional(struct lua_State *L, const char **data,
const char *coll = mp_decode_str(data, &len);
lua_pushlstring(L, coll, len);
lua_setfield(L, -2, "collation");
+ } else {
+ assert(key == IPROTO_FIELD_IS_NULLABLE);
+ bool is_nullable = mp_decode_bool(data);
+ lua_pushboolean(L, is_nullable);
+ lua_setfield(L, -2, "is_nullable");
}
}
}
@@ -667,7 +672,7 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
lua_createtable(L, count, 0);
for (uint32_t i = 0; i < count; ++i) {
uint32_t map_size = mp_decode_map(data);
- assert(map_size == 2 || map_size == 3);
+ assert(map_size >= 2 && map_size <= 4);
uint32_t key = mp_decode_uint(data);
assert(key == IPROTO_FIELD_NAME);
(void) key;
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 169814a2e..cc5891bb8 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -420,6 +420,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
sqlVdbeSetNumCols(v, 1);
vdbe_metadata_set_col_name(v, 0, "rows deleted");
vdbe_metadata_set_col_type(v, 0, "integer");
+ vdbe_metadata_set_col_nullability(v, 0, -1);
}
delete_from_cleanup:
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index f1290e01c..159acec25 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -787,6 +787,7 @@ sqlInsert(Parse * pParse, /* Parser context */
column_name = "rows inserted";
vdbe_metadata_set_col_name(v, 0, column_name);
vdbe_metadata_set_col_type(v, 0, "integer");
+ vdbe_metadata_set_col_nullability(v, 0, -1);
}
insert_cleanup:
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 00ecde0a9..a6a5f25ce 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -122,6 +122,7 @@ vdbe_set_pragma_result_columns(struct Vdbe *v, const struct PragmaName *pragma)
for (int i = 0, j = pragma->iPragCName; i < n; ++i) {
vdbe_metadata_set_col_name(v, i, pragCName[j++]);
vdbe_metadata_set_col_type(v, i, pragCName[j++]);
+ vdbe_metadata_set_col_nullability(v, i, -1);
}
}
@@ -168,8 +169,10 @@ vdbe_emit_pragma_status(struct Parse *parse)
sqlVdbeSetNumCols(v, 2);
vdbe_metadata_set_col_name(v, 0, "pragma_name");
vdbe_metadata_set_col_type(v, 0, "text");
+ vdbe_metadata_set_col_nullability(v, 0, -1);
vdbe_metadata_set_col_name(v, 1, "pragma_value");
vdbe_metadata_set_col_type(v, 1, "integer");
+ vdbe_metadata_set_col_nullability(v, 1, -1);
parse->nMem = 2;
for (int i = 0; i < ArraySize(aPragmaName); ++i) {
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 39e897ba3..5b5981c46 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -150,6 +150,7 @@ sqlPrepare(sql * db, /* Database handle. */
azColName[name_index]);
vdbe_metadata_set_col_type(sParse.pVdbe, i,
azColName[name_index + 1]);
+ vdbe_metadata_set_col_nullability(sParse.pVdbe, i, -1);
}
}
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 506c1a7c7..73ce95eba 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1813,6 +1813,7 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
coll_id->name_len);
}
}
+ vdbe_metadata_set_col_nullability(v, i, -1);
if (p->op == TK_COLUMN || p->op == TK_AGG_COLUMN) {
char *zCol;
int iCol = p->iColumn;
@@ -1839,6 +1840,12 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
}
}
vdbe_metadata_set_col_name(v, i, name);
+ if (is_full_meta) {
+ bool is_nullable =
+ space_def->fields[iCol].is_nullable;
+ vdbe_metadata_set_col_nullability(v, i,
+ is_nullable);
+ }
} else {
const char *z = NULL;
if (pEList->a[i].zName != NULL)
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index e9ad068f8..3698a5942 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -579,6 +579,9 @@ sql_column_datatype(sql_stmt *, int N);
const char *
sql_column_coll(sql_stmt *stmt, int n);
+int
+sql_column_nullable(sql_stmt *stmt, int n);
+
int
sql_initialize(void);
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index c08777a2d..489328960 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -500,6 +500,7 @@ sqlUpdate(Parse * pParse, /* The parser context */
sqlVdbeSetNumCols(v, 1);
vdbe_metadata_set_col_name(v, 0, "rows updated");
vdbe_metadata_set_col_type(v, 0, "integer");
+ vdbe_metadata_set_col_nullability(v, 0, -1);
}
update_cleanup:
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 22ba0b756..384d7dc0a 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -257,6 +257,9 @@ int
vdbe_metadata_set_col_collation(struct Vdbe *p, int idx, const char *coll,
size_t coll_len);
+void
+vdbe_metadata_set_col_nullability(struct Vdbe *p, int idx, int nullable);
+
void sqlVdbeCountChanges(Vdbe *);
sql *sqlVdbeDb(Vdbe *);
void sqlVdbeSetSql(Vdbe *, const char *z, int n, int);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index d3de5770b..92a50dd7b 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -350,6 +350,11 @@ struct sql_column_metadata {
char *name;
char *type;
char *collation;
+ /**
+ * -1 is for any member of result set except for pure
+ * columns: all other expressions are nullable by default.
+ */
+ int8_t nullable : 2;
};
/*
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index e57a80334..30f173984 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -753,6 +753,13 @@ sql_column_coll(sql_stmt *stmt, int n)
return p->metadata[n].collation;
}
+int
+sql_column_nullable(sql_stmt *stmt, int n)
+{
+ struct Vdbe *p = (struct Vdbe *) stmt;
+ assert(n < sql_column_count(stmt) && n >= 0);
+ return p->metadata[n].nullable;
+}
/******************************* sql_bind_ **************************
*
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 02d9fc6ce..2de41a70a 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1904,6 +1904,13 @@ vdbe_metadata_set_col_collation(struct Vdbe *p, int idx, const char *coll,
return 0;
}
+void
+vdbe_metadata_set_col_nullability(struct Vdbe *p, int idx, int nullable)
+{
+ assert(idx < p->nResColumn);
+ p->metadata[idx].nullable = nullable;
+}
+
/*
* This routine checks that the sql.nVdbeActive count variable
* matches the number of vdbe's in the list sql.pVdbe that are
diff --git a/test/sql/full_metadata.result b/test/sql/full_metadata.result
index aebb938fe..20ed29e1c 100644
--- a/test/sql/full_metadata.result
+++ b/test/sql/full_metadata.result
@@ -65,6 +65,7 @@ execute("SELECT c FROM t;")
| ---
| - metadata:
| - type: string
+ | is_nullable: true
| name: C
| collation: unicode_ci
| rows:
@@ -80,6 +81,41 @@ execute("SELECT c COLLATE \"unicode\" FROM t;")
| - ['aSd']
| ...
+-- Make sure that nullability is presented.
+--
+execute("SELECT id, a, c FROM t;")
+ | ---
+ | - metadata:
+ | - type: integer
+ | name: ID
+ | is_nullable: false
+ | - type: integer
+ | name: A
+ | is_nullable: false
+ | - type: string
+ | is_nullable: true
+ | name: C
+ | collation: unicode_ci
+ | rows:
+ | - [1, 1, 'aSd']
+ | ...
+execute("SELECT * FROM t;")
+ | ---
+ | - metadata:
+ | - type: integer
+ | name: ID
+ | is_nullable: false
+ | - type: integer
+ | name: A
+ | is_nullable: false
+ | - type: string
+ | is_nullable: true
+ | name: C
+ | collation: unicode_ci
+ | rows:
+ | - [1, 1, 'aSd']
+ | ...
+
execute("PRAGMA full_metadata = false;")
| ---
| - row_count: 0
diff --git a/test/sql/full_metadata.test.lua b/test/sql/full_metadata.test.lua
index 4aa2492a1..0ff5fefe6 100644
--- a/test/sql/full_metadata.test.lua
+++ b/test/sql/full_metadata.test.lua
@@ -30,6 +30,11 @@ execute("SELECT 'aSd' COLLATE \"unicode_ci\";")
execute("SELECT c FROM t;")
execute("SELECT c COLLATE \"unicode\" FROM t;")
+-- Make sure that nullability is presented.
+--
+execute("SELECT id, a, c FROM t;")
+execute("SELECT * FROM t;")
+
execute("PRAGMA full_metadata = false;")
test_run:cmd("setopt delimiter ';'")
--
2.15.1
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 4/6] sql: extend result set with nullability
2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 4/6] sql: extend result set with nullability Nikita Pettik
@ 2019-12-18 0:29 ` Vladislav Shpilevoy
2019-12-24 0:26 ` Nikita Pettik
0 siblings, 1 reply; 25+ messages in thread
From: Vladislav Shpilevoy @ 2019-12-18 0:29 UTC (permalink / raw)
To: Nikita Pettik, tarantool-patches
Thanks for the patch!
See 2 comments below.
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index 169814a2e..cc5891bb8 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -420,6 +420,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> sqlVdbeSetNumCols(v, 1);
> vdbe_metadata_set_col_name(v, 0, "rows deleted");
> vdbe_metadata_set_col_type(v, 0, "integer");
> + vdbe_metadata_set_col_nullability(v, 0, -1);
1. All these set_col_nullability(-1) look really doubtful. You
don't set collation NULL explicitly, but you set nullability -1
explicitly. Why?
I think it is much better to fill default values of newly allocated
sql_column_metadata in sqlVdbeSetNumCols. See my diff (not on the
branch):
==================================================================
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index cc5891bb8..169814a2e 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -420,7 +420,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
sqlVdbeSetNumCols(v, 1);
vdbe_metadata_set_col_name(v, 0, "rows deleted");
vdbe_metadata_set_col_type(v, 0, "integer");
- vdbe_metadata_set_col_nullability(v, 0, -1);
}
delete_from_cleanup:
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 159acec25..f1290e01c 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -787,7 +787,6 @@ sqlInsert(Parse * pParse, /* Parser context */
column_name = "rows inserted";
vdbe_metadata_set_col_name(v, 0, column_name);
vdbe_metadata_set_col_type(v, 0, "integer");
- vdbe_metadata_set_col_nullability(v, 0, -1);
}
insert_cleanup:
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index a6a5f25ce..00ecde0a9 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -122,7 +122,6 @@ vdbe_set_pragma_result_columns(struct Vdbe *v, const struct PragmaName *pragma)
for (int i = 0, j = pragma->iPragCName; i < n; ++i) {
vdbe_metadata_set_col_name(v, i, pragCName[j++]);
vdbe_metadata_set_col_type(v, i, pragCName[j++]);
- vdbe_metadata_set_col_nullability(v, i, -1);
}
}
@@ -169,10 +168,8 @@ vdbe_emit_pragma_status(struct Parse *parse)
sqlVdbeSetNumCols(v, 2);
vdbe_metadata_set_col_name(v, 0, "pragma_name");
vdbe_metadata_set_col_type(v, 0, "text");
- vdbe_metadata_set_col_nullability(v, 0, -1);
vdbe_metadata_set_col_name(v, 1, "pragma_value");
vdbe_metadata_set_col_type(v, 1, "integer");
- vdbe_metadata_set_col_nullability(v, 1, -1);
parse->nMem = 2;
for (int i = 0; i < ArraySize(aPragmaName); ++i) {
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 5b5981c46..39e897ba3 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -150,7 +150,6 @@ sqlPrepare(sql * db, /* Database handle. */
azColName[name_index]);
vdbe_metadata_set_col_type(sParse.pVdbe, i,
azColName[name_index + 1]);
- vdbe_metadata_set_col_nullability(sParse.pVdbe, i, -1);
}
}
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 73ce95eba..63f517e62 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1813,7 +1813,6 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
coll_id->name_len);
}
}
- vdbe_metadata_set_col_nullability(v, i, -1);
if (p->op == TK_COLUMN || p->op == TK_AGG_COLUMN) {
char *zCol;
int iCol = p->iColumn;
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 489328960..c08777a2d 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -500,7 +500,6 @@ sqlUpdate(Parse * pParse, /* The parser context */
sqlVdbeSetNumCols(v, 1);
vdbe_metadata_set_col_name(v, 0, "rows updated");
vdbe_metadata_set_col_type(v, 0, "integer");
- vdbe_metadata_set_col_nullability(v, 0, -1);
}
update_cleanup:
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 2de41a70a..ef542efc3 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1849,6 +1849,7 @@ vdbe_metadata_delete(struct Vdbe *v)
void
sqlVdbeSetNumCols(Vdbe * p, int nResColumn)
{
+ int old_n = p->nResColumn;
vdbe_metadata_delete(p);
p->nResColumn = (u16) nResColumn;
p->metadata = (struct sql_column_metadata *)
@@ -1859,6 +1860,9 @@ sqlVdbeSetNumCols(Vdbe * p, int nResColumn)
"calloc", "metadata");
return;
}
+ for (int i = old_n; i < nResColumn; ++i)
+ p->metadata[i].nullable = -1;
+
}
==================================================================
> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> index d3de5770b..92a50dd7b 100644
> --- a/src/box/sql/vdbeInt.h
> +++ b/src/box/sql/vdbeInt.h
> @@ -350,6 +350,11 @@ struct sql_column_metadata {
> char *name;
> char *type;
> char *collation;
> + /**
> + * -1 is for any member of result set except for pure
> + * columns: all other expressions are nullable by default.
> + */
> + int8_t nullable : 2;
2. What is a point of having it :2 bits, if you don't
have other flags in the same byte? Due to alignment
this member anyway will become 1 byte.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 4/6] sql: extend result set with nullability
2019-12-18 0:29 ` Vladislav Shpilevoy
@ 2019-12-24 0:26 ` Nikita Pettik
0 siblings, 0 replies; 25+ messages in thread
From: Nikita Pettik @ 2019-12-24 0:26 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On 18 Dec 01:29, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 2 comments below.
>
> > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> > index 169814a2e..cc5891bb8 100644
> > --- a/src/box/sql/delete.c
> > +++ b/src/box/sql/delete.c
> > @@ -420,6 +420,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> > sqlVdbeSetNumCols(v, 1);
> > vdbe_metadata_set_col_name(v, 0, "rows deleted");
> > vdbe_metadata_set_col_type(v, 0, "integer");
> > + vdbe_metadata_set_col_nullability(v, 0, -1);
>
> 1. All these set_col_nullability(-1) look really doubtful. You
> don't set collation NULL explicitly, but you set nullability -1
> explicitly. Why?
>
> I think it is much better to fill default values of newly allocated
> sql_column_metadata in sqlVdbeSetNumCols. See my diff (not on the
> branch):
Looks reasonable. Thx, applied.
> ==================================================================
>
> > diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> > index d3de5770b..92a50dd7b 100644
> > --- a/src/box/sql/vdbeInt.h
> > +++ b/src/box/sql/vdbeInt.h
> > @@ -350,6 +350,11 @@ struct sql_column_metadata {
> > char *name;
> > char *type;
> > char *collation;
> > + /**
> > + * -1 is for any member of result set except for pure
> > + * columns: all other expressions are nullable by default.
> > + */
> > + int8_t nullable : 2;
>
> 2. What is a point of having it :2 bits, if you don't
> have other flags in the same byte? Due to alignment
> this member anyway will become 1 byte.
I guess it's an artifact of one of previous iteration of review fixes
or of the original patch. Anyway, removed this bit field.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Tarantool-patches] [PATCH v2 5/6] sql: extend result set with autoincrement
[not found] ` <cover.1576071711.git.korablev@tarantool.org>
` (3 preceding siblings ...)
2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 4/6] sql: extend result set with nullability Nikita Pettik
@ 2019-12-11 13:44 ` Nikita Pettik
2019-12-18 0:29 ` Vladislav Shpilevoy
2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 6/6] sql: extend result set with alias Nikita Pettik
5 siblings, 1 reply; 25+ messages in thread
From: Nikita Pettik @ 2019-12-11 13:44 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
If result set contains column which features attached sequence
(AUTOINCREMENT in terms of SQL) then meta-information will contain
corresponding field ('is_autoicrement' : boolean) in response.
Part of #4407
---
src/box/execute.c | 18 +++++++++++++++---
src/box/iproto_constants.h | 1 +
src/box/lua/execute.c | 8 +++++++-
src/box/lua/net_box.c | 10 +++++++---
src/box/sql/select.c | 6 ++++++
src/box/sql/sqlInt.h | 3 +++
src/box/sql/vdbe.h | 3 +++
src/box/sql/vdbeInt.h | 2 ++
src/box/sql/vdbeapi.c | 8 ++++++++
src/box/sql/vdbeaux.c | 7 +++++++
test/sql/full_metadata.result | 4 +++-
test/sql/full_metadata.test.lua | 2 +-
12 files changed, 63 insertions(+), 9 deletions(-)
diff --git a/src/box/execute.c b/src/box/execute.c
index 2e43cdb22..c853991a0 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -270,7 +270,7 @@ error:
static size_t
metadata_map_sizeof(const char *name, const char *type, const char *coll,
- int nullable)
+ int nullable, bool is_autoincrement)
{
uint32_t members_count = 2;
size_t map_size = 0;
@@ -284,6 +284,11 @@ metadata_map_sizeof(const char *name, const char *type, const char *coll,
map_size += mp_sizeof_uint(IPROTO_FIELD_IS_NULLABLE);
map_size += mp_sizeof_bool(nullable);
}
+ if (is_autoincrement) {
+ members_count++;
+ map_size += mp_sizeof_uint(IPROTO_FIELD_IS_AUTOINCREMENT);
+ map_size += mp_sizeof_bool(true);
+ }
map_size += mp_sizeof_uint(IPROTO_FIELD_NAME);
map_size += mp_sizeof_uint(IPROTO_FIELD_TYPE);
map_size += mp_sizeof_str(strlen(name));
@@ -319,6 +324,7 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
const char *name = sql_column_name(stmt, i);
const char *type = sql_column_datatype(stmt, i);
int nullable = sql_column_nullable(stmt, i);
+ bool is_autoincrement = sql_column_is_autoincrement(stmt, i);
/*
* Can not fail, since all column names and types
* are preallocated during prepare phase and the
@@ -326,13 +332,15 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
*/
assert(name != NULL);
assert(type != NULL);
- size = metadata_map_sizeof(name, type, coll, nullable);
+ size = metadata_map_sizeof(name, type, coll, nullable,
+ is_autoincrement);
char *pos = (char *) obuf_alloc(out, size);
if (pos == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "pos");
return -1;
}
- uint32_t map_sz = 2 + (coll != NULL) + (nullable != -1);
+ uint32_t map_sz = 2 + (coll != NULL) + (nullable != -1) +
+ is_autoincrement;
pos = mp_encode_map(pos, map_sz);
pos = mp_encode_uint(pos, IPROTO_FIELD_NAME);
pos = mp_encode_str(pos, name, strlen(name));
@@ -346,6 +354,10 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
pos = mp_encode_uint(pos, IPROTO_FIELD_IS_NULLABLE);
pos = mp_encode_bool(pos, nullable);
}
+ if (is_autoincrement) {
+ pos = mp_encode_uint(pos, IPROTO_FIELD_IS_AUTOINCREMENT);
+ pos = mp_encode_bool(pos, true);
+ }
}
return 0;
}
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index 53d014b60..30d1af4cb 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -133,6 +133,7 @@ enum iproto_metadata_key {
IPROTO_FIELD_TYPE = 1,
IPROTO_FIELD_COLL = 2,
IPROTO_FIELD_IS_NULLABLE = 3,
+ IPROTO_FIELD_IS_AUTOINCREMENT = 4,
};
enum iproto_ballot_key {
diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index debe94cb9..e8e3e2a9f 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -24,7 +24,9 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L,
const char *name = sql_column_name(stmt, i);
const char *type = sql_column_datatype(stmt, i);
int nullable = sql_column_nullable(stmt, i);
- size_t table_sz = 2 + (coll != NULL) + (nullable != -1);
+ bool is_autoincrement = sql_column_is_autoincrement(stmt, i);
+ size_t table_sz = 2 + (coll != NULL) + (nullable != -1) +
+ is_autoincrement;
lua_createtable(L, 0, table_sz);
/*
* Can not fail, since all column names are
@@ -45,6 +47,10 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L,
lua_pushboolean(L, nullable);
lua_setfield(L, -2, "is_nullable");
}
+ if (is_autoincrement) {
+ lua_pushboolean(L, true);
+ lua_setfield(L, -2, "is_autoincrement");
+ }
lua_rawseti(L, -2, i + 1);
}
}
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 644b373c9..88ef4ff78 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -651,11 +651,15 @@ decode_metadata_optional(struct lua_State *L, const char **data,
const char *coll = mp_decode_str(data, &len);
lua_pushlstring(L, coll, len);
lua_setfield(L, -2, "collation");
- } else {
- assert(key == IPROTO_FIELD_IS_NULLABLE);
+ } else if (key == IPROTO_FIELD_IS_NULLABLE) {
bool is_nullable = mp_decode_bool(data);
lua_pushboolean(L, is_nullable);
lua_setfield(L, -2, "is_nullable");
+ } else {
+ assert(key == IPROTO_FIELD_IS_AUTOINCREMENT);
+ bool autoincrement = mp_decode_bool(data);
+ lua_pushboolean(L, autoincrement);
+ lua_setfield(L, -2, "is_autoincrement");
}
}
}
@@ -672,7 +676,7 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
lua_createtable(L, count, 0);
for (uint32_t i = 0; i < count; ++i) {
uint32_t map_size = mp_decode_map(data);
- assert(map_size >= 2 && map_size <= 4);
+ assert(map_size >= 2 && map_size <= 5);
uint32_t key = mp_decode_uint(data);
assert(key == IPROTO_FIELD_NAME);
(void) key;
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 73ce95eba..d92da4d8e 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1845,6 +1845,12 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
space_def->fields[iCol].is_nullable;
vdbe_metadata_set_col_nullability(v, i,
is_nullable);
+ if (pTabList->a[j].space->sequence != NULL) {
+ int afno =
+ pTabList->a[j].space->sequence_fieldno;
+ if (afno == iCol)
+ vdbe_metadata_set_col_autoincrement(v, i);
+ }
}
} else {
const char *z = NULL;
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 3698a5942..e248bc673 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -582,6 +582,9 @@ sql_column_coll(sql_stmt *stmt, int n);
int
sql_column_nullable(sql_stmt *stmt, int n);
+bool
+sql_column_is_autoincrement(sql_stmt *stmt, int n);
+
int
sql_initialize(void);
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 384d7dc0a..da9a311b8 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -260,6 +260,9 @@ vdbe_metadata_set_col_collation(struct Vdbe *p, int idx, const char *coll,
void
vdbe_metadata_set_col_nullability(struct Vdbe *p, int idx, int nullable);
+void
+vdbe_metadata_set_col_autoincrement(struct Vdbe *p, int idx);
+
void sqlVdbeCountChanges(Vdbe *);
sql *sqlVdbeDb(Vdbe *);
void sqlVdbeSetSql(Vdbe *, const char *z, int n, int);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 92a50dd7b..d79635b14 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -355,6 +355,8 @@ struct sql_column_metadata {
* columns: all other expressions are nullable by default.
*/
int8_t nullable : 2;
+ /** True if column features autoincrement property. */
+ bool is_actoincrement;
};
/*
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 30f173984..5b423c9df 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -761,6 +761,14 @@ sql_column_nullable(sql_stmt *stmt, int n)
return p->metadata[n].nullable;
}
+bool
+sql_column_is_autoincrement(sql_stmt *stmt, int n)
+{
+ struct Vdbe *p = (struct Vdbe *) stmt;
+ assert(n < sql_column_count(stmt) && n >= 0);
+ return p->metadata[n].is_actoincrement;
+}
+
/******************************* sql_bind_ **************************
*
* Routines used to attach values to wildcards in a compiled SQL statement.
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 2de41a70a..e3672097c 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1911,6 +1911,13 @@ vdbe_metadata_set_col_nullability(struct Vdbe *p, int idx, int nullable)
p->metadata[idx].nullable = nullable;
}
+void
+vdbe_metadata_set_col_autoincrement(struct Vdbe *p, int idx)
+{
+ assert(idx < p->nResColumn);
+ p->metadata[idx].is_actoincrement = 1;
+}
+
/*
* This routine checks that the sql.nVdbeActive count variable
* matches the number of vdbe's in the list sql.pVdbe that are
diff --git a/test/sql/full_metadata.result b/test/sql/full_metadata.result
index 20ed29e1c..7c2982682 100644
--- a/test/sql/full_metadata.result
+++ b/test/sql/full_metadata.result
@@ -81,12 +81,13 @@ execute("SELECT c COLLATE \"unicode\" FROM t;")
| - ['aSd']
| ...
--- Make sure that nullability is presented.
+-- Make sure that nullability/autoincrement are presented.
--
execute("SELECT id, a, c FROM t;")
| ---
| - metadata:
| - type: integer
+ | is_autoincrement: true
| name: ID
| is_nullable: false
| - type: integer
@@ -103,6 +104,7 @@ execute("SELECT * FROM t;")
| ---
| - metadata:
| - type: integer
+ | is_autoincrement: true
| name: ID
| is_nullable: false
| - type: integer
diff --git a/test/sql/full_metadata.test.lua b/test/sql/full_metadata.test.lua
index 0ff5fefe6..e6bfa1eaf 100644
--- a/test/sql/full_metadata.test.lua
+++ b/test/sql/full_metadata.test.lua
@@ -30,7 +30,7 @@ execute("SELECT 'aSd' COLLATE \"unicode_ci\";")
execute("SELECT c FROM t;")
execute("SELECT c COLLATE \"unicode\" FROM t;")
--- Make sure that nullability is presented.
+-- Make sure that nullability/autoincrement are presented.
--
execute("SELECT id, a, c FROM t;")
execute("SELECT * FROM t;")
--
2.15.1
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 5/6] sql: extend result set with autoincrement
2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 5/6] sql: extend result set with autoincrement Nikita Pettik
@ 2019-12-18 0:29 ` Vladislav Shpilevoy
2019-12-24 0:26 ` Nikita Pettik
0 siblings, 1 reply; 25+ messages in thread
From: Vladislav Shpilevoy @ 2019-12-18 0:29 UTC (permalink / raw)
To: Nikita Pettik, tarantool-patches
Thanks for the patch!
See 3 comments below.
On 11/12/2019 14:44, Nikita Pettik wrote:
> If result set contains column which features attached sequence
> (AUTOINCREMENT in terms of SQL) then meta-information will contain
> corresponding field ('is_autoicrement' : boolean) in response.
>
> Part of #4407
> ---
> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 644b373c9..88ef4ff78 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -651,11 +651,15 @@ decode_metadata_optional(struct lua_State *L, const char **data,
> const char *coll = mp_decode_str(data, &len);
> lua_pushlstring(L, coll, len);
> lua_setfield(L, -2, "collation");
> - } else {
> - assert(key == IPROTO_FIELD_IS_NULLABLE);
> + } else if (key == IPROTO_FIELD_IS_NULLABLE) {
> bool is_nullable = mp_decode_bool(data);
> lua_pushboolean(L, is_nullable);
> lua_setfield(L, -2, "is_nullable");
> + } else {
> + assert(key == IPROTO_FIELD_IS_AUTOINCREMENT);
> + bool autoincrement = mp_decode_bool(data);
1. autoincrement -> is_autoincrement.
> + lua_pushboolean(L, autoincrement);
> + lua_setfield(L, -2, "is_autoincrement");
> }
> }
> }
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 73ce95eba..d92da4d8e 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1845,6 +1845,12 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> space_def->fields[iCol].is_nullable;
> vdbe_metadata_set_col_nullability(v, i,
> is_nullable);
> + if (pTabList->a[j].space->sequence != NULL) {
> + int afno =
> + pTabList->a[j].space->sequence_fieldno;
> + if (afno == iCol)
> + vdbe_metadata_set_col_autoincrement(v, i);
> + }
> }
> } else {
> const char *z = NULL;
2. Consider this refactoring to make it more accurate:
============================================================================
@@ -1822,7 +1822,8 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
break;
}
assert(j < pTabList->nSrc);
- struct space_def *space_def = pTabList->a[j].space->def;
+ struct space *space = pTabList->a[j].space;
+ struct space_def *space_def = space->def;
assert(iCol >= 0 && iCol < (int)space_def->field_count);
zCol = space_def->fields[iCol].name;
const char *name = NULL;
@@ -1845,12 +1846,9 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
space_def->fields[iCol].is_nullable;
vdbe_metadata_set_col_nullability(v, i,
is_nullable);
- if (pTabList->a[j].space->sequence != NULL) {
- int afno =
- pTabList->a[j].space->sequence_fieldno;
- if (afno == iCol)
- vdbe_metadata_set_col_autoincrement(v, i);
- }
+ if (space->sequence != NULL &&
+ space->sequence_fieldno == iCol)
+ vdbe_metadata_set_col_autoincrement(v, i);
}
} else {
const char *z = NULL;
============================================================================
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index 2de41a70a..e3672097c 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -1911,6 +1911,13 @@ vdbe_metadata_set_col_nullability(struct Vdbe *p, int idx, int nullable)
> p->metadata[idx].nullable = nullable;
> }
>
> +void
> +vdbe_metadata_set_col_autoincrement(struct Vdbe *p, int idx)
> +{
> + assert(idx < p->nResColumn);
> + p->metadata[idx].is_actoincrement = 1;
3. Why 1 instead of true?
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 5/6] sql: extend result set with autoincrement
2019-12-18 0:29 ` Vladislav Shpilevoy
@ 2019-12-24 0:26 ` Nikita Pettik
2019-12-24 15:30 ` Vladislav Shpilevoy
0 siblings, 1 reply; 25+ messages in thread
From: Nikita Pettik @ 2019-12-24 0:26 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On 18 Dec 01:29, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 3 comments below.
>
> On 11/12/2019 14:44, Nikita Pettik wrote:
> > If result set contains column which features attached sequence
> > (AUTOINCREMENT in terms of SQL) then meta-information will contain
> > corresponding field ('is_autoicrement' : boolean) in response.
> >
> > Part of #4407
> > ---
> > diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> > index 644b373c9..88ef4ff78 100644
> > --- a/src/box/lua/net_box.c
> > +++ b/src/box/lua/net_box.c
> > @@ -651,11 +651,15 @@ decode_metadata_optional(struct lua_State *L, const char **data,
> > const char *coll = mp_decode_str(data, &len);
> > lua_pushlstring(L, coll, len);
> > lua_setfield(L, -2, "collation");
> > - } else {
> > - assert(key == IPROTO_FIELD_IS_NULLABLE);
> > + } else if (key == IPROTO_FIELD_IS_NULLABLE) {
> > bool is_nullable = mp_decode_bool(data);
> > lua_pushboolean(L, is_nullable);
> > lua_setfield(L, -2, "is_nullable");
> > + } else {
> > + assert(key == IPROTO_FIELD_IS_AUTOINCREMENT);
> > + bool autoincrement = mp_decode_bool(data);
>
> 1. autoincrement -> is_autoincrement.
Fixed.
> > diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> > index 73ce95eba..d92da4d8e 100644
> > --- a/src/box/sql/select.c
> > +++ b/src/box/sql/select.c
> > @@ -1845,6 +1845,12 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> > space_def->fields[iCol].is_nullable;
> > vdbe_metadata_set_col_nullability(v, i,
> > is_nullable);
> > + if (pTabList->a[j].space->sequence != NULL) {
> > + int afno =
> > + pTabList->a[j].space->sequence_fieldno;
> > + if (afno == iCol)
> > + vdbe_metadata_set_col_autoincrement(v, i);
> > + }
> > }
> > } else {
> > const char *z = NULL;
>
> 2. Consider this refactoring to make it more accurate:
>
> ============================================================================
>
> @@ -1822,7 +1822,8 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> break;
> }
> assert(j < pTabList->nSrc);
> - struct space_def *space_def = pTabList->a[j].space->def;
> + struct space *space = pTabList->a[j].space;
> + struct space_def *space_def = space->def;
> assert(iCol >= 0 && iCol < (int)space_def->field_count);
> zCol = space_def->fields[iCol].name;
> const char *name = NULL;
> @@ -1845,12 +1846,9 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> space_def->fields[iCol].is_nullable;
> vdbe_metadata_set_col_nullability(v, i,
> is_nullable);
> - if (pTabList->a[j].space->sequence != NULL) {
> - int afno =
> - pTabList->a[j].space->sequence_fieldno;
> - if (afno == iCol)
> - vdbe_metadata_set_col_autoincrement(v, i);
> - }
> + if (space->sequence != NULL &&
> + space->sequence_fieldno == iCol)
> + vdbe_metadata_set_col_autoincrement(v, i);
> }
> } else {
> const char *z = NULL;
>
Applied.
> ============================================================================
>
> > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> > index 2de41a70a..e3672097c 100644
> > --- a/src/box/sql/vdbeaux.c
> > +++ b/src/box/sql/vdbeaux.c
> > @@ -1911,6 +1911,13 @@ vdbe_metadata_set_col_nullability(struct Vdbe *p, int idx, int nullable)
> > p->metadata[idx].nullable = nullable;
> > }
> >
> > +void
> > +vdbe_metadata_set_col_autoincrement(struct Vdbe *p, int idx)
> > +{
> > + assert(idx < p->nResColumn);
> > + p->metadata[idx].is_actoincrement = 1;
>
> 3. Why 1 instead of true?
Idk, replaced with boolean.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 5/6] sql: extend result set with autoincrement
2019-12-24 0:26 ` Nikita Pettik
@ 2019-12-24 15:30 ` Vladislav Shpilevoy
2019-12-25 12:17 ` Nikita Pettik
0 siblings, 1 reply; 25+ messages in thread
From: Vladislav Shpilevoy @ 2019-12-24 15:30 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
Thanks for the patch!
>> @@ -1845,12 +1846,9 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
>> space_def->fields[iCol].is_nullable;
>> vdbe_metadata_set_col_nullability(v, i,
>> is_nullable);
>> - if (pTabList->a[j].space->sequence != NULL) {
>> - int afno =
>> - pTabList->a[j].space->sequence_fieldno;
>> - if (afno == iCol)
>> - vdbe_metadata_set_col_autoincrement(v, i);
>> - }
>> + if (space->sequence != NULL &&
>> + space->sequence_fieldno == iCol)
>> + vdbe_metadata_set_col_autoincrement(v, i);
Sorry, I know that I proposed this, but now I am getting a compilation
error here:
tarantool/src/box/sql/select.c:1850:33: error: comparison of integers of different signs: 'uint32_t' (aka 'unsigned int') and 'int' [-Werror,-Wsign-compare]
space->sequence_fieldno == iCol)
I don't know why was not it raised earlier.
I've pushed my review fix on top of this commit. See it below
and on the branch.
================================================================================
commit 53fd4b3b3a27cf99c6c7a71a085574b2fd8a0dc7
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date: Tue Dec 24 15:56:24 2019 +0100
Review fix 5/6
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index ddb2509f4..a19494ed9 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1847,7 +1847,7 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
vdbe_metadata_set_col_nullability(v, i,
is_nullable);
if (space->sequence != NULL &&
- space->sequence_fieldno == iCol)
+ space->sequence_fieldno == (uint32_t) iCol)
vdbe_metadata_set_col_autoincrement(v, i);
}
} else {
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 5/6] sql: extend result set with autoincrement
2019-12-24 15:30 ` Vladislav Shpilevoy
@ 2019-12-25 12:17 ` Nikita Pettik
2019-12-25 15:40 ` Vladislav Shpilevoy
0 siblings, 1 reply; 25+ messages in thread
From: Nikita Pettik @ 2019-12-25 12:17 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On 24 Dec 16:30, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> I've pushed my review fix on top of this commit. See it below
> and on the branch.
Thanks, I've squashed your review fixes. Are the rest of patches
(except last one) LGTM? If so, I'd like to push them to focus on
the last.
> commit 53fd4b3b3a27cf99c6c7a71a085574b2fd8a0dc7
> Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Date: Tue Dec 24 15:56:24 2019 +0100
>
> Review fix 5/6
>
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index ddb2509f4..a19494ed9 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1847,7 +1847,7 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> vdbe_metadata_set_col_nullability(v, i,
> is_nullable);
> if (space->sequence != NULL &&
> - space->sequence_fieldno == iCol)
> + space->sequence_fieldno == (uint32_t) iCol)
> vdbe_metadata_set_col_autoincrement(v, i);
> }
> } else {
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 5/6] sql: extend result set with autoincrement
2019-12-25 12:17 ` Nikita Pettik
@ 2019-12-25 15:40 ` Vladislav Shpilevoy
2019-12-25 23:18 ` Nikita Pettik
0 siblings, 1 reply; 25+ messages in thread
From: Vladislav Shpilevoy @ 2019-12-25 15:40 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
Hi!
Yes, first 5 patches are good to push.
On 25/12/2019 13:17, Nikita Pettik wrote:
> On 24 Dec 16:30, Vladislav Shpilevoy wrote:
>> Thanks for the patch!
>>
>> I've pushed my review fix on top of this commit. See it below
>> and on the branch.
>
> Thanks, I've squashed your review fixes. Are the rest of patches
> (except last one) LGTM? If so, I'd like to push them to focus on
> the last.
>
>> commit 53fd4b3b3a27cf99c6c7a71a085574b2fd8a0dc7
>> Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
>> Date: Tue Dec 24 15:56:24 2019 +0100
>>
>> Review fix 5/6
>>
>> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
>> index ddb2509f4..a19494ed9 100644
>> --- a/src/box/sql/select.c
>> +++ b/src/box/sql/select.c
>> @@ -1847,7 +1847,7 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
>> vdbe_metadata_set_col_nullability(v, i,
>> is_nullable);
>> if (space->sequence != NULL &&
>> - space->sequence_fieldno == iCol)
>> + space->sequence_fieldno == (uint32_t) iCol)
>> vdbe_metadata_set_col_autoincrement(v, i);
>> }
>> } else {
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 5/6] sql: extend result set with autoincrement
2019-12-25 15:40 ` Vladislav Shpilevoy
@ 2019-12-25 23:18 ` Nikita Pettik
0 siblings, 0 replies; 25+ messages in thread
From: Nikita Pettik @ 2019-12-25 23:18 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On 25 Dec 16:40, Vladislav Shpilevoy wrote:
> Hi!
>
> Yes, first 5 patches are good to push.
Pushed them to master.
> On 25/12/2019 13:17, Nikita Pettik wrote:
> > On 24 Dec 16:30, Vladislav Shpilevoy wrote:
> >> Thanks for the patch!
> >>
> >> I've pushed my review fix on top of this commit. See it below
> >> and on the branch.
> >
> > Thanks, I've squashed your review fixes. Are the rest of patches
> > (except last one) LGTM? If so, I'd like to push them to focus on
> > the last.
> >
> >> commit 53fd4b3b3a27cf99c6c7a71a085574b2fd8a0dc7
> >> Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> >> Date: Tue Dec 24 15:56:24 2019 +0100
> >>
> >> Review fix 5/6
> >>
> >> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> >> index ddb2509f4..a19494ed9 100644
> >> --- a/src/box/sql/select.c
> >> +++ b/src/box/sql/select.c
> >> @@ -1847,7 +1847,7 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> >> vdbe_metadata_set_col_nullability(v, i,
> >> is_nullable);
> >> if (space->sequence != NULL &&
> >> - space->sequence_fieldno == iCol)
> >> + space->sequence_fieldno == (uint32_t) iCol)
> >> vdbe_metadata_set_col_autoincrement(v, i);
> >> }
> >> } else {
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Tarantool-patches] [PATCH v2 6/6] sql: extend result set with alias
[not found] ` <cover.1576071711.git.korablev@tarantool.org>
` (4 preceding siblings ...)
2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 5/6] sql: extend result set with autoincrement Nikita Pettik
@ 2019-12-11 13:44 ` Nikita Pettik
2019-12-18 0:29 ` Vladislav Shpilevoy
5 siblings, 1 reply; 25+ messages in thread
From: Nikita Pettik @ 2019-12-11 13:44 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
Each column of result set can feature its name alias. For instance:
SELECT x + 1 AS add FROM ...;
In this case real name of resulting set column is "x + 1" meanwhile
"add" is its alias. This patch extends metadata with optional metadata
member which corresponds to column's alias.
Closes #4407
@TarantoolBot document
Title: extended SQL metadata
Before this patch metadata for SQL DQL contained only two fields:
name and type of each column of result set. Now it may contain
following properties:
- collation (in case type of resulting set column is string and
collation is different from default "none");
is encoded with IPROTO_FIELD_COLL (0x2) key in IPROTO_METADATA map;
in msgpack is encoded as string and held with MP_STR type;
- is_nullable (in case column of result set corresponds to space's
field; for expressions like x+1 for the sake of
simplicity nullability is omitted);
is encoded with IPROTO_FIELD_IS_NULLABLE key (0x3) in IPROTO_METADATA;
in msgpack is encoded as boolean and held with MP_BOOL type;
note that absence of this field implies that nullability is unknown;
- is_autoincrement (is set only for autoincrement column in result
set);
is encoded with IPROTO_FIELD_IS_AUNTOINCREMENT (0x4) key in IPROTO_METADATA;
in msgpack is encoded as boolean and held with MP_BOOL type;
- alias (if column of result set is specified with AS label);
is encoded with IPROTO_FIELD_ALIAS (0x5) key in IPROTO_METADATA map;
in msgpack is encoded as string and held with MP_STR type;
note that if there's no
This extended metadata is send only when PRAGMA full_metadata is
enabled. Otherwise, only basic (name and type) metadata is processed.
---
src/box/execute.c | 18 ++++++++++++++----
src/box/iproto_constants.h | 1 +
src/box/lua/execute.c | 9 +++++++--
src/box/lua/net_box.c | 6 +++++-
src/box/sql/select.c | 32 ++++++++++++++++++++++++++++----
src/box/sql/sqlInt.h | 3 +++
src/box/sql/vdbe.h | 3 +++
src/box/sql/vdbeInt.h | 1 +
src/box/sql/vdbeapi.c | 8 ++++++++
src/box/sql/vdbeaux.c | 15 +++++++++++++++
test/sql/full_metadata.result | 38 +++++++++++++++++++++++++++++++++++---
test/sql/full_metadata.test.lua | 7 +++++++
12 files changed, 127 insertions(+), 14 deletions(-)
diff --git a/src/box/execute.c b/src/box/execute.c
index c853991a0..0fe98af3d 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -270,7 +270,7 @@ error:
static size_t
metadata_map_sizeof(const char *name, const char *type, const char *coll,
- int nullable, bool is_autoincrement)
+ const char *alias, int nullable, bool is_autoincrement)
{
uint32_t members_count = 2;
size_t map_size = 0;
@@ -279,6 +279,11 @@ metadata_map_sizeof(const char *name, const char *type, const char *coll,
map_size += mp_sizeof_uint(IPROTO_FIELD_COLL);
map_size += mp_sizeof_str(strlen(coll));
}
+ if (alias != NULL) {
+ members_count++;
+ map_size += mp_sizeof_uint(IPROTO_FIELD_ALIAS);
+ map_size += mp_sizeof_str(strlen(alias));
+ }
if (nullable != -1) {
members_count++;
map_size += mp_sizeof_uint(IPROTO_FIELD_IS_NULLABLE);
@@ -323,6 +328,7 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
const char *coll = sql_column_coll(stmt, i);
const char *name = sql_column_name(stmt, i);
const char *type = sql_column_datatype(stmt, i);
+ const char *alias = sql_column_alias(stmt, i);
int nullable = sql_column_nullable(stmt, i);
bool is_autoincrement = sql_column_is_autoincrement(stmt, i);
/*
@@ -332,15 +338,15 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
*/
assert(name != NULL);
assert(type != NULL);
- size = metadata_map_sizeof(name, type, coll, nullable,
+ size = metadata_map_sizeof(name, type, coll, alias, nullable,
is_autoincrement);
char *pos = (char *) obuf_alloc(out, size);
if (pos == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "pos");
return -1;
}
- uint32_t map_sz = 2 + (coll != NULL) + (nullable != -1) +
- is_autoincrement;
+ uint32_t map_sz = 2 + (coll != NULL) + (alias != NULL) +
+ (nullable != -1) + is_autoincrement;
pos = mp_encode_map(pos, map_sz);
pos = mp_encode_uint(pos, IPROTO_FIELD_NAME);
pos = mp_encode_str(pos, name, strlen(name));
@@ -350,6 +356,10 @@ sql_get_metadata(struct sql_stmt *stmt, struct obuf *out, int column_count)
pos = mp_encode_uint(pos, IPROTO_FIELD_COLL);
pos = mp_encode_str(pos, coll, strlen(coll));
}
+ if (alias != NULL) {
+ pos = mp_encode_uint(pos, IPROTO_FIELD_ALIAS);
+ pos = mp_encode_str(pos, alias, strlen(alias));
+ }
if (nullable != -1) {
pos = mp_encode_uint(pos, IPROTO_FIELD_IS_NULLABLE);
pos = mp_encode_bool(pos, nullable);
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index 30d1af4cb..8e50e0cb1 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -134,6 +134,7 @@ enum iproto_metadata_key {
IPROTO_FIELD_COLL = 2,
IPROTO_FIELD_IS_NULLABLE = 3,
IPROTO_FIELD_IS_AUTOINCREMENT = 4,
+ IPROTO_FIELD_ALIAS = 5,
};
enum iproto_ballot_key {
diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index e8e3e2a9f..9055d3a76 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -23,10 +23,11 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L,
const char *coll = sql_column_coll(stmt, i);
const char *name = sql_column_name(stmt, i);
const char *type = sql_column_datatype(stmt, i);
+ const char *alias = sql_column_alias(stmt, i);
int nullable = sql_column_nullable(stmt, i);
bool is_autoincrement = sql_column_is_autoincrement(stmt, i);
- size_t table_sz = 2 + (coll != NULL) + (nullable != -1) +
- is_autoincrement;
+ size_t table_sz = 2 + (coll != NULL) + (alias != NULL) +
+ (nullable != -1) + is_autoincrement ;
lua_createtable(L, 0, table_sz);
/*
* Can not fail, since all column names are
@@ -51,6 +52,10 @@ lua_sql_get_metadata(struct sql_stmt *stmt, struct lua_State *L,
lua_pushboolean(L, true);
lua_setfield(L, -2, "is_autoincrement");
}
+ if (alias != NULL) {
+ lua_pushstring(L, alias);
+ lua_setfield(L, -2, "alias");
+ }
lua_rawseti(L, -2, i + 1);
}
}
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 88ef4ff78..1bfe08f64 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -655,6 +655,10 @@ decode_metadata_optional(struct lua_State *L, const char **data,
bool is_nullable = mp_decode_bool(data);
lua_pushboolean(L, is_nullable);
lua_setfield(L, -2, "is_nullable");
+ } else if (key == IPROTO_FIELD_ALIAS) {
+ const char *alias = mp_decode_str(data, &len);
+ lua_pushlstring(L, alias, len);
+ lua_setfield(L, -2, "alias");
} else {
assert(key == IPROTO_FIELD_IS_AUTOINCREMENT);
bool autoincrement = mp_decode_bool(data);
@@ -676,7 +680,7 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
lua_createtable(L, count, 0);
for (uint32_t i = 0; i < count; ++i) {
uint32_t map_size = mp_decode_map(data);
- assert(map_size >= 2 && map_size <= 5);
+ assert(map_size >= 2 && map_size <= 6);
uint32_t key = mp_decode_uint(data);
assert(key == IPROTO_FIELD_NAME);
(void) key;
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index d92da4d8e..c1770e7b4 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1827,7 +1827,19 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
zCol = space_def->fields[iCol].name;
const char *name = NULL;
if (pEList->a[i].zName != NULL) {
- name = pEList->a[i].zName;
+ if (is_full_meta) {
+ const char *alias = NULL;
+ if (pEList->a[i].zSpan != NULL) {
+ alias = pEList->a[i].zName;
+ name = pEList->a[i].zSpan;
+ } else {
+ alias = pEList->a[i].zName;
+ name = pEList->a[i].zName;
+ }
+ vdbe_metadata_set_col_alias(v, i, alias);
+ } else {
+ name = pEList->a[i].zName;
+ }
} else {
if (!shortNames && !fullNames) {
name = pEList->a[i].zSpan;
@@ -1854,9 +1866,21 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
}
} else {
const char *z = NULL;
- if (pEList->a[i].zName != NULL)
- z = pEList->a[i].zName;
- else if (pEList->a[i].zSpan != NULL)
+ if (pEList->a[i].zName != NULL) {
+ if (is_full_meta ) {
+ const char *alias = NULL;
+ if (pEList->a[i].zSpan != NULL) {
+ alias = pEList->a[i].zName;
+ z = pEList->a[i].zSpan;
+ } else {
+ alias = pEList->a[i].zName;
+ z = pEList->a[i].zName;
+ }
+ vdbe_metadata_set_col_alias(v, i, alias);
+ } else {
+ z = pEList->a[i].zName;
+ }
+ } else if (pEList->a[i].zSpan != NULL)
z = pEList->a[i].zSpan;
else
z = tt_sprintf("column%d", i + 1);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index e248bc673..b38b66b74 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -585,6 +585,9 @@ sql_column_nullable(sql_stmt *stmt, int n);
bool
sql_column_is_autoincrement(sql_stmt *stmt, int n);
+const char *
+sql_column_alias(sql_stmt *stmt, int n);
+
int
sql_initialize(void);
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index da9a311b8..f35f11241 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -263,6 +263,9 @@ vdbe_metadata_set_col_nullability(struct Vdbe *p, int idx, int nullable);
void
vdbe_metadata_set_col_autoincrement(struct Vdbe *p, int idx);
+int
+vdbe_metadata_set_col_alias(struct Vdbe *p, int idx, const char *alias);
+
void sqlVdbeCountChanges(Vdbe *);
sql *sqlVdbeDb(Vdbe *);
void sqlVdbeSetSql(Vdbe *, const char *z, int n, int);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index d79635b14..eaa40a35e 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -350,6 +350,7 @@ struct sql_column_metadata {
char *name;
char *type;
char *collation;
+ char *alias;
/**
* -1 is for any member of result set except for pure
* columns: all other expressions are nullable by default.
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 5b423c9df..47cd068ff 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -769,6 +769,14 @@ sql_column_is_autoincrement(sql_stmt *stmt, int n)
return p->metadata[n].is_actoincrement;
}
+const char *
+sql_column_alias(sql_stmt *stmt, int n)
+{
+ struct Vdbe *p = (struct Vdbe *) stmt;
+ assert(n < sql_column_count(stmt) && n >= 0);
+ return p->metadata[n].alias;
+}
+
/******************************* sql_bind_ **************************
*
* Routines used to attach values to wildcards in a compiled SQL statement.
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index e3672097c..625464902 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1835,6 +1835,7 @@ vdbe_metadata_delete(struct Vdbe *v)
free(v->metadata[i].name);
free(v->metadata[i].type);
free(v->metadata[i].collation);
+ free(v->metadata[i].alias);
}
free(v->metadata);
}
@@ -1918,6 +1919,20 @@ vdbe_metadata_set_col_autoincrement(struct Vdbe *p, int idx)
p->metadata[idx].is_actoincrement = 1;
}
+int
+vdbe_metadata_set_col_alias(struct Vdbe *p, int idx, const char *alias)
+{
+ assert(idx < p->nResColumn);
+ if (p->metadata[idx].alias != NULL)
+ free((void *)p->metadata[idx].alias);
+ p->metadata[idx].alias = strdup(alias);
+ if (p->metadata[idx].alias == NULL) {
+ diag_set(OutOfMemory, strlen(alias) + 1, "strdup", "alias");
+ return -1;
+ }
+ return 0;
+}
+
/*
* This routine checks that the sql.nVdbeActive count variable
* matches the number of vdbe's in the list sql.pVdbe that are
diff --git a/test/sql/full_metadata.result b/test/sql/full_metadata.result
index 7c2982682..8f0bced36 100644
--- a/test/sql/full_metadata.result
+++ b/test/sql/full_metadata.result
@@ -103,14 +103,17 @@ execute("SELECT id, a, c FROM t;")
execute("SELECT * FROM t;")
| ---
| - metadata:
- | - type: integer
+ | - alias: ID
+ | type: integer
| is_autoincrement: true
| name: ID
| is_nullable: false
| - type: integer
- | name: A
| is_nullable: false
- | - type: string
+ | name: A
+ | alias: A
+ | - alias: C
+ | type: string
| is_nullable: true
| name: C
| collation: unicode_ci
@@ -118,6 +121,35 @@ execute("SELECT * FROM t;")
| - [1, 1, 'aSd']
| ...
+-- Alias is always set in extended metadata. If column label in
+-- form of AS clause is set, then this alias is presented in
+-- metadata. Otherwise, alias is just the same as name.
+--
+execute("SELECT 1 AS x;")
+ | ---
+ | - metadata:
+ | - type: integer
+ | name: '1'
+ | alias: X
+ | rows:
+ | - [1]
+ | ...
+execute("SELECT a AS a_label, c AS c_label FROM t;")
+ | ---
+ | - metadata:
+ | - type: integer
+ | is_nullable: false
+ | name: a
+ | alias: A_LABEL
+ | - alias: C_LABEL
+ | type: string
+ | is_nullable: true
+ | name: c
+ | collation: unicode_ci
+ | rows:
+ | - [1, 'aSd']
+ | ...
+
execute("PRAGMA full_metadata = false;")
| ---
| - row_count: 0
diff --git a/test/sql/full_metadata.test.lua b/test/sql/full_metadata.test.lua
index e6bfa1eaf..533fa90c7 100644
--- a/test/sql/full_metadata.test.lua
+++ b/test/sql/full_metadata.test.lua
@@ -35,6 +35,13 @@ execute("SELECT c COLLATE \"unicode\" FROM t;")
execute("SELECT id, a, c FROM t;")
execute("SELECT * FROM t;")
+-- Alias is always set in extended metadata. If column label in
+-- form of AS clause is set, then this alias is presented in
+-- metadata. Otherwise, alias is just the same as name.
+--
+execute("SELECT 1 AS x;")
+execute("SELECT a AS a_label, c AS c_label FROM t;")
+
execute("PRAGMA full_metadata = false;")
test_run:cmd("setopt delimiter ';'")
--
2.15.1
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 6/6] sql: extend result set with alias
2019-12-11 13:44 ` [Tarantool-patches] [PATCH v2 6/6] sql: extend result set with alias Nikita Pettik
@ 2019-12-18 0:29 ` Vladislav Shpilevoy
2019-12-24 0:26 ` Nikita Pettik
0 siblings, 1 reply; 25+ messages in thread
From: Vladislav Shpilevoy @ 2019-12-18 0:29 UTC (permalink / raw)
To: Nikita Pettik, tarantool-patches
Thanks for the patch!
See 4 comments below.
On 11/12/2019 14:44, Nikita Pettik wrote:
> Each column of result set can feature its name alias. For instance:
>
> SELECT x + 1 AS add FROM ...;
>
> In this case real name of resulting set column is "x + 1" meanwhile
> "add" is its alias. This patch extends metadata with optional metadata
> member which corresponds to column's alias.
>
> Closes #4407
>
> @TarantoolBot document
> Title: extended SQL metadata
>
> Before this patch metadata for SQL DQL contained only two fields:
> name and type of each column of result set. Now it may contain
> following properties:
> - collation (in case type of resulting set column is string and
> collation is different from default "none");
> is encoded with IPROTO_FIELD_COLL (0x2) key in IPROTO_METADATA map;
> in msgpack is encoded as string and held with MP_STR type;
> - is_nullable (in case column of result set corresponds to space's
> field; for expressions like x+1 for the sake of
> simplicity nullability is omitted);
> is encoded with IPROTO_FIELD_IS_NULLABLE key (0x3) in IPROTO_METADATA;
> in msgpack is encoded as boolean and held with MP_BOOL type;
> note that absence of this field implies that nullability is unknown;
> - is_autoincrement (is set only for autoincrement column in result
> set);
> is encoded with IPROTO_FIELD_IS_AUNTOINCREMENT (0x4) key in IPROTO_METADATA;
> in msgpack is encoded as boolean and held with MP_BOOL type;
> - alias (if column of result set is specified with AS label);
> is encoded with IPROTO_FIELD_ALIAS (0x5) key in IPROTO_METADATA map;
> in msgpack is encoded as string and held with MP_STR type;
> note that if there's no
>
> This extended metadata is send only when PRAGMA full_metadata is
> enabled. Otherwise, only basic (name and type) metadata is processed.
1. You didn't mention, that the PRAGMA not only regulates what
metadata to return, but also changes 'name' field behaviour:
box.execute('PRAGMA full_metadata = true;')
tarantool> box.execute('SELECT 1 AS label')
---
- metadata:
- type: integer
name: '1'
alias: LABEL
rows:
- [1]
...
box.execute('PRAGMA full_metadata = false;')
tarantool> box.execute('SELECT 1 AS label')
---
- metadata:
- name: LABEL
type: integer
rows:
- [1]
...
I don't think it is ok though. Not the missing doc, but the
not stable 'name'. It is one another reason to keep the 'name'
as is and return the original expression span in a new column.
'Orig_name', 'oname', 'span', 'column', 'expr', etc.
> ---
> src/box/execute.c | 18 ++++++++++++++----
> src/box/iproto_constants.h | 1 +
> src/box/lua/execute.c | 9 +++++++--
> src/box/lua/net_box.c | 6 +++++-
> src/box/sql/select.c | 32 ++++++++++++++++++++++++++++----
> src/box/sql/sqlInt.h | 3 +++
> src/box/sql/vdbe.h | 3 +++
> src/box/sql/vdbeInt.h | 1 +
> src/box/sql/vdbeapi.c | 8 ++++++++
> src/box/sql/vdbeaux.c | 15 +++++++++++++++
> test/sql/full_metadata.result | 38 +++++++++++++++++++++++++++++++++++---
> test/sql/full_metadata.test.lua | 7 +++++++
> 12 files changed, 127 insertions(+), 14 deletions(-)
> > diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index d92da4d8e..c1770e7b4 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1827,7 +1827,19 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> zCol = space_def->fields[iCol].name;
> const char *name = NULL;
> if (pEList->a[i].zName != NULL) {
> - name = pEList->a[i].zName;
> + if (is_full_meta) {
> + const char *alias = NULL;
> + if (pEList->a[i].zSpan != NULL) {
> + alias = pEList->a[i].zName;
> + name = pEList->a[i].zSpan;
> + } else {
> + alias = pEList->a[i].zName;
> + name = pEList->a[i].zName;
2. What is a point of assigning alias to the same value
in both branches of 'if'? Couldn't you do it out of
the 'if' and drop {} ? The same below.
> + }
> + vdbe_metadata_set_col_alias(v, i, alias);
> + } else {
> + name = pEList->a[i].zName;
> + }
> } else {
> if (!shortNames && !fullNames) {
> name = pEList->a[i].zSpan;
> @@ -1854,9 +1866,21 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> }
> } else {
> const char *z = NULL;
> - if (pEList->a[i].zName != NULL)
> - z = pEList->a[i].zName;
> - else if (pEList->a[i].zSpan != NULL)
> + if (pEList->a[i].zName != NULL) {
> + if (is_full_meta ) {
3. Extra whitespace after is_full_meta.
> + const char *alias = NULL;
> + if (pEList->a[i].zSpan != NULL) {
> + alias = pEList->a[i].zName;
> + z = pEList->a[i].zSpan;
> + } else {
> + alias = pEList->a[i].zName;
> + z = pEList->a[i].zName;
> + }
> + vdbe_metadata_set_col_alias(v, i, alias);
> + } else {
> + z = pEList->a[i].zName;
> + }
> + } else if (pEList->a[i].zSpan != NULL)
> z = pEList->a[i].zSpan;
> else
> z = tt_sprintf("column%d", i + 1);
> diff --git a/test/sql/full_metadata.result b/test/sql/full_metadata.result
> index 7c2982682..8f0bced36 100644
> --- a/test/sql/full_metadata.result
> +++ b/test/sql/full_metadata.result
> @@ -118,6 +121,35 @@ execute("SELECT * FROM t;")
> | - [1, 1, 'aSd']
> | ...
>
> +-- Alias is always set in extended metadata. If column label in
> +-- form of AS clause is set, then this alias is presented in
> +-- metadata. Otherwise, alias is just the same as name.
4. But it is not true. The alias is not set always:
=========================================================
tarantool> box.execute('PRAGMA full_metadata = true;')
---
- row_count: 0
...
tarantool> box.execute('SELECT 1')
---
- metadata:
- name: '1'
type: integer
rows:
- [1]
...
=========================================================
As you can see, no AS = no alias. This is what I don't like
in this patch the most. A user does not have a reliable always
present column with a displayable name: either the column
span or its alias after AS. He can't use name, because it
changes its behavour depending on AS. And he can't use alias,
because sometimes it is not present. That leads to confusion,
and necessity to branch in user code a lot.
The same example shows, that alias is a bad name. Here you need
to return it, but it is not alias like AS. 'Label' would fit
better here.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 6/6] sql: extend result set with alias
2019-12-18 0:29 ` Vladislav Shpilevoy
@ 2019-12-24 0:26 ` Nikita Pettik
2019-12-24 15:34 ` Vladislav Shpilevoy
0 siblings, 1 reply; 25+ messages in thread
From: Nikita Pettik @ 2019-12-24 0:26 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On 18 Dec 01:29, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 4 comments below.
>
> 1. You didn't mention, that the PRAGMA not only regulates what
> metadata to return, but also changes 'name' field behaviour:
>
> box.execute('PRAGMA full_metadata = true;')
> tarantool> box.execute('SELECT 1 AS label')
> ---
> - metadata:
> - type: integer
> name: '1'
> alias: LABEL
> rows:
> - [1]
> ...
>
> box.execute('PRAGMA full_metadata = false;')
> tarantool> box.execute('SELECT 1 AS label')
> ---
> - metadata:
> - name: LABEL
> type: integer
> rows:
> - [1]
> ...
>
> I don't think it is ok though. Not the missing doc, but the
> not stable 'name'. It is one another reason to keep the 'name'
> as is and return the original expression span in a new column.
> 'Orig_name', 'oname', 'span', 'column', 'expr', etc.
But behaviour you suggesting contradicts all other DBs.
See my answer at the end of letter.
> > > diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> > index d92da4d8e..c1770e7b4 100644
> > --- a/src/box/sql/select.c
> > +++ b/src/box/sql/select.c
> > @@ -1827,7 +1827,19 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> > zCol = space_def->fields[iCol].name;
> > const char *name = NULL;
> > if (pEList->a[i].zName != NULL) {
> > - name = pEList->a[i].zName;
> > + if (is_full_meta) {
> > + const char *alias = NULL;
> > + if (pEList->a[i].zSpan != NULL) {
> > + alias = pEList->a[i].zName;
> > + name = pEList->a[i].zSpan;
> > + } else {
> > + alias = pEList->a[i].zName;
> > + name = pEList->a[i].zName;
>
> 2. What is a point of assigning alias to the same value
> in both branches of 'if'? Couldn't you do it out of
> the 'if' and drop {} ? The same below.
It makes no sense indeed, refactored:
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 0baabc62e..f0cf775c4 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1829,14 +1829,11 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
const char *name = NULL;
if (pEList->a[i].zName != NULL) {
if (is_full_meta) {
- const char *alias = NULL;
- if (pEList->a[i].zSpan != NULL) {
- alias = pEList->a[i].zName;
+ if (pEList->a[i].zSpan != NULL)
name = pEList->a[i].zSpan;
- } else {
- alias = pEList->a[i].zName;
+ else
name = pEList->a[i].zName;
- }
+ const char *alias = pEList->a[i].zName;
vdbe_metadata_set_col_alias(v, i, alias);
} else {
name = pEList->a[i].zName;
@@ -1866,14 +1863,11 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
const char *z = NULL;
if (pEList->a[i].zName != NULL) {
if (is_full_meta ) {
- const char *alias = NULL;
- if (pEList->a[i].zSpan != NULL) {
- alias = pEList->a[i].zName;
+ if (pEList->a[i].zSpan != NULL)
z = pEList->a[i].zSpan;
- } else {
- alias = pEList->a[i].zName;
+ else
z = pEList->a[i].zName;
- }
+ const char *alias = pEList->a[i].zName;
vdbe_metadata_set_col_alias(v, i, alias);
> > @@ -1854,9 +1866,21 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
> > }
> > } else {
> > const char *z = NULL;
> > - if (pEList->a[i].zName != NULL)
> > - z = pEList->a[i].zName;
> > - else if (pEList->a[i].zSpan != NULL)
> > + if (pEList->a[i].zName != NULL) {
> > + if (is_full_meta ) {
>
> 3. Extra whitespace after is_full_meta.
Removed.
> > diff --git a/test/sql/full_metadata.result b/test/sql/full_metadata.result
> > index 7c2982682..8f0bced36 100644
> > --- a/test/sql/full_metadata.result
> > +++ b/test/sql/full_metadata.result
> > @@ -118,6 +121,35 @@ execute("SELECT * FROM t;")
> > | - [1, 1, 'aSd']
> > | ...
> >
> > +-- Alias is always set in extended metadata. If column label in
> > +-- form of AS clause is set, then this alias is presented in
> > +-- metadata. Otherwise, alias is just the same as name.
>
> 4. But it is not true. The alias is not set always:
>
> =========================================================
>
> tarantool> box.execute('PRAGMA full_metadata = true;')
> ---
> - row_count: 0
> ...
>
> tarantool> box.execute('SELECT 1')
> ---
> - metadata:
> - name: '1'
> type: integer
> rows:
> - [1]
> ...
>
> =========================================================
>
> As you can see, no AS = no alias.
Always displaying alias (or label - call it whatever you want)
even in full_metadata mode doesn't seem to be reasonable for me.
Users can easily handle it in their code. Sending one map field
with string value duplicating the name and decoding it may affect
performance much more significant than one NULL check (IMHO).
> This is what I don't like
> in this patch the most. A user does not have a reliable always
> present column with a displayable name: either the column
> span or its alias after AS. He can't use name, because it
> changes its behavour depending on AS. And he can't use alias,
> because sometimes it is not present.
> That leads to confusion,
> and necessity to branch in user code a lot.
Yep, if user wants (for some reason; I don't have statistics how
often alias value can be useful or/and is used in production code)
to get alias value, one should make a small effort and add one
'if' condition:
if (is_full_metadata && alias == NULL)
alias = name;
That's it.
Alternatively (I don't take into consideration option of swapping
name and alias as you suggest) we can change current default behaviour
and in 'casual metadata mode' return name as name (not alias):
SELECT 1 AS x; -- would return '1' as name (now it returns 'x' as name).
But I guess some users may turn out to be confused (in case they
haven't red about full_metadata pragma in docs).
> The same example shows, that alias is a bad name. Here you need
> to return it, but it is not alias like AS. 'Label' would fit
> better here.
I don't care much how to call it. If you prefer 'label', I will rename it.
TBO I also don't care much which approach we should follow: always return
alias/label, fix current name/alias resolution, or swap name and alias
at all. If you insist on certain implementation, let's do it. If you
don't care much as well, let's ask smb else to take a final decision.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 6/6] sql: extend result set with alias
2019-12-24 0:26 ` Nikita Pettik
@ 2019-12-24 15:34 ` Vladislav Shpilevoy
2019-12-26 11:24 ` Nikita Pettik
0 siblings, 1 reply; 25+ messages in thread
From: Vladislav Shpilevoy @ 2019-12-24 15:34 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
Thanks for the discussion!
>>> diff --git a/test/sql/full_metadata.result b/test/sql/full_metadata.result
>>> index 7c2982682..8f0bced36 100644
>>> --- a/test/sql/full_metadata.result
>>> +++ b/test/sql/full_metadata.result
>>> @@ -118,6 +121,35 @@ execute("SELECT * FROM t;")
>>> | - [1, 1, 'aSd']
>>> | ...
>>>
>>> +-- Alias is always set in extended metadata. If column label in
>>> +-- form of AS clause is set, then this alias is presented in
>>> +-- metadata. Otherwise, alias is just the same as name.
>>
>> 4. But it is not true. The alias is not set always:
>>
>> =========================================================
>>
>> tarantool> box.execute('PRAGMA full_metadata = true;')
>> ---
>> - row_count: 0
>> ...
>>
>> tarantool> box.execute('SELECT 1')
>> ---
>> - metadata:
>> - name: '1'
>> type: integer
>> rows:
>> - [1]
>> ...
>>
>> =========================================================
>>
>> As you can see, no AS = no alias.
>
> Always displaying alias (or label - call it whatever you want)
> even in full_metadata mode doesn't seem to be reasonable for me.
> Users can easily handle it in their code. Sending one map field
> with string value duplicating the name and decoding it may affect
> performance much more significant than one NULL check (IMHO).
As I said already, I don't propose to send a duplicate name. I
propose to handle it on the client side. In Lua you can push one
string 2 times, and it will cost nothing - they will reference
one string object.
In IProto you still can send one name, when possible, and on the
netbox side push both names.
>> This is what I don't like
>> in this patch the most. A user does not have a reliable always
>> present column with a displayable name: either the column
>> span or its alias after AS. He can't use name, because it
>> changes its behavour depending on AS. And he can't use alias,
>> because sometimes it is not present.
>
>> That leads to confusion,
>> and necessity to branch in user code a lot.
>
> Yep, if user wants (for some reason; I don't have statistics how
> often alias value can be useful or/and is used in production code)
> to get alias value, one should make a small effort and add one
> 'if' condition:
>
> if (is_full_metadata && alias == NULL)
> alias = name;
>
> That's it.
>
Why, if we can remove that effort easily? I still don't see any
real arguments why can't we always push a label. The argument
about sending duplicate names is really weak - you can avoid
sending two names, and push them on the client side, as I said
above. No extra payload, not extra MessagePack decoding.
Even the standard driver, to which you appeal so much, always
returns a label.
> Alternatively (I don't take into consideration option of swapping
> name and alias as you suggest) we can change current default behaviour
> and in 'casual metadata mode' return name as name (not alias):
>
> SELECT 1 AS x; -- would return '1' as name (now it returns 'x' as name).
>
> But I guess some users may turn out to be confused (in case they
> haven't red about full_metadata pragma in docs).
>
>> The same example shows, that alias is a bad name. Here you need
>> to return it, but it is not alias like AS. 'Label' would fit
>> better here.
>
> I don't care much how to call it. If you prefer 'label', I will rename it.
Yes, I prefer label. Not because this is my personal taste. I provided
reasons, why it can't be called 'alias'. If you think alias is better,
they say why. That is a discussion.
> TBO I also don't care much which approach we should follow: always return
> alias/label, fix current name/alias resolution, or swap name and alias
> at all. If you insist on certain implementation, let's do it. If you
> don't care much as well, let's ask smb else to take a final decision.
>
I insist on not breaking the current behaviour. The patch breaks it -
'name' behaves differently whether full_metadata is specified or not.
Current clients may rely on the fact, that 'name' is always present,
and that it behaves like now: returns either alias, or the original
expression.
Here are solutions with which I am good:
- Keep the name as is, and add a new column meaning the original
expression: 'orig_name', 'oname', 'span', 'column', 'expr', etc.
In that case we don't break anything, we satisfy the driver, and
'full_metadata' only adds new fields. It does not change the
behaviour of always present columns.
- Or we could break the 'name' column a little, as you proposed -
always return it as if AS is not specified, even in short metadata.
In the full metadata it still will return the original expression,
but we add a 'label' column, which returns either alias or name.
In both solutions I want to rely on that both columns are always
present on the client side, when full metadata is specified.
Regardless of how are they sent in the binary protocol.
Talking of asking somebody - I tried to get a final decision of what we
should return from Alexander, because others just don't care. But the
only thing he provides is samples of how other DBs behave. AFAIU,
whatever we return and with whatever names, the driver will handle that.
But how the driver will parse our protocol is not a public thing. I worry
about public API, in Lua. Here the names and their behaviour matter.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 6/6] sql: extend result set with alias
2019-12-24 15:34 ` Vladislav Shpilevoy
@ 2019-12-26 11:24 ` Nikita Pettik
2019-12-27 11:53 ` Vladislav Shpilevoy
2019-12-27 23:44 ` Nikita Pettik
0 siblings, 2 replies; 25+ messages in thread
From: Nikita Pettik @ 2019-12-26 11:24 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On 24 Dec 16:34, Vladislav Shpilevoy wrote:
> Thanks for the discussion!
>
> >>> diff --git a/test/sql/full_metadata.result b/test/sql/full_metadata.result
> >>> index 7c2982682..8f0bced36 100644
> >>> --- a/test/sql/full_metadata.result
> >>> +++ b/test/sql/full_metadata.result
> >>> @@ -118,6 +121,35 @@ execute("SELECT * FROM t;")
> >>> | - [1, 1, 'aSd']
> >>> | ...
> >>>
> >>> +-- Alias is always set in extended metadata. If column label in
> >>> +-- form of AS clause is set, then this alias is presented in
> >>> +-- metadata. Otherwise, alias is just the same as name.
> >>
> >> 4. But it is not true. The alias is not set always:
> >>
> >> =========================================================
> >>
> >> tarantool> box.execute('PRAGMA full_metadata = true;')
> >> ---
> >> - row_count: 0
> >> ...
> >>
> >> tarantool> box.execute('SELECT 1')
> >> ---
> >> - metadata:
> >> - name: '1'
> >> type: integer
> >> rows:
> >> - [1]
> >> ...
> >>
> >> =========================================================
> >>
> >> As you can see, no AS = no alias.
> >
> > Always displaying alias (or label - call it whatever you want)
> > even in full_metadata mode doesn't seem to be reasonable for me.
> > Users can easily handle it in their code. Sending one map field
> > with string value duplicating the name and decoding it may affect
> > performance much more significant than one NULL check (IMHO).
>
> As I said already, I don't propose to send a duplicate name. I
> propose to handle it on the client side. In Lua you can push one
> string 2 times, and it will cost nothing - they will reference
> one string object.
>
> In IProto you still can send one name, when possible, and on the
> netbox side push both names.
From performance point of view this is ok. But still some
users may wonder why alias/label is displayed while there's
no explicit AS clause (even in full_metadata mode).
Moreover, now client only decodes received metadata, but in your
suggestion it is assumed that full_metadata mode is the same on
client and server. Otherwise, it would look inconsistent:
server (full_metadata = true), client (full_metadata = off):
server sends name, is_autoincrement, but client ignores extended
metadata and displays only name;
server (full_metadata = false), clien (full_metadata = true):
server sends name (but in full_metadata mode there's also is_autoincrement
for instance), but client displays only half of extended metadata (alias,
but not is_autoincrement or other fields).
Surely, we can send server's metadata status, but is it worth it?
> >> This is what I don't like
> >> in this patch the most. A user does not have a reliable always
> >> present column with a displayable name: either the column
> >> span or its alias after AS. He can't use name, because it
> >> changes its behavour depending on AS. And he can't use alias,
> >> because sometimes it is not present.
> >
> >> That leads to confusion,
> >> and necessity to branch in user code a lot.
> >
> > Yep, if user wants (for some reason; I don't have statistics how
> > often alias value can be useful or/and is used in production code)
> > to get alias value, one should make a small effort and add one
> > 'if' condition:
> >
> > if (is_full_metadata && alias == NULL)
> > alias = name;
> >
> > That's it.
> >
>
> Why, if we can remove that effort easily? I still don't see any
> real arguments why can't we always push a label. The argument
> about sending duplicate names is really weak - you can avoid
> sending two names, and push them on the client side, as I said
> above. No extra payload, not extra MessagePack decoding.
>
> Even the standard driver, to which you appeal so much, always
> returns a label.
> > Alternatively (I don't take into consideration option of swapping
> > name and alias as you suggest) we can change current default behaviour
> > and in 'casual metadata mode' return name as name (not alias):
> >
> > SELECT 1 AS x; -- would return '1' as name (now it returns 'x' as name).
> >
> > But I guess some users may turn out to be confused (in case they
> > haven't red about full_metadata pragma in docs).
> >
> >> The same example shows, that alias is a bad name. Here you need
> >> to return it, but it is not alias like AS. 'Label' would fit
> >> better here.
> >
> > I don't care much how to call it. If you prefer 'label', I will rename it.
>
> Yes, I prefer label. Not because this is my personal taste. I provided
> reasons, why it can't be called 'alias'. If you think alias is better,
> they say why. That is a discussion.
There are no strict definitions under these terms, so it is just
matter of taste. Let's call it label, I am not against (I guess
you assume that label = (name ∪ alias), meanwhile alias is supposed
only to be idenfitier after AS clause).
> > TBO I also don't care much which approach we should follow: always return
> > alias/label, fix current name/alias resolution, or swap name and alias
> > at all. If you insist on certain implementation, let's do it. If you
> > don't care much as well, let's ask smb else to take a final decision.
> >
>
> I insist on not breaking the current behaviour. The patch breaks it -
> 'name' behaves differently whether full_metadata is specified or not.
>
> Current clients may rely on the fact, that 'name' is always present,
> and that it behaves like now: returns either alias, or the original
> expression.
>
> Here are solutions with which I am good:
>
> - Keep the name as is, and add a new column meaning the original
> expression: 'orig_name', 'oname', 'span', 'column', 'expr', etc.
> In that case we don't break anything, we satisfy the driver, and
> 'full_metadata' only adds new fields. It does not change the
> behaviour of always present columns.
>
> - Or we could break the 'name' column a little, as you proposed -
> always return it as if AS is not specified, even in short metadata.
> In the full metadata it still will return the original expression,
> but we add a 'label' column, which returns either alias or name.
I am going to implement last option.
> In both solutions I want to rely on that both columns are always
> present on the client side, when full metadata is specified.
> Regardless of how are they sent in the binary protocol.
>
> Talking of asking somebody - I tried to get a final decision of what we
> should return from Alexander, because others just don't care. But the
> only thing he provides is samples of how other DBs behave. AFAIU,
> whatever we return and with whatever names, the driver will handle that.
>
> But how the driver will parse our protocol is not a public thing. I worry
> about public API, in Lua. Here the names and their behaviour matter.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 6/6] sql: extend result set with alias
2019-12-26 11:24 ` Nikita Pettik
@ 2019-12-27 11:53 ` Vladislav Shpilevoy
2019-12-27 23:44 ` Nikita Pettik
1 sibling, 0 replies; 25+ messages in thread
From: Vladislav Shpilevoy @ 2019-12-27 11:53 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
Thanks for the discussion!
>>>>> diff --git a/test/sql/full_metadata.result b/test/sql/full_metadata.result
>>>>> index 7c2982682..8f0bced36 100644
>>>>> --- a/test/sql/full_metadata.result
>>>>> +++ b/test/sql/full_metadata.result
>>>>> @@ -118,6 +121,35 @@ execute("SELECT * FROM t;")
>>>>> | - [1, 1, 'aSd']
>>>>> | ...
>>>>>
>>>>> +-- Alias is always set in extended metadata. If column label in
>>>>> +-- form of AS clause is set, then this alias is presented in
>>>>> +-- metadata. Otherwise, alias is just the same as name.
>>>>
>>>> 4. But it is not true. The alias is not set always:
>>>>
>>>> =========================================================
>>>>
>>>> tarantool> box.execute('PRAGMA full_metadata = true;')
>>>> ---
>>>> - row_count: 0
>>>> ...
>>>>
>>>> tarantool> box.execute('SELECT 1')
>>>> ---
>>>> - metadata:
>>>> - name: '1'
>>>> type: integer
>>>> rows:
>>>> - [1]
>>>> ...
>>>>
>>>> =========================================================
>>>>
>>>> As you can see, no AS = no alias.
>>>
>>> Always displaying alias (or label - call it whatever you want)
>>> even in full_metadata mode doesn't seem to be reasonable for me.
>>> Users can easily handle it in their code. Sending one map field
>>> with string value duplicating the name and decoding it may affect
>>> performance much more significant than one NULL check (IMHO).
>>
>> As I said already, I don't propose to send a duplicate name. I
>> propose to handle it on the client side. In Lua you can push one
>> string 2 times, and it will cost nothing - they will reference
>> one string object.
>>
>> In IProto you still can send one name, when possible, and on the
>> netbox side push both names.
>
> From performance point of view this is ok. But still some
> users may wonder why alias/label is displayed while there's
> no explicit AS clause (even in full_metadata mode).
When it is called 'label', I don't see a problem in showing it
always. It does not say, that it is something after AS. Now we
show 'name', even for expressions, and IMO this is more confusing.
Because expressions have no name, strictly speaking.
> Moreover, now client only decodes received metadata, but in your
> suggestion it is assumed that full_metadata mode is the same on
> client and server. Otherwise, it would look inconsistent:
I don't propose to introduce client-side full_metadata, it makes
no sense. But I do understand the problem.
Then we could send the not specified 'label' as MP_NIL. That will
occupy a couple of bytes per column, and will mean, that it is the
same as the 'name'. Or we could send a flag in IPROTO_FLAGS, that
the response contains full metadata even if it does not look so. It
depends on whether we need to know fullness of the metadata on the
client for anything else or not.
> server (full_metadata = true), client (full_metadata = off):
> server sends name, is_autoincrement, but client ignores extended
> metadata and displays only name;
>
> server (full_metadata = false), clien (full_metadata = true):
> server sends name (but in full_metadata mode there's also is_autoincrement
> for instance), but client displays only half of extended metadata (alias,
> but not is_autoincrement or other fields).
>
> Surely, we can send server's metadata status, but is it worth it?
I don't know. This is purely subjective thing - should we show both
name and label in full_metadata always or not. I think we should, and is
worth doing. You think we shouldn't. I provide arguments, that it will
simplify client's code. You provide that without this we don't need to
send MP_NIL as a not specified name / shouldn't push 2 names, when there
is only one, and it is maybe faster.
Also my argument is that client-side code is not so perf sensitive as
server's. So even if it will show sometimes 2 names instead of 1, it
won't really affect anything.
You can try asking in the red chat, what behaviour they expect.
>>>> This is what I don't like
>>>> in this patch the most. A user does not have a reliable always
>>>> present column with a displayable name: either the column
>>>> span or its alias after AS. He can't use name, because it
>>>> changes its behavour depending on AS. And he can't use alias,
>>>> because sometimes it is not present.
>>>
>>>> That leads to confusion,
>>>> and necessity to branch in user code a lot.
>>>
>>> Yep, if user wants (for some reason; I don't have statistics how
>>> often alias value can be useful or/and is used in production code)
>>> to get alias value, one should make a small effort and add one
>>> 'if' condition:
>>>
>>> if (is_full_metadata && alias == NULL)
>>> alias = name;
>>>
>>> That's it.
>>>
>>
>> Why, if we can remove that effort easily? I still don't see any
>> real arguments why can't we always push a label. The argument
>> about sending duplicate names is really weak - you can avoid
>> sending two names, and push them on the client side, as I said
>> above. No extra payload, not extra MessagePack decoding.
>>
>> Even the standard driver, to which you appeal so much, always
>> returns a label.
>>> Alternatively (I don't take into consideration option of swapping
>>> name and alias as you suggest) we can change current default behaviour
>>> and in 'casual metadata mode' return name as name (not alias):
>>>
>>> SELECT 1 AS x; -- would return '1' as name (now it returns 'x' as name).
>>>
>>> But I guess some users may turn out to be confused (in case they
>>> haven't red about full_metadata pragma in docs).
>>>
>>>> The same example shows, that alias is a bad name. Here you need
>>>> to return it, but it is not alias like AS. 'Label' would fit
>>>> better here.
>>>
>>> I don't care much how to call it. If you prefer 'label', I will rename it.
>>
>> Yes, I prefer label. Not because this is my personal taste. I provided
>> reasons, why it can't be called 'alias'. If you think alias is better,
>> they say why. That is a discussion.
>
> There are no strict definitions under these terms, so it is just
> matter of taste. Let's call it label, I am not against (I guess
> you assume that label = (name ∪ alias), meanwhile alias is supposed
> only to be idenfitier after AS clause).
Yes, this is why I vote for label.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 6/6] sql: extend result set with alias
2019-12-26 11:24 ` Nikita Pettik
2019-12-27 11:53 ` Vladislav Shpilevoy
@ 2019-12-27 23:44 ` Nikita Pettik
2019-12-28 9:29 ` Vladislav Shpilevoy
1 sibling, 1 reply; 25+ messages in thread
From: Nikita Pettik @ 2019-12-27 23:44 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On 26 Dec 14:24, Nikita Pettik wrote:
> On 24 Dec 16:34, Vladislav Shpilevoy wrote:
> > Here are solutions with which I am good:
> >
> > - Keep the name as is, and add a new column meaning the original
> > expression: 'orig_name', 'oname', 'span', 'column', 'expr', etc.
> > In that case we don't break anything, we satisfy the driver, and
> > 'full_metadata' only adds new fields. It does not change the
> > behaviour of always present columns.
> >
> > - Or we could break the 'name' column a little, as you proposed -
> > always return it as if AS is not specified, even in short metadata.
> > In the full metadata it still will return the original expression,
> > but we add a 'label' column, which returns either alias or name.
>
> I am going to implement last option.
Okay, I've tried but it turned out to be not so easy (to be more
precise - not easy at all). The problem lies in views.
We store aliases as view column names. So changing names may
(most likely) make already existing view unable to be queried.
There's simple reason for using aliases as field names:
in case spans are taken for names, users won't be able to select
columns by names:
CREATE VIEW v1 AS SELECT a+1 FROM t; -- 'a+1' is a column name
SELECT a+1 FROM v1 -- fails: can't resolve field 'A' since real
field's name is 'a+1' (a+1 is considered
to be expression, not id).
We might think making exeption in name/label rule for views,
but from functional point of view there's no difference between
view and casual select, since first step of execution of
SELECT ... FROM VIEW ... is substitution of view body into select.
So I have to reject my initial plan and instead I am going
to follow first option from your suggestion.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Tarantool-patches] [PATCH v2 6/6] sql: extend result set with alias
2019-12-27 23:44 ` Nikita Pettik
@ 2019-12-28 9:29 ` Vladislav Shpilevoy
0 siblings, 0 replies; 25+ messages in thread
From: Vladislav Shpilevoy @ 2019-12-28 9:29 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
Ok, understood, thanks for working on this and
checking these options.
On 28/12/2019 02:44, Nikita Pettik wrote:
> On 26 Dec 14:24, Nikita Pettik wrote:
>> On 24 Dec 16:34, Vladislav Shpilevoy wrote:
>>> Here are solutions with which I am good:
>>>
>>> - Keep the name as is, and add a new column meaning the original
>>> expression: 'orig_name', 'oname', 'span', 'column', 'expr', etc.
>>> In that case we don't break anything, we satisfy the driver, and
>>> 'full_metadata' only adds new fields. It does not change the
>>> behaviour of always present columns.
>>>
>>> - Or we could break the 'name' column a little, as you proposed -
>>> always return it as if AS is not specified, even in short metadata.
>>> In the full metadata it still will return the original expression,
>>> but we add a 'label' column, which returns either alias or name.
>>
>> I am going to implement last option.
>
> Okay, I've tried but it turned out to be not so easy (to be more
> precise - not easy at all). The problem lies in views.
> We store aliases as view column names. So changing names may
> (most likely) make already existing view unable to be queried.
> There's simple reason for using aliases as field names:
> in case spans are taken for names, users won't be able to select
> columns by names:
>
> CREATE VIEW v1 AS SELECT a+1 FROM t; -- 'a+1' is a column name
> SELECT a+1 FROM v1 -- fails: can't resolve field 'A' since real
> field's name is 'a+1' (a+1 is considered
> to be expression, not id).
>
> We might think making exeption in name/label rule for views,
> but from functional point of view there's no difference between
> view and casual select, since first step of execution of
> SELECT ... FROM VIEW ... is substitution of view body into select.
>
> So I have to reject my initial plan and instead I am going
> to follow first option from your suggestion.
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread