From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id B7F9B2E847 for ; Sun, 26 May 2019 08:05:29 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 0hJhFt8f8BS8 for ; Sun, 26 May 2019 08:05:29 -0400 (EDT) Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 6A6CE2E6CD for ; Sun, 26 May 2019 08:05:29 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v5 3/6] sql: introduce tuple_fetcher class References: From: Vladislav Shpilevoy Message-ID: Date: Sun, 26 May 2019 15:05:26 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Kirill Shcherbatov , tarantool-patches@freelists.org Hi! Thanks for the fixes! See 10 comments below. > diff --git a/src/box/sql.h b/src/box/sql.h > index 15ef74b19..3aaeb2274 100644 > --- a/src/box/sql.h > +++ b/src/box/sql.h > @@ -386,6 +386,52 @@ sql_src_list_entry_name(const struct SrcList *list, int i); > void > sqlSrcListDelete(struct sql *db, struct SrcList *list); > > +/** > + * Auxilary VDBE structure to speed-up tuple data field access. 1. 'Auxilary' -> 'Auxiliary'. Looks like you still did not install spell checker, as I asked in some previous patchsets. Please, do it. > + * A memory allocation that manage this structure must have > + * trailing unused bytes that extends the last 'slots' array. > + * The amount of reserved memory should correspond to the problem > + * to be solved and is usually equal to the greatest number of > + * fields in the tuple. > + * > + * +------------------------+ > + * | struct tuple_fetcher | > + * +------------------------+ > + * | RESERVED MEMORY | > + * +------------------------+ > + */ > +struct tuple_fetcher { > + /** Tuple pointer or NULL when undefined. */ > + struct tuple *tuple; > + /** Tuple data pointer. */ > + const char *data; > + /** Tuple data size. */ > + uint32_t data_sz; > + /** Count of fields in tuple. */ > + uint32_t field_count; > + /** > + * Index of the rightmost initialized slot in slots > + * array. > + */ > + uint32_t rightmost_slot; > + /** > + * Array of offsets of tuple fields. > + * Only values <= rightmost_slot are valid. > + */ > + uint32_t slots[1]; > +}; > + > +/** > + * Initialize a new tuple_fetcher instance with given tuple > + * data. > + * @param fetcher The tuple_fetcher instance to initialize. > + * @param tuple The tuple object pointer or NULL when undefined. > + * @param data The tuple data (is always defined). > + * @param data_sz The size of tuple data (is always defined). > + */ > +void > +tuple_fetcher_create(struct tuple_fetcher *fetcher, struct tuple *tuple, > + const char *data, uint32_t data_sz); 2. Great, I like this function, especially that it does not have any single 'if' - perfect case for processor's conveyor parallelizing. But the API is ambiguous - what should a user pass here? Always both tuple and data, or tuple and different data, or tuple and its data, or tuple + NULL vs NULL + data? Please, expose two public functions: tuple_fetcher_prepare_data and tuple_fetcher_prepare_tuple. Each takes either const char *data or struct tuple *tuple. Internally they call tuple_fetcher_create with both tuple and data. Once you did it, you can stop calling tarantoolsqlPayloadFetch in OP_Column and do either tuple_fetcher_prepare_tuple(pCrsr->last_tuple) in case it is a normal cursor, or do tuple_fetcher_prepare_data(pReg->z, pReg->n) in case of a pseudo cursor. > > #if defined(__cplusplus) > } /* extern "C" { */ > diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h > index 2b04d961e..bb0f8f1a4 100644 > --- a/src/box/sql/tarantoolInt.h > +++ b/src/box/sql/tarantoolInt.h > @@ -28,7 +28,8 @@ const void *tarantoolsqlPayloadFetch(BtCursor * pCur, u32 * pAmt); > * offset to @a fieldno. > */ > const void * > -tarantoolsqlTupleColumnFast(BtCursor *pCur, u32 fieldno, u32 *field_size); > +tarantool_tuple_field_fast(struct tuple *tuple, uint32_t fieldno, > + uint32_t *field_size); 3. I suggest you to make this function method of the fetcher and static inline right above tuple_fetcher_fetch(). It is not needed in any other place. > > int tarantoolsqlFirst(BtCursor * pCur, int *pRes); > int tarantoolsqlLast(BtCursor * pCur, int *pRes); > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 5d37f63fb..33c5458cd 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -612,6 +612,119 @@ mem_type_to_str(const struct Mem *p) > } > } > > +/** > + * Fetch field by field_idx using tuple_fetcher and store result > + * in dest_mem. > + * @param fetcher The initialized tuple_fetcher instance to use. > + * @param field_idx The id of the field to fetch. > + * @param field_type The destination memory field type is used > + * when fetch result type is undefined. 4. The only reason to make tuple_fetcher_fetch() a separate function was to get rid of this parameter and related code, keeping it in OP_Column. Please, do it. Remove the parameter and keep it in OP_Column. > + * @param default_val_mem The value to return when fetcher's data > + * lacks field by given @a field_idx. 5. Why would ever need that parameter for OP_Fetch? It is not a task of a fetcher to return default fields IMO. It should only fetch a field, not replace it with a default value. > + * @param[out] dest_mem The memory variable to store result. > + * @retval SQL_OK Status code in case of success. > + * @retval sql_ret_code Error code otherwise. > + */ > +static int > +tuple_fetcher_fetch(struct tuple_fetcher *fetcher, uint32_t field_idx, > + enum field_type field_type, struct Mem *default_val_mem, > + struct Mem *dest_mem) > @@ -2614,19 +2722,21 @@ case OP_Column: {> > +/* Opcode: Fetch P1 P2 P3 P4 P5 > + * Synopsis: r[P3]=PX > + * > + * Interpret the data that P1 points as an initialized 6. "Interpret data P1 points at as an initialized ...". > + * tuple_fetcher object. > + * > + * Fetch the P2-th column from their tuple. The value extracted 7. 'Their' is for plural. Use 'its'. > + * is stored in register P3. > + * > + * If the column contains fewer than P2 fields, then extract > + * a NULL. Or, if the P4 argument is a P4_MEM use the value of > + * the P4 argument as the result. 8. You never generate OP_Fetch with a default value. Please, drop P4 argument. As well as P5. They are never used, even in the last patch. > + * > + * Value of P5 register is an 'expected' destination value type. 9. It is not 'expected'. I can't pass here FIELD_TYPE_STRING and get a string value despite an actually stored type. It is rather a flag, that you need to transform 'int' into 'number'. Anyway, it should be dropped. > + */ > +case OP_Fetch: { > + struct tuple_fetcher *fetcher = > + (struct tuple_fetcher *) p->aMem[pOp->p1].u.p; > + uint32_t field_idx = pOp->p2; > + struct Mem *dest_mem = &aMem[pOp->p3]; > + struct Mem *default_val_mem = > + pOp->p4type == P4_MEM ? pOp->p4.pMem : NULL; > + enum field_type field_type = pOp->p5; > + memAboutToChange(p, dest_mem); > + rc = tuple_fetcher_fetch(fetcher, field_idx, field_type, > + default_val_mem, dest_mem); > + if (rc != SQL_OK) > + goto abort_due_to_error; > + UPDATE_MAX_BLOBSIZE(dest_mem); 10. Why is not this command a part of tuple_fetcher_fetch()? > + REGISTER_TRACE(p, pOp->p3, dest_mem); > + break; > +}