[Tarantool-patches] [PATCH 1/6] sql: refactor resulting set metadata
Nikita Pettik
korablev at tarantool.org
Thu Dec 5 14:39:33 MSK 2019
> > diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> > index 0ecc676e2..2d3466cc7 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_set_metadata_col_name(sParse.pVdbe, i,
> > + azColName[name_index]);
> > + vdbe_set_metadata_col_type(sParse.pVdbe, i,
> > + azColName[name_index + 1]);
>
> 1. Lets fix this indentation.
NP:
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 2d3466cc7..9edc59ab5 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -147,9 +147,9 @@ sqlPrepare(sql * db, /* Database handle. */
for (int i = 0; i < name_count; i++) {
int name_index = 2 * i + name_first;
vdbe_set_metadata_col_name(sParse.pVdbe, i,
- azColName[name_index]);
+ azColName[name_index]);
vdbe_set_metadata_col_type(sParse.pVdbe, i,
- azColName[name_index + 1]);
+ azColName[name_index + 1]);
}
}
> > diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> > index 8f93edd16..d6b8a158f 100644
> > --- a/src/box/sql/select.c
> > +++ b/src/box/sql/select.c
> > @@ -2828,7 +2824,7 @@ multiSelect(Parse * pParse, /* Parsing context */
> > Select *pFirst = p;
> > while (pFirst->pPrior)
> > pFirst = pFirst->pPrior;
> > - generateColumnNames(pParse,
> > + generate_column_metadata(pParse,
> > pFirst->pSrc,
> > pFirst->pEList);
>
> 2. And this too.
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index d6b8a158f..212f33dd7 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -2825,8 +2825,8 @@ multiSelect(Parse * pParse, /* Parsing context */
while (pFirst->pPrior)
pFirst = pFirst->pPrior;
generate_column_metadata(pParse,
- pFirst->pSrc,
- pFirst->pEList);
+ pFirst->pSrc,
+ pFirst->pEList);
}
iBreak = sqlVdbeMakeLabel(v);
iCont = sqlVdbeMakeLabel(v);
> > }
> > diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
> > index 582d48a1f..4142fb6ba 100644
> > --- a/src/box/sql/vdbe.h
> > +++ b/src/box/sql/vdbe.h
> > @@ -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_set_metadata_col_name(struct Vdbe *v, int col_idx, const char *name);
> > +
> > +int
> > +vdbe_set_metadata_col_type(struct Vdbe *v, int col_idx, const char *type);
>
> 3. I think, that we should use one prefix for metadata
> methods. You used 'vdbe_metadata_delete()' above, so lets
> make it 'vdbe_metadata_*' everywhere. And these functions
> will be
>
> vdbe_metadata_set_col_name/type...
Ok, renamed here and in other patches.
> > diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> > index 0f32b4cd6..9ab3753cb 100644
> > --- a/src/box/sql/vdbeInt.h
> > +++ b/src/box/sql/vdbeInt.h
> > @@ -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 SELECT queries. */
>
> 4. Looks like it can be emitted on DML too. I see
> vdbe_set_metadata_col_name(v, 0, "rows updated") in
> sqlUpdate(), and vdbe_set_metadata_col_name(v, 0, "rows deleted")
> in sql_table_delete_from().
Fair, fixed comment:
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 9ab3753cb..4fabab9d6 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -399,7 +399,7 @@ 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 */
- /** SQL metadata for SELECT queries. */
+ /** 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 */
> > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> > index a1d658648..db11fbf33 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((void *)v->metadata[i].name);
> > + free((void *)v->metadata[i].type);
>
> 5. The necessity to do that cast basically makes 'const'
> in the struct useless. Could you please make them just
> char *, not const char *?
Yep:
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 9ab3753cb..64250bee2 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -347,8 +347,8 @@ struct ScanStatus {
};
struct sql_column_metadata {
- const char *name;
- const char *type;
+ char *name;
+ char *type;
};
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 8cbe6c368..e5528c0e2 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1832,8 +1832,8 @@ vdbe_metadata_delete(struct Vdbe *v)
{
if (v->metadata != NULL) {
for (int i = 0; i < v->nResColumn; ++i) {
- free((void *)v->metadata[i].name);
- free((void *)v->metadata[i].type);
+ free(v->metadata[i].name);
+ free(v->metadata[i].type);
}
free(v->metadata);
}
>
> > + }
> > + 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)
> > +vdbe_set_metadata_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((void *)p->metadata[idx].name);
> > + p->metadata[idx].name = strdup(name);
> > + if (p->metadata[idx].name == NULL) {
> > + diag_set(OutOfMemory, strlen(name), "strdup", "name");
>
> 6. Len + 1. Dup allocates terminating zero too. In the next patches
> too.
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index db11fbf33..8cbe6c368 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1861,28 +1861,28 @@ sqlVdbeSetNumCols(Vdbe * p, int nResColumn)
}
int
-vdbe_set_metadata_col_name(struct Vdbe *p, int idx, const char *name)
+vdbe_metadata_set_col_name(struct Vdbe *p, int idx, const char *name)
{
assert(idx < p->nResColumn);
if (p->metadata[idx].name != NULL)
free((void *)p->metadata[idx].name);
p->metadata[idx].name = strdup(name);
if (p->metadata[idx].name == NULL) {
- diag_set(OutOfMemory, strlen(name), "strdup", "name");
+ diag_set(OutOfMemory, strlen(name) + 1, "strdup", "name");
return -1;
}
return 0;
}
int
-vdbe_set_metadata_col_type(struct Vdbe *p, int idx, const char *type)
+vdbe_metadata_set_col_type(struct Vdbe *p, int idx, const char *type)
{
assert(idx < p->nResColumn);
if (p->metadata[idx].type != NULL)
free((void *)p->metadata[idx].type);
p->metadata[idx].type = strdup(type);
if (p->metadata[idx].type == NULL) {
- diag_set(OutOfMemory, strlen(type), "strdup", "type");
+ diag_set(OutOfMemory, strlen(type) + 1, "strdup", "type");
return -1;
}
return 0;
More information about the Tarantool-patches
mailing list