Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format
@ 2020-04-10 21:54 imeevma
  0 siblings, 0 replies; 7+ messages in thread
From: imeevma @ 2020-04-10 21:54 UTC (permalink / raw)
  To: korablev, tsafin, tarantool-patches

This patch allows to specify field types in ephemeral space format.
Prior to this patch, all fields had a SCALAR field type.

This patch allows us to not use the primary index to obtain field
types, since now the ephemeral space has field types in the
format. This allows us to change the structure of the primary
index, which helps to solve the issue #4256. In addition, since we
can now set the field types of the ephemeral space, we can use
this feature to set the field types according to the left value of
the IN operator. This will fix issue #4692.

Needed for #4256
Needed for #4692
Closes #3841
---
https://github.com/tarantool/tarantool/issues/3841
https://github.com/tarantool/tarantool/tree/imeevma/gh-3841-format-for-ephemeral-space

 src/box/space_def.c    |  5 ++---
 src/box/space_def.h    |  3 ++-
 src/box/sql.c          | 52 +++++++++++++++++++++++++++++++++++---------------
 src/box/tuple_format.c | 22 ++++++++++-----------
 4 files changed, 51 insertions(+), 31 deletions(-)

diff --git a/src/box/space_def.c b/src/box/space_def.c
index 6611312..5af0f38 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -233,7 +233,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
 }
 
 struct space_def*
-space_def_new_ephemeral(uint32_t field_count)
+space_def_new_ephemeral(uint32_t field_count, struct field_def *fields)
 {
 	struct space_opts opts = space_opts_default;
 	opts.is_temporary = true;
@@ -242,8 +242,7 @@ space_def_new_ephemeral(uint32_t field_count)
 						    "ephemeral",
 						    strlen("ephemeral"),
 						    "memtx", strlen("memtx"),
-						    &opts, &field_def_default,
-						    0);
+						    &opts, fields, field_count);
 	return space_def;
 }
 
diff --git a/src/box/space_def.h b/src/box/space_def.h
index ac6d226..61bbdb4 100644
--- a/src/box/space_def.h
+++ b/src/box/space_def.h
@@ -173,11 +173,12 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
 /**
  * Create a new ephemeral space definition.
  * @param field_count Number of fields in the space.
+ * @param fields Field definitions.
  *
  * @retval Space definition.
  */
 struct space_def *
-space_def_new_ephemeral(uint32_t field_count);
+space_def_new_ephemeral(uint32_t field_count, struct field_def *fields);
 
 /**
  * Size of the space_def.
diff --git a/src/box/sql.c b/src/box/sql.c
index 1256df8..e4434b2 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -330,13 +330,41 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 			return NULL;
 	}
 
-	struct key_part_def *ephemer_key_parts = region_alloc(&fiber()->gc,
-				sizeof(*ephemer_key_parts) * field_count);
-	if (ephemer_key_parts == NULL) {
-		diag_set(OutOfMemory, sizeof(*ephemer_key_parts) * field_count,
-			 "region", "key parts");
-		return NULL;
+	struct region *region = &fiber()->gc;
+	/*
+	 * Name of the fields will be "_COLUMN_1", "_COLUMN_2"
+	 * and so on. Since number of columns no more than 2000,
+	 * length of each name is no more than strlen("_COLUMN_")
+	 * + 5.
+	 */
+	assert(SQL_MAX_COLUMN <= 2000);
+	uint32_t name_len = strlen("_COLUMN_") + 5;
+	uint32_t size = field_count * (sizeof(struct field_def) + name_len +
+				       sizeof(struct key_part_def));
+	struct field_def *fields = region_alloc(region, size);
+	struct key_part_def *ephemer_key_parts = (void *)fields +
+				     field_count * sizeof(struct field_def);
+	char *names = (char *)ephemer_key_parts +
+		      field_count * sizeof(struct key_part_def);
+	for (uint32_t i = 0; i < field_count; ++i) {
+		struct field_def *field = &fields[i];
+		field->name = names;
+		names += name_len;
+		sprintf(field->name, "_COLUMN_%d", i);
+		field->is_nullable = true;
+		field->nullable_action = ON_CONFLICT_ACTION_NONE;
+		field->default_value = NULL;
+		field->default_value_expr = NULL;
+		if (def != NULL && i < def->part_count) {
+			assert(def->parts[i].type < field_type_MAX);
+			field->type = def->parts[i].type;
+			field->coll_id = def->parts[i].coll_id;
+		} else {
+			field->coll_id = COLL_NONE;
+			field->type = FIELD_TYPE_SCALAR;
+		}
 	}
+
 	for (uint32_t i = 0; i < field_count; ++i) {
 		struct key_part_def *part = &ephemer_key_parts[i];
 		part->fieldno = i;
@@ -344,14 +372,8 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 		part->is_nullable = true;
 		part->sort_order = SORT_ORDER_ASC;
 		part->path = NULL;
-		if (def != NULL && i < def->part_count) {
-			assert(def->parts[i].type < field_type_MAX);
-			part->type = def->parts[i].type;
-			part->coll_id = def->parts[i].coll_id;
-		} else {
-			part->coll_id = COLL_NONE;
-			part->type = FIELD_TYPE_SCALAR;
-		}
+		part->type = fields[i].type;
+		part->coll_id = fields[i].coll_id;
 	}
 	struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
 						      field_count, false);
@@ -370,7 +392,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 	rlist_add_entry(&key_list, ephemer_index_def, link);
 
 	struct space_def *ephemer_space_def =
-		space_def_new_ephemeral(field_count);
+		space_def_new_ephemeral(field_count, fields);
 	if (ephemer_space_def == NULL) {
 		index_def_delete(ephemer_index_def);
 		return NULL;
diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index 312c966..beaeb0f 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -698,6 +698,12 @@ tuple_format_destroy(struct tuple_format *format)
  * dictionary will never be altered. If it can, then alter can
  * affect another space, which shares a format with one which is
  * altered.
+ *
+ * The only way to change the format of the space is to recreate
+ * space with the new format inside of BOX. Since there is no
+ * mechanism for recreating the ephemeral space, we need not worry
+ * about changing the format of the ephemeral space.
+ *
  * @param p_format Double pointer to format. It is updated with
  * 		   hashed value, if corresponding format was found
  * 		   in hash table
@@ -709,13 +715,7 @@ static bool
 tuple_format_reuse(struct tuple_format **p_format)
 {
 	struct tuple_format *format = *p_format;
-	if (!format->is_ephemeral)
-		return false;
-	/*
-	 * These fields do not participate in hashing.
-	 * Make sure they're unset.
-	 */
-	assert(format->dict->name_count == 0);
+	assert(format->is_ephemeral);
 	assert(format->is_temporary);
 	mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format,
 					    NULL);
@@ -739,9 +739,7 @@ tuple_format_reuse(struct tuple_format **p_format)
 static int
 tuple_format_add_to_hash(struct tuple_format *format)
 {
-	if(!format->is_ephemeral)
-		return 0;
-	assert(format->dict->name_count == 0);
+	assert(format->is_ephemeral);
 	assert(format->is_temporary);
 	mh_int_t key = mh_tuple_format_put(tuple_formats_hash,
 					   (const struct tuple_format **)&format,
@@ -795,11 +793,11 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine,
 	if (tuple_format_create(format, keys, key_count, space_fields,
 				space_field_count) < 0)
 		goto err;
-	if (tuple_format_reuse(&format))
+	if (is_ephemeral && tuple_format_reuse(&format))
 		return format;
 	if (tuple_format_register(format) < 0)
 		goto err;
-	if (tuple_format_add_to_hash(format) < 0) {
+	if (is_ephemeral && tuple_format_add_to_hash(format) < 0) {
 		tuple_format_deregister(format);
 		goto err;
 	}
-- 
2.7.4

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format
  2020-04-10 19:16     ` Vladislav Shpilevoy
@ 2020-04-10 21:52       ` Mergen Imeev
  0 siblings, 0 replies; 7+ messages in thread
From: Mergen Imeev @ 2020-04-10 21:52 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi! Thank you for review. My answers below.

On Fri, Apr 10, 2020 at 09:16:28PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> See 2 comments below.
> 
> > diff --git a/src/box/sql.c b/src/box/sql.c
> > index 27089bd..288c4fa 100644
> > --- a/src/box/sql.c
> > +++ b/src/box/sql.c
> > @@ -337,14 +337,15 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
> >  	 * length of each name is no more than strlen("_COLUMN_")
> >  	 * + 5.
> >  	 */
> > -	assert(SQL_MAX_COLUMN < 9999);
> > +	assert(SQL_MAX_COLUMN <= 2000);
> >  	uint32_t name_len = sizeof("_COLUMN_") + 5;
> 
> 1. sizeof("_COLUMN_") != strlen("_COLUMN_"). The latter is 8,
> the former is 9. Better use strlen().
> 
Thanks, fixed.

> >  	uint32_t size = field_count * (sizeof(struct field_def) + name_len +
> >  				       sizeof(struct key_part_def));
> >  	struct field_def *fields = region_alloc(region, size);
> > -	struct key_part_def *parts = (void *)fields +
> > +	struct key_part_def *ephemer_key_parts = (void *)fields +
> >  				     field_count * sizeof(struct field_def);
> > -	char *names = (char *)parts + field_count * sizeof(struct key_part_def);
> > +	char *names = (char *)ephemer_key_parts +
> > +		      field_count * sizeof(struct key_part_def);
> >  	for (uint32_t i = 0; i < field_count; ++i) {
> >  		struct field_def *field = &fields[i];
> >  		field->name = names;
> 
> 2. It is worth extending the commit message with more details, how
> is it needed for 4256 and 4692, with all what we discussed in private.
> 
Done:

    sql: specify field types in ephemeral space format
    
    This patch allows to specify field types in ephemeral space format.
    Prior to this patch, all fields had a SCALAR field type.
    
    This patch allows us to not use the primary index to obtain field
    types, since now the ephemeral space has field types in the
    format. This allows us to change the structure of the primary
    index, which helps to solve the issue #4256. In addition, since we
    can now set the field types of the ephemeral space, we can use
    this feature to set the field types according to the left value of
    the IN operator. This will fix issue #4692.
    
    Needed for #4256
    Needed for #4692
    Closes #3841


> Please, do the things above and send to Nikita.
> 
> Also, in case it will be pushed in this form to master, I propose you to
> create a ticket on ephemeral_space API rework. As you fairly noticed, we
> have issues with using sql_key_info as, in fact, a format definition.
> We basically *can't* index fields not added to the format.
Ok, will do.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format
  2020-04-10  0:04   ` Mergen Imeev
@ 2020-04-10 19:16     ` Vladislav Shpilevoy
  2020-04-10 21:52       ` Mergen Imeev
  0 siblings, 1 reply; 7+ messages in thread
From: Vladislav Shpilevoy @ 2020-04-10 19:16 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Hi! Thanks for the fixes!

See 2 comments below.

> diff --git a/src/box/sql.c b/src/box/sql.c
> index 27089bd..288c4fa 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -337,14 +337,15 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
>  	 * length of each name is no more than strlen("_COLUMN_")
>  	 * + 5.
>  	 */
> -	assert(SQL_MAX_COLUMN < 9999);
> +	assert(SQL_MAX_COLUMN <= 2000);
>  	uint32_t name_len = sizeof("_COLUMN_") + 5;

1. sizeof("_COLUMN_") != strlen("_COLUMN_"). The latter is 8,
the former is 9. Better use strlen().

>  	uint32_t size = field_count * (sizeof(struct field_def) + name_len +
>  				       sizeof(struct key_part_def));
>  	struct field_def *fields = region_alloc(region, size);
> -	struct key_part_def *parts = (void *)fields +
> +	struct key_part_def *ephemer_key_parts = (void *)fields +
>  				     field_count * sizeof(struct field_def);
> -	char *names = (char *)parts + field_count * sizeof(struct key_part_def);
> +	char *names = (char *)ephemer_key_parts +
> +		      field_count * sizeof(struct key_part_def);
>  	for (uint32_t i = 0; i < field_count; ++i) {
>  		struct field_def *field = &fields[i];
>  		field->name = names;

2. It is worth extending the commit message with more details, how
is it needed for 4256 and 4692, with all what we discussed in private.

Please, do the things above and send to Nikita.

Also, in case it will be pushed in this form to master, I propose you to
create a ticket on ephemeral_space API rework. As you fairly noticed, we
have issues with using sql_key_info as, in fact, a format definition.
We basically *can't* index fields not added to the format.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format
  2020-04-04 18:34 ` Vladislav Shpilevoy
@ 2020-04-10  0:04   ` Mergen Imeev
  2020-04-10 19:16     ` Vladislav Shpilevoy
  0 siblings, 1 reply; 7+ messages in thread
From: Mergen Imeev @ 2020-04-10  0:04 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi! Thank you for review! My answers, diff and new patch
below.

On Sat, Apr 04, 2020 at 08:34:32PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> On 02/04/2020 12:41, imeevma@tarantool.org wrote:
> > This patch allows to specify field types in ephemeral space format.
> > Prior to this patch, all fields had a SCALAR field type.
> 
> 1. Why is it needed? (I see description in the ticket, but it should
> be in the commit message too).
> 
> I copy paste it here:
> 
> > During ephemeral table creation via SQL all parts are declared as SCALAR: see sql_ephemeral_space_create. However, in most cases it is possible to fill it with specific type - it may speed up internal routine connected with tuples comparison.
> 
I am not sure that it will speed up something, at least at
this patch. But this patch is needed for #4692 and #4256.

> And the real question is how is it going to speedup and what exactly?
> If the subject are internal comparators, they don't depend on format.
> They depend only on key definitions, and the latter are ok so far, they
> have types.
> 
> > Closes #3841
> > ---
> > https://github.com/tarantool/tarantool/issues/3841
> > https://github.com/tarantool/tarantool/tree/imeevma/gh-3841-format-for-ephemeral-space
> > 
> > diff --git a/src/box/space_def.h b/src/box/space_def.h
> > index ac6d226..e7b0c28 100644
> > --- a/src/box/space_def.h
> > +++ b/src/box/space_def.h
> > @@ -177,7 +177,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
> >   * @retval Space definition.
> >   */
> >  struct space_def *
> > -space_def_new_ephemeral(uint32_t field_count);
> > +space_def_new_ephemeral(uint32_t field_count, struct field_def *fields);
> 
> 2. The comment is not updated.
> 
Fixed.

> >  /**
> >   * Size of the space_def.
> > diff --git a/src/box/sql.c b/src/box/sql.c
> > index 1256df8..27089bd 100644
> > --- a/src/box/sql.c
> > +++ b/src/box/sql.c
> > @@ -330,58 +330,77 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
> >  			return NULL;
> >  	}
> >  
> > -	struct key_part_def *ephemer_key_parts = region_alloc(&fiber()->gc,
> > -				sizeof(*ephemer_key_parts) * field_count);
> > -	if (ephemer_key_parts == NULL) {
> > -		diag_set(OutOfMemory, sizeof(*ephemer_key_parts) * field_count,
> > -			 "region", "key parts");
> > -		return NULL;
> > +	struct region *region = &fiber()->gc;
> > +	/*
> > +	 * Name of the fields will be "_COLUMN_1", "_COLUMN_2"
> > +	 * and so on. Since number of columns no more than 2000,
> 
> 3. You said no more than 2000, but below check no more than 9999.
> Why?
> 
Important here is that number of figures is no more than 4.
Still, I fixed that.

> > +	 * length of each name is no more than strlen("_COLUMN_")
> > +	 * + 5.
> > +	 */
> > +	assert(SQL_MAX_COLUMN < 9999);
> > +	uint32_t name_len = sizeof("_COLUMN_") + 5;
> > +	uint32_t size = field_count * (sizeof(struct field_def) + name_len +
> > +				       sizeof(struct key_part_def));
> > +	struct field_def *fields = region_alloc(region, size);
> > +	struct key_part_def *parts = (void *)fields +
> > +				     field_count * sizeof(struct field_def);
> > +	char *names = (char *)parts + field_count * sizeof(struct key_part_def);
> > +	for (uint32_t i = 0; i < field_count; ++i) {
> 
> 4. Why do you need a second cycle for this? Can the fields
> be initialized in the same cycle as key parts?
> 
They can, but I plan to change the definition of the index
in some cases, which is easier to do when they are in two
different cycles. The only problem was that index had to have
all fields since it worked as format in some cases. This is not
so now since format is fixed. Still, I changed assert.

> > +		struct field_def *field = &fields[i];
> > +		field->name = names;
> > +		names += name_len;
> > +		sprintf(field->name, "_COLUMN_%d", i);
> > +		field->is_nullable = true;
> > +		field->nullable_action = ON_CONFLICT_ACTION_NONE;
> > +		field->default_value = NULL;
> > +		field->default_value_expr = NULL;
> > +		if (def != NULL && i < def->part_count) {
> > +			assert(def->parts[i].type < field_type_MAX);
> > +			field->type = def->parts[i].type;
> > +			field->coll_id = def->parts[i].coll_id;
> > +		} else {
> > +			field->coll_id = COLL_NONE;
> > +			field->type = FIELD_TYPE_SCALAR;
> > +		}
> >  	}
> > +
> >  	for (uint32_t i = 0; i < field_count; ++i) {
> > -		struct key_part_def *part = &ephemer_key_parts[i];
> > +		struct key_part_def *part = &parts[i];
> >  		part->fieldno = i;
> >  		part->nullable_action = ON_CONFLICT_ACTION_NONE;
> >  		part->is_nullable = true;
> >  		part->sort_order = SORT_ORDER_ASC;
> >  		part->path = NULL;
> > -		if (def != NULL && i < def->part_count) {
> > -			assert(def->parts[i].type < field_type_MAX);
> > -			part->type = def->parts[i].type;
> > -			part->coll_id = def->parts[i].coll_id;
> > -		} else {
> > -			part->coll_id = COLL_NONE;
> > -			part->type = FIELD_TYPE_SCALAR;
> > -		}
> > +		part->type = fields[i].type;
> > +		part->coll_id = fields[i].coll_id;
> >  	}
> > -	struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
> > -						      field_count, false);
> > -	if (ephemer_key_def == NULL)
> > +	struct key_def *key_def = key_def_new(parts, field_count, false);
> > +	if (key_def == NULL)
> >  		return NULL;
> >  
> > -	struct index_def *ephemer_index_def =
> > +	struct index_def *index_def =
> >  		index_def_new(0, 0, "ephemer_idx", strlen("ephemer_idx"), TREE,
> > -			      &index_opts_default, ephemer_key_def, NULL);
> > -	key_def_delete(ephemer_key_def);
> > -	if (ephemer_index_def == NULL)
> > +			      &index_opts_default, key_def, NULL);
> > +	key_def_delete(key_def);
> > +	if (index_def == NULL)
> >  		return NULL;
> >  
> >  	struct rlist key_list;
> >  	rlist_create(&key_list);
> > -	rlist_add_entry(&key_list, ephemer_index_def, link);
> > +	rlist_add_entry(&key_list, index_def, link);
> >  
> > -	struct space_def *ephemer_space_def =
> > -		space_def_new_ephemeral(field_count);
> > -	if (ephemer_space_def == NULL) {
> > -		index_def_delete(ephemer_index_def);
> > +	struct space_def *space_def = space_def_new_ephemeral(field_count,
> > +							      fields);
> > +	if (space_def == NULL) {
> > +		index_def_delete(index_def);
> >  		return NULL;
> >  	}
> >  
> > -	struct space *ephemer_new_space = space_new_ephemeral(ephemer_space_def,
> > -							      &key_list);
> > -	index_def_delete(ephemer_index_def);
> > -	space_def_delete(ephemer_space_def);
> > +	struct space *space = space_new_ephemeral(space_def, &key_list);
> > +	index_def_delete(index_def);
> > +	space_def_delete(space_def);
> >  
> > -	return ephemer_new_space;
> > +	return space;
> 
> 5. Seems like you could keep the old names, and avoid half of these
> changes.
> 
Fixed, returned old names.

> >  }
> >  
> >  int tarantoolsqlEphemeralInsert(struct space *space, const char *tuple,
> > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
> > index 312c966..beaeb0f 100644
> > --- a/src/box/tuple_format.c
> > +++ b/src/box/tuple_format.c
> 
> 6. Looks like all the diff in this file can be dropped. Because it
> does not contain any functional changes. Right? You just turned
> 'if' into assert + the same 'if' a bit earlier.
> 
Not exactly so. Since now we have dict in format of
ephemeral space old assert throws an error each time
ephemeral space is used. New 'if' allows to remove this
assert without changing general logic.

> > @@ -698,6 +698,12 @@ tuple_format_destroy(struct tuple_format *format)
> >   * dictionary will never be altered. If it can, then alter can
> >   * affect another space, which shares a format with one which is
> >   * altered.
> > + *
> > + * The only way to change the format of the space is to recreate
> > + * space with the new format inside of BOX. Since there is no
> > + * mechanism for recreating the ephemeral space, we need not worry
> > + * about changing the format of the ephemeral space.
> > + *
> >   * @param p_format Double pointer to format. It is updated with
> >   * 		   hashed value, if corresponding format was found
> >   * 		   in hash table
> > @@ -709,13 +715,7 @@ static bool
> >  tuple_format_reuse(struct tuple_format **p_format)
> >  {
> >  	struct tuple_format *format = *p_format;
> > -	if (!format->is_ephemeral)
> > -		return false;
> > -	/*
> > -	 * These fields do not participate in hashing.
> > -	 * Make sure they're unset.
> > -	 */
> > -	assert(format->dict->name_count == 0);
> > +	assert(format->is_ephemeral);
> >  	assert(format->is_temporary);
> >  	mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format,
> >  					    NULL);
> > @@ -739,9 +739,7 @@ tuple_format_reuse(struct tuple_format **p_format)
> >  static int
> >  tuple_format_add_to_hash(struct tuple_format *format)
> >  {
> > -	if(!format->is_ephemeral)
> > -		return 0;
> > -	assert(format->dict->name_count == 0);
> > +	assert(format->is_ephemeral);
> >  	assert(format->is_temporary);
> >  	mh_int_t key = mh_tuple_format_put(tuple_formats_hash,
> >  					   (const struct tuple_format **)&format,
> > @@ -795,11 +793,11 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine,
> >  	if (tuple_format_create(format, keys, key_count, space_fields,
> >  				space_field_count) < 0)
> >  		goto err;
> > -	if (tuple_format_reuse(&format))
> > +	if (is_ephemeral && tuple_format_reuse(&format))
> >  		return format;
> >  	if (tuple_format_register(format) < 0)
> >  		goto err;
> > -	if (tuple_format_add_to_hash(format) < 0) {
> > +	if (is_ephemeral && tuple_format_add_to_hash(format) < 0) {
> >  		tuple_format_deregister(format);
> >  		goto err;
> >  	}
> > 

Diff:

From f5e07106409a3183d81b57f4dd44bb868848021d Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Fri, 10 Apr 2020 02:42:54 +0300
Subject: [PATCH] Review fix


diff --git a/src/box/space_def.h b/src/box/space_def.h
index e7b0c28..61bbdb4 100644
--- a/src/box/space_def.h
+++ b/src/box/space_def.h
@@ -173,6 +173,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
 /**
  * Create a new ephemeral space definition.
  * @param field_count Number of fields in the space.
+ * @param fields Field definitions.
  *
  * @retval Space definition.
  */
diff --git a/src/box/sql.c b/src/box/sql.c
index 27089bd..288c4fa 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -337,14 +337,15 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 	 * length of each name is no more than strlen("_COLUMN_")
 	 * + 5.
 	 */
-	assert(SQL_MAX_COLUMN < 9999);
+	assert(SQL_MAX_COLUMN <= 2000);
 	uint32_t name_len = sizeof("_COLUMN_") + 5;
 	uint32_t size = field_count * (sizeof(struct field_def) + name_len +
 				       sizeof(struct key_part_def));
 	struct field_def *fields = region_alloc(region, size);
-	struct key_part_def *parts = (void *)fields +
+	struct key_part_def *ephemer_key_parts = (void *)fields +
 				     field_count * sizeof(struct field_def);
-	char *names = (char *)parts + field_count * sizeof(struct key_part_def);
+	char *names = (char *)ephemer_key_parts +
+		      field_count * sizeof(struct key_part_def);
 	for (uint32_t i = 0; i < field_count; ++i) {
 		struct field_def *field = &fields[i];
 		field->name = names;
@@ -365,7 +366,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 	}
 
 	for (uint32_t i = 0; i < field_count; ++i) {
-		struct key_part_def *part = &parts[i];
+		struct key_part_def *part = &ephemer_key_parts[i];
 		part->fieldno = i;
 		part->nullable_action = ON_CONFLICT_ACTION_NONE;
 		part->is_nullable = true;
@@ -374,33 +375,35 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 		part->type = fields[i].type;
 		part->coll_id = fields[i].coll_id;
 	}
-	struct key_def *key_def = key_def_new(parts, field_count, false);
-	if (key_def == NULL)
+	struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
+						      field_count, false);
+	if (ephemer_key_def == NULL)
 		return NULL;
 
-	struct index_def *index_def =
+	struct index_def *ephemer_index_def =
 		index_def_new(0, 0, "ephemer_idx", strlen("ephemer_idx"), TREE,
-			      &index_opts_default, key_def, NULL);
-	key_def_delete(key_def);
-	if (index_def == NULL)
+			      &index_opts_default, ephemer_key_def, NULL);
+	key_def_delete(ephemer_key_def);
+	if (ephemer_index_def == NULL)
 		return NULL;
 
 	struct rlist key_list;
 	rlist_create(&key_list);
-	rlist_add_entry(&key_list, index_def, link);
+	rlist_add_entry(&key_list, ephemer_index_def, link);
 
-	struct space_def *space_def = space_def_new_ephemeral(field_count,
-							      fields);
-	if (space_def == NULL) {
-		index_def_delete(index_def);
+	struct space_def *ephemer_space_def =
+		space_def_new_ephemeral(field_count, fields);
+	if (ephemer_space_def == NULL) {
+		index_def_delete(ephemer_index_def);
 		return NULL;
 	}
 
-	struct space *space = space_new_ephemeral(space_def, &key_list);
-	index_def_delete(index_def);
-	space_def_delete(space_def);
+	struct space *ephemer_new_space = space_new_ephemeral(ephemer_space_def,
+							      &key_list);
+	index_def_delete(ephemer_index_def);
+	space_def_delete(ephemer_space_def);
 
-	return space;
+	return ephemer_new_space;
 }
 
 int tarantoolsqlEphemeralInsert(struct space *space, const char *tuple,


New patch:

From 2cf0d4749e7858b533225374f62da8556a51bcd9 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Thu, 23 Jan 2020 18:34:00 +0300
Subject: [PATCH] sql: specify field types in ephemeral space format

This patch allows to specify field types in ephemeral space format.
Prior to this patch, all fields had a SCALAR field type.

This patch simplifies the structure of the ephemeral space a little.

Needed for #4256
Needed for #4692
Closes #3841

diff --git a/src/box/space_def.c b/src/box/space_def.c
index 6611312..5af0f38 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -233,7 +233,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
 }
 
 struct space_def*
-space_def_new_ephemeral(uint32_t field_count)
+space_def_new_ephemeral(uint32_t field_count, struct field_def *fields)
 {
 	struct space_opts opts = space_opts_default;
 	opts.is_temporary = true;
@@ -242,8 +242,7 @@ space_def_new_ephemeral(uint32_t field_count)
 						    "ephemeral",
 						    strlen("ephemeral"),
 						    "memtx", strlen("memtx"),
-						    &opts, &field_def_default,
-						    0);
+						    &opts, fields, field_count);
 	return space_def;
 }
 
diff --git a/src/box/space_def.h b/src/box/space_def.h
index ac6d226..61bbdb4 100644
--- a/src/box/space_def.h
+++ b/src/box/space_def.h
@@ -173,11 +173,12 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
 /**
  * Create a new ephemeral space definition.
  * @param field_count Number of fields in the space.
+ * @param fields Field definitions.
  *
  * @retval Space definition.
  */
 struct space_def *
-space_def_new_ephemeral(uint32_t field_count);
+space_def_new_ephemeral(uint32_t field_count, struct field_def *fields);
 
 /**
  * Size of the space_def.
diff --git a/src/box/sql.c b/src/box/sql.c
index 1256df8..288c4fa 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -330,13 +330,41 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 			return NULL;
 	}
 
-	struct key_part_def *ephemer_key_parts = region_alloc(&fiber()->gc,
-				sizeof(*ephemer_key_parts) * field_count);
-	if (ephemer_key_parts == NULL) {
-		diag_set(OutOfMemory, sizeof(*ephemer_key_parts) * field_count,
-			 "region", "key parts");
-		return NULL;
+	struct region *region = &fiber()->gc;
+	/*
+	 * Name of the fields will be "_COLUMN_1", "_COLUMN_2"
+	 * and so on. Since number of columns no more than 2000,
+	 * length of each name is no more than strlen("_COLUMN_")
+	 * + 5.
+	 */
+	assert(SQL_MAX_COLUMN <= 2000);
+	uint32_t name_len = sizeof("_COLUMN_") + 5;
+	uint32_t size = field_count * (sizeof(struct field_def) + name_len +
+				       sizeof(struct key_part_def));
+	struct field_def *fields = region_alloc(region, size);
+	struct key_part_def *ephemer_key_parts = (void *)fields +
+				     field_count * sizeof(struct field_def);
+	char *names = (char *)ephemer_key_parts +
+		      field_count * sizeof(struct key_part_def);
+	for (uint32_t i = 0; i < field_count; ++i) {
+		struct field_def *field = &fields[i];
+		field->name = names;
+		names += name_len;
+		sprintf(field->name, "_COLUMN_%d", i);
+		field->is_nullable = true;
+		field->nullable_action = ON_CONFLICT_ACTION_NONE;
+		field->default_value = NULL;
+		field->default_value_expr = NULL;
+		if (def != NULL && i < def->part_count) {
+			assert(def->parts[i].type < field_type_MAX);
+			field->type = def->parts[i].type;
+			field->coll_id = def->parts[i].coll_id;
+		} else {
+			field->coll_id = COLL_NONE;
+			field->type = FIELD_TYPE_SCALAR;
+		}
 	}
+
 	for (uint32_t i = 0; i < field_count; ++i) {
 		struct key_part_def *part = &ephemer_key_parts[i];
 		part->fieldno = i;
@@ -344,14 +372,8 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 		part->is_nullable = true;
 		part->sort_order = SORT_ORDER_ASC;
 		part->path = NULL;
-		if (def != NULL && i < def->part_count) {
-			assert(def->parts[i].type < field_type_MAX);
-			part->type = def->parts[i].type;
-			part->coll_id = def->parts[i].coll_id;
-		} else {
-			part->coll_id = COLL_NONE;
-			part->type = FIELD_TYPE_SCALAR;
-		}
+		part->type = fields[i].type;
+		part->coll_id = fields[i].coll_id;
 	}
 	struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
 						      field_count, false);
@@ -370,7 +392,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 	rlist_add_entry(&key_list, ephemer_index_def, link);
 
 	struct space_def *ephemer_space_def =
-		space_def_new_ephemeral(field_count);
+		space_def_new_ephemeral(field_count, fields);
 	if (ephemer_space_def == NULL) {
 		index_def_delete(ephemer_index_def);
 		return NULL;
diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index 312c966..beaeb0f 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -698,6 +698,12 @@ tuple_format_destroy(struct tuple_format *format)
  * dictionary will never be altered. If it can, then alter can
  * affect another space, which shares a format with one which is
  * altered.
+ *
+ * The only way to change the format of the space is to recreate
+ * space with the new format inside of BOX. Since there is no
+ * mechanism for recreating the ephemeral space, we need not worry
+ * about changing the format of the ephemeral space.
+ *
  * @param p_format Double pointer to format. It is updated with
  * 		   hashed value, if corresponding format was found
  * 		   in hash table
@@ -709,13 +715,7 @@ static bool
 tuple_format_reuse(struct tuple_format **p_format)
 {
 	struct tuple_format *format = *p_format;
-	if (!format->is_ephemeral)
-		return false;
-	/*
-	 * These fields do not participate in hashing.
-	 * Make sure they're unset.
-	 */
-	assert(format->dict->name_count == 0);
+	assert(format->is_ephemeral);
 	assert(format->is_temporary);
 	mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format,
 					    NULL);
@@ -739,9 +739,7 @@ tuple_format_reuse(struct tuple_format **p_format)
 static int
 tuple_format_add_to_hash(struct tuple_format *format)
 {
-	if(!format->is_ephemeral)
-		return 0;
-	assert(format->dict->name_count == 0);
+	assert(format->is_ephemeral);
 	assert(format->is_temporary);
 	mh_int_t key = mh_tuple_format_put(tuple_formats_hash,
 					   (const struct tuple_format **)&format,
@@ -795,11 +793,11 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine,
 	if (tuple_format_create(format, keys, key_count, space_fields,
 				space_field_count) < 0)
 		goto err;
-	if (tuple_format_reuse(&format))
+	if (is_ephemeral && tuple_format_reuse(&format))
 		return format;
 	if (tuple_format_register(format) < 0)
 		goto err;
-	if (tuple_format_add_to_hash(format) < 0) {
+	if (is_ephemeral && tuple_format_add_to_hash(format) < 0) {
 		tuple_format_deregister(format);
 		goto err;
 	}

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format
  2020-04-02 10:41 imeevma
  2020-04-03 13:00 ` Nikita Pettik
@ 2020-04-04 18:34 ` Vladislav Shpilevoy
  2020-04-10  0:04   ` Mergen Imeev
  1 sibling, 1 reply; 7+ messages in thread
From: Vladislav Shpilevoy @ 2020-04-04 18:34 UTC (permalink / raw)
  To: imeevma, tsafin, tarantool-patches

Hi! Thanks for the patch!

On 02/04/2020 12:41, imeevma@tarantool.org wrote:
> This patch allows to specify field types in ephemeral space format.
> Prior to this patch, all fields had a SCALAR field type.

1. Why is it needed? (I see description in the ticket, but it should
be in the commit message too).

I copy paste it here:

> During ephemeral table creation via SQL all parts are declared as SCALAR: see sql_ephemeral_space_create. However, in most cases it is possible to fill it with specific type - it may speed up internal routine connected with tuples comparison.

And the real question is how is it going to speedup and what exactly?
If the subject are internal comparators, they don't depend on format.
They depend only on key definitions, and the latter are ok so far, they
have types.

> Closes #3841
> ---
> https://github.com/tarantool/tarantool/issues/3841
> https://github.com/tarantool/tarantool/tree/imeevma/gh-3841-format-for-ephemeral-space
> 
> diff --git a/src/box/space_def.h b/src/box/space_def.h
> index ac6d226..e7b0c28 100644
> --- a/src/box/space_def.h
> +++ b/src/box/space_def.h
> @@ -177,7 +177,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
>   * @retval Space definition.
>   */
>  struct space_def *
> -space_def_new_ephemeral(uint32_t field_count);
> +space_def_new_ephemeral(uint32_t field_count, struct field_def *fields);

2. The comment is not updated.

>  /**
>   * Size of the space_def.
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 1256df8..27089bd 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -330,58 +330,77 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
>  			return NULL;
>  	}
>  
> -	struct key_part_def *ephemer_key_parts = region_alloc(&fiber()->gc,
> -				sizeof(*ephemer_key_parts) * field_count);
> -	if (ephemer_key_parts == NULL) {
> -		diag_set(OutOfMemory, sizeof(*ephemer_key_parts) * field_count,
> -			 "region", "key parts");
> -		return NULL;
> +	struct region *region = &fiber()->gc;
> +	/*
> +	 * Name of the fields will be "_COLUMN_1", "_COLUMN_2"
> +	 * and so on. Since number of columns no more than 2000,

3. You said no more than 2000, but below check no more than 9999.
Why?

> +	 * length of each name is no more than strlen("_COLUMN_")
> +	 * + 5.
> +	 */
> +	assert(SQL_MAX_COLUMN < 9999);
> +	uint32_t name_len = sizeof("_COLUMN_") + 5;
> +	uint32_t size = field_count * (sizeof(struct field_def) + name_len +
> +				       sizeof(struct key_part_def));
> +	struct field_def *fields = region_alloc(region, size);
> +	struct key_part_def *parts = (void *)fields +
> +				     field_count * sizeof(struct field_def);
> +	char *names = (char *)parts + field_count * sizeof(struct key_part_def);
> +	for (uint32_t i = 0; i < field_count; ++i) {

4. Why do you need a second cycle for this? Can the fields
be initialized in the same cycle as key parts?

> +		struct field_def *field = &fields[i];
> +		field->name = names;
> +		names += name_len;
> +		sprintf(field->name, "_COLUMN_%d", i);
> +		field->is_nullable = true;
> +		field->nullable_action = ON_CONFLICT_ACTION_NONE;
> +		field->default_value = NULL;
> +		field->default_value_expr = NULL;
> +		if (def != NULL && i < def->part_count) {
> +			assert(def->parts[i].type < field_type_MAX);
> +			field->type = def->parts[i].type;
> +			field->coll_id = def->parts[i].coll_id;
> +		} else {
> +			field->coll_id = COLL_NONE;
> +			field->type = FIELD_TYPE_SCALAR;
> +		}
>  	}
> +
>  	for (uint32_t i = 0; i < field_count; ++i) {
> -		struct key_part_def *part = &ephemer_key_parts[i];
> +		struct key_part_def *part = &parts[i];
>  		part->fieldno = i;
>  		part->nullable_action = ON_CONFLICT_ACTION_NONE;
>  		part->is_nullable = true;
>  		part->sort_order = SORT_ORDER_ASC;
>  		part->path = NULL;
> -		if (def != NULL && i < def->part_count) {
> -			assert(def->parts[i].type < field_type_MAX);
> -			part->type = def->parts[i].type;
> -			part->coll_id = def->parts[i].coll_id;
> -		} else {
> -			part->coll_id = COLL_NONE;
> -			part->type = FIELD_TYPE_SCALAR;
> -		}
> +		part->type = fields[i].type;
> +		part->coll_id = fields[i].coll_id;
>  	}
> -	struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
> -						      field_count, false);
> -	if (ephemer_key_def == NULL)
> +	struct key_def *key_def = key_def_new(parts, field_count, false);
> +	if (key_def == NULL)
>  		return NULL;
>  
> -	struct index_def *ephemer_index_def =
> +	struct index_def *index_def =
>  		index_def_new(0, 0, "ephemer_idx", strlen("ephemer_idx"), TREE,
> -			      &index_opts_default, ephemer_key_def, NULL);
> -	key_def_delete(ephemer_key_def);
> -	if (ephemer_index_def == NULL)
> +			      &index_opts_default, key_def, NULL);
> +	key_def_delete(key_def);
> +	if (index_def == NULL)
>  		return NULL;
>  
>  	struct rlist key_list;
>  	rlist_create(&key_list);
> -	rlist_add_entry(&key_list, ephemer_index_def, link);
> +	rlist_add_entry(&key_list, index_def, link);
>  
> -	struct space_def *ephemer_space_def =
> -		space_def_new_ephemeral(field_count);
> -	if (ephemer_space_def == NULL) {
> -		index_def_delete(ephemer_index_def);
> +	struct space_def *space_def = space_def_new_ephemeral(field_count,
> +							      fields);
> +	if (space_def == NULL) {
> +		index_def_delete(index_def);
>  		return NULL;
>  	}
>  
> -	struct space *ephemer_new_space = space_new_ephemeral(ephemer_space_def,
> -							      &key_list);
> -	index_def_delete(ephemer_index_def);
> -	space_def_delete(ephemer_space_def);
> +	struct space *space = space_new_ephemeral(space_def, &key_list);
> +	index_def_delete(index_def);
> +	space_def_delete(space_def);
>  
> -	return ephemer_new_space;
> +	return space;

5. Seems like you could keep the old names, and avoid half of these
changes.

>  }
>  
>  int tarantoolsqlEphemeralInsert(struct space *space, const char *tuple,
> diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
> index 312c966..beaeb0f 100644
> --- a/src/box/tuple_format.c
> +++ b/src/box/tuple_format.c

6. Looks like all the diff in this file can be dropped. Because it
does not contain any functional changes. Right? You just turned
'if' into assert + the same 'if' a bit earlier.

> @@ -698,6 +698,12 @@ tuple_format_destroy(struct tuple_format *format)
>   * dictionary will never be altered. If it can, then alter can
>   * affect another space, which shares a format with one which is
>   * altered.
> + *
> + * The only way to change the format of the space is to recreate
> + * space with the new format inside of BOX. Since there is no
> + * mechanism for recreating the ephemeral space, we need not worry
> + * about changing the format of the ephemeral space.
> + *
>   * @param p_format Double pointer to format. It is updated with
>   * 		   hashed value, if corresponding format was found
>   * 		   in hash table
> @@ -709,13 +715,7 @@ static bool
>  tuple_format_reuse(struct tuple_format **p_format)
>  {
>  	struct tuple_format *format = *p_format;
> -	if (!format->is_ephemeral)
> -		return false;
> -	/*
> -	 * These fields do not participate in hashing.
> -	 * Make sure they're unset.
> -	 */
> -	assert(format->dict->name_count == 0);
> +	assert(format->is_ephemeral);
>  	assert(format->is_temporary);
>  	mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format,
>  					    NULL);
> @@ -739,9 +739,7 @@ tuple_format_reuse(struct tuple_format **p_format)
>  static int
>  tuple_format_add_to_hash(struct tuple_format *format)
>  {
> -	if(!format->is_ephemeral)
> -		return 0;
> -	assert(format->dict->name_count == 0);
> +	assert(format->is_ephemeral);
>  	assert(format->is_temporary);
>  	mh_int_t key = mh_tuple_format_put(tuple_formats_hash,
>  					   (const struct tuple_format **)&format,
> @@ -795,11 +793,11 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine,
>  	if (tuple_format_create(format, keys, key_count, space_fields,
>  				space_field_count) < 0)
>  		goto err;
> -	if (tuple_format_reuse(&format))
> +	if (is_ephemeral && tuple_format_reuse(&format))
>  		return format;
>  	if (tuple_format_register(format) < 0)
>  		goto err;
> -	if (tuple_format_add_to_hash(format) < 0) {
> +	if (is_ephemeral && tuple_format_add_to_hash(format) < 0) {
>  		tuple_format_deregister(format);
>  		goto err;
>  	}
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format
  2020-04-02 10:41 imeevma
@ 2020-04-03 13:00 ` Nikita Pettik
  2020-04-04 18:34 ` Vladislav Shpilevoy
  1 sibling, 0 replies; 7+ messages in thread
From: Nikita Pettik @ 2020-04-03 13:00 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches, v.shpilevoy

On 02 Apr 13:41, imeevma@tarantool.org wrote:
> This patch allows to specify field types in ephemeral space format.
> Prior to this patch, all fields had a SCALAR field type.
> Closes #3841
> ---
> https://github.com/tarantool/tarantool/issues/3841
> https://github.com/tarantool/tarantool/tree/imeevma/gh-3841-format-for-ephemeral-space
> 
> @ChangeLog
>  - Format of ephemeral spaces can have field with types other that
>    SCALAR now (gh-3841).
> 

This change is not visible to users. Should we extend changelog
with corresponding entry?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format
@ 2020-04-02 10:41 imeevma
  2020-04-03 13:00 ` Nikita Pettik
  2020-04-04 18:34 ` Vladislav Shpilevoy
  0 siblings, 2 replies; 7+ messages in thread
From: imeevma @ 2020-04-02 10:41 UTC (permalink / raw)
  To: v.shpilevoy, tsafin, tarantool-patches

This patch allows to specify field types in ephemeral space format.
Prior to this patch, all fields had a SCALAR field type.

Closes #3841
---
https://github.com/tarantool/tarantool/issues/3841
https://github.com/tarantool/tarantool/tree/imeevma/gh-3841-format-for-ephemeral-space

@ChangeLog
 - Format of ephemeral spaces can have field with types other that
   SCALAR now (gh-3841).

 src/box/space_def.c    |  5 ++-
 src/box/space_def.h    |  2 +-
 src/box/sql.c          | 83 +++++++++++++++++++++++++++++++-------------------
 src/box/tuple_format.c | 22 ++++++-------
 4 files changed, 64 insertions(+), 48 deletions(-)

diff --git a/src/box/space_def.c b/src/box/space_def.c
index 6611312..5af0f38 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -233,7 +233,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
 }
 
 struct space_def*
-space_def_new_ephemeral(uint32_t field_count)
+space_def_new_ephemeral(uint32_t field_count, struct field_def *fields)
 {
 	struct space_opts opts = space_opts_default;
 	opts.is_temporary = true;
@@ -242,8 +242,7 @@ space_def_new_ephemeral(uint32_t field_count)
 						    "ephemeral",
 						    strlen("ephemeral"),
 						    "memtx", strlen("memtx"),
-						    &opts, &field_def_default,
-						    0);
+						    &opts, fields, field_count);
 	return space_def;
 }
 
diff --git a/src/box/space_def.h b/src/box/space_def.h
index ac6d226..e7b0c28 100644
--- a/src/box/space_def.h
+++ b/src/box/space_def.h
@@ -177,7 +177,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
  * @retval Space definition.
  */
 struct space_def *
-space_def_new_ephemeral(uint32_t field_count);
+space_def_new_ephemeral(uint32_t field_count, struct field_def *fields);
 
 /**
  * Size of the space_def.
diff --git a/src/box/sql.c b/src/box/sql.c
index 1256df8..27089bd 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -330,58 +330,77 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 			return NULL;
 	}
 
-	struct key_part_def *ephemer_key_parts = region_alloc(&fiber()->gc,
-				sizeof(*ephemer_key_parts) * field_count);
-	if (ephemer_key_parts == NULL) {
-		diag_set(OutOfMemory, sizeof(*ephemer_key_parts) * field_count,
-			 "region", "key parts");
-		return NULL;
+	struct region *region = &fiber()->gc;
+	/*
+	 * Name of the fields will be "_COLUMN_1", "_COLUMN_2"
+	 * and so on. Since number of columns no more than 2000,
+	 * length of each name is no more than strlen("_COLUMN_")
+	 * + 5.
+	 */
+	assert(SQL_MAX_COLUMN < 9999);
+	uint32_t name_len = sizeof("_COLUMN_") + 5;
+	uint32_t size = field_count * (sizeof(struct field_def) + name_len +
+				       sizeof(struct key_part_def));
+	struct field_def *fields = region_alloc(region, size);
+	struct key_part_def *parts = (void *)fields +
+				     field_count * sizeof(struct field_def);
+	char *names = (char *)parts + field_count * sizeof(struct key_part_def);
+	for (uint32_t i = 0; i < field_count; ++i) {
+		struct field_def *field = &fields[i];
+		field->name = names;
+		names += name_len;
+		sprintf(field->name, "_COLUMN_%d", i);
+		field->is_nullable = true;
+		field->nullable_action = ON_CONFLICT_ACTION_NONE;
+		field->default_value = NULL;
+		field->default_value_expr = NULL;
+		if (def != NULL && i < def->part_count) {
+			assert(def->parts[i].type < field_type_MAX);
+			field->type = def->parts[i].type;
+			field->coll_id = def->parts[i].coll_id;
+		} else {
+			field->coll_id = COLL_NONE;
+			field->type = FIELD_TYPE_SCALAR;
+		}
 	}
+
 	for (uint32_t i = 0; i < field_count; ++i) {
-		struct key_part_def *part = &ephemer_key_parts[i];
+		struct key_part_def *part = &parts[i];
 		part->fieldno = i;
 		part->nullable_action = ON_CONFLICT_ACTION_NONE;
 		part->is_nullable = true;
 		part->sort_order = SORT_ORDER_ASC;
 		part->path = NULL;
-		if (def != NULL && i < def->part_count) {
-			assert(def->parts[i].type < field_type_MAX);
-			part->type = def->parts[i].type;
-			part->coll_id = def->parts[i].coll_id;
-		} else {
-			part->coll_id = COLL_NONE;
-			part->type = FIELD_TYPE_SCALAR;
-		}
+		part->type = fields[i].type;
+		part->coll_id = fields[i].coll_id;
 	}
-	struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
-						      field_count, false);
-	if (ephemer_key_def == NULL)
+	struct key_def *key_def = key_def_new(parts, field_count, false);
+	if (key_def == NULL)
 		return NULL;
 
-	struct index_def *ephemer_index_def =
+	struct index_def *index_def =
 		index_def_new(0, 0, "ephemer_idx", strlen("ephemer_idx"), TREE,
-			      &index_opts_default, ephemer_key_def, NULL);
-	key_def_delete(ephemer_key_def);
-	if (ephemer_index_def == NULL)
+			      &index_opts_default, key_def, NULL);
+	key_def_delete(key_def);
+	if (index_def == NULL)
 		return NULL;
 
 	struct rlist key_list;
 	rlist_create(&key_list);
-	rlist_add_entry(&key_list, ephemer_index_def, link);
+	rlist_add_entry(&key_list, index_def, link);
 
-	struct space_def *ephemer_space_def =
-		space_def_new_ephemeral(field_count);
-	if (ephemer_space_def == NULL) {
-		index_def_delete(ephemer_index_def);
+	struct space_def *space_def = space_def_new_ephemeral(field_count,
+							      fields);
+	if (space_def == NULL) {
+		index_def_delete(index_def);
 		return NULL;
 	}
 
-	struct space *ephemer_new_space = space_new_ephemeral(ephemer_space_def,
-							      &key_list);
-	index_def_delete(ephemer_index_def);
-	space_def_delete(ephemer_space_def);
+	struct space *space = space_new_ephemeral(space_def, &key_list);
+	index_def_delete(index_def);
+	space_def_delete(space_def);
 
-	return ephemer_new_space;
+	return space;
 }
 
 int tarantoolsqlEphemeralInsert(struct space *space, const char *tuple,
diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index 312c966..beaeb0f 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -698,6 +698,12 @@ tuple_format_destroy(struct tuple_format *format)
  * dictionary will never be altered. If it can, then alter can
  * affect another space, which shares a format with one which is
  * altered.
+ *
+ * The only way to change the format of the space is to recreate
+ * space with the new format inside of BOX. Since there is no
+ * mechanism for recreating the ephemeral space, we need not worry
+ * about changing the format of the ephemeral space.
+ *
  * @param p_format Double pointer to format. It is updated with
  * 		   hashed value, if corresponding format was found
  * 		   in hash table
@@ -709,13 +715,7 @@ static bool
 tuple_format_reuse(struct tuple_format **p_format)
 {
 	struct tuple_format *format = *p_format;
-	if (!format->is_ephemeral)
-		return false;
-	/*
-	 * These fields do not participate in hashing.
-	 * Make sure they're unset.
-	 */
-	assert(format->dict->name_count == 0);
+	assert(format->is_ephemeral);
 	assert(format->is_temporary);
 	mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format,
 					    NULL);
@@ -739,9 +739,7 @@ tuple_format_reuse(struct tuple_format **p_format)
 static int
 tuple_format_add_to_hash(struct tuple_format *format)
 {
-	if(!format->is_ephemeral)
-		return 0;
-	assert(format->dict->name_count == 0);
+	assert(format->is_ephemeral);
 	assert(format->is_temporary);
 	mh_int_t key = mh_tuple_format_put(tuple_formats_hash,
 					   (const struct tuple_format **)&format,
@@ -795,11 +793,11 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine,
 	if (tuple_format_create(format, keys, key_count, space_fields,
 				space_field_count) < 0)
 		goto err;
-	if (tuple_format_reuse(&format))
+	if (is_ephemeral && tuple_format_reuse(&format))
 		return format;
 	if (tuple_format_register(format) < 0)
 		goto err;
-	if (tuple_format_add_to_hash(format) < 0) {
+	if (is_ephemeral && tuple_format_add_to_hash(format) < 0) {
 		tuple_format_deregister(format);
 		goto err;
 	}
-- 
2.7.4

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-04-10 21:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-10 21:54 [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format imeevma
  -- strict thread matches above, loose matches on Subject: below --
2020-04-02 10:41 imeevma
2020-04-03 13:00 ` Nikita Pettik
2020-04-04 18:34 ` Vladislav Shpilevoy
2020-04-10  0:04   ` Mergen Imeev
2020-04-10 19:16     ` Vladislav Shpilevoy
2020-04-10 21:52       ` Mergen Imeev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox