[tarantool-patches] Re: [PATCH v1 04/10] sql: create interface vstream

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch! See 4 comments below.

On 17/11/2018 17:03, imeevma@tarantool.org wrote:
> If we want to use functions from execute.h not only in IPROTO we
> should create special interface. This interface will allow us to
> create different implementations for mpstream and lua_State and
> use functions from execute.c without changing them. This patch
> creates such interface and its implementation for mpstream and
> replaces mpstream functions in execute.c by methods of this
> interface.
> 
> Needed for #3505
> ---
>   src/box/CMakeLists.txt |   1 +
>   src/box/execute.c      |  92 +++++++++--------------
>   src/box/execute.h      |   4 +-
>   src/box/iproto.cc      |  13 +++-
>   src/box/vstream.c      | 200 +++++++++++++++++++++++++++++++++++++++++++++++++
>   src/box/vstream.h      | 191 ++++++++++++++++++++++++++++++++++++++++++++++
>   6 files changed, 439 insertions(+), 62 deletions(-)
>   create mode 100644 src/box/vstream.c
>   create mode 100644 src/box/vstream.h
> > diff --git a/src/box/vstream.c b/src/box/vstream.c
> new file mode 100644
> index 0000000..d43c352
> --- /dev/null
> +++ b/src/box/vstream.c
> @@ -0,0 +1,200 @@
> +/*
> + * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY AUTHORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * AUTHORS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#include "vstream.h"
> +#include "mpstream.h"
> +#include "iproto_constants.h"
> +#include "port.h"
> +#include "xrow.h"

1. The whole this file should be merged into mpstream.c.
Vstream is an 'abstract' class, it even has no vstream.c
file. Only header.

> +
> +const struct vstream_vtab mp_vstream_vtab = {
> +	/** encode_array = */ mp_vstream_encode_array,
> +	/** encode_map = */ mp_vstream_encode_map,
> +	/** encode_uint = */ mp_vstream_encode_uint,
> +	/** encode_int = */ mp_vstream_encode_int,
> +	/** encode_float = */ mp_vstream_encode_float,
> +	/** encode_double = */ mp_vstream_encode_double,
> +	/** encode_strn = */ mp_vstream_encode_strn,
> +	/** encode_nil = */ mp_vstream_encode_nil,
> +	/** encode_bool = */ mp_vstream_encode_bool,
> +	/** encode_enum = */ mp_vstream_encode_enum,
> +	/** encode_port = */ mp_vstream_encode_port,
> +	/** encode_reply_array = */ mp_vstream_encode_reply_array,
> +	/** encode_reply_map = */ mp_vstream_encode_reply_map,
> +	/** encode_array_commit = */ mp_vstream_encode_array_commit,
> +	/** encode_reply_commit = */ mp_vstream_encode_reply_commit,
> +	/** encode_map_commit = */ mp_vstream_encode_map_commit,

2. Once you did it, please, do not wrap each mpstream function
with a one line wrapper which differs in first argument pointer type
only. Most of this functions you can assign with an explicit cast.

> +};
> +
> +void
> +mp_vstream_init(struct vstream *vstream, struct mpstream *mpstream)
> +{
> +	vstream->vtab = &mp_vstream_vtab;
> +	vstream->mpstream = mpstream;
> +	vstream->is_flatten = false;
> +}
> diff --git a/src/box/vstream.h b/src/box/vstream.h
> new file mode 100644
> index 0000000..42f9813
> --- /dev/null
> +++ b/src/box/vstream.h
> @@ -0,0 +1,191 @@
> +#ifndef TARANTOOL_VSTREAM_H_INCLUDED
> +#define TARANTOOL_VSTREAM_H_INCLUDED
> +/*
> + * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY AUTHORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * AUTHORS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#include "diag.h"
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif /* defined(__cplusplus) */
> +
> +struct vstream;
> +struct mpstream;
> +struct lua_State;
> +struct port;
> +
> +struct vstream_vtab {
> +	void (*encode_array)(struct vstream *stream, uint32_t size);
> +	void (*encode_map)(struct vstream *stream, uint32_t size);
> +	void (*encode_uint)(struct vstream *stream, uint64_t num);
> +	void (*encode_int)(struct vstream *stream, int64_t num);
> +	void (*encode_float)(struct vstream *stream, float num);
> +	void (*encode_double)(struct vstream *stream, double num);
> +	void (*encode_strn)(struct vstream *stream, const char *str,
> +			    uint32_t len);
> +	void (*encode_nil)(struct vstream *stream);
> +	void (*encode_bool)(struct vstream *stream, bool val);
> +	void (*encode_enum)(struct vstream *stream, int64_t num,
> +			    const char *str);
> +	int (*encode_port)(struct vstream *stream, struct port *port);
> +	int (*encode_reply_array)(struct vstream *stream, uint32_t size,
> +				  uint8_t key, const char *str);
> +	int (*encode_reply_map)(struct vstream *stream, uint32_t size,
> +				uint8_t key, const char *str);
> +	void (*encode_array_commit)(struct vstream *stream, uint32_t id);
> +	void (*encode_reply_commit)(struct vstream *stream);
> +	void (*encode_map_commit)(struct vstream *stream);

3. encode_reply_* functions should not exist. As I said earlier, you
can inline them as already existing encode_* functions.

> +};
> +
> +struct vstream {
> +	/** Virtual function table. */
> +	const struct vstream_vtab *vtab;
> +	/** TODO: Write comment. */
> +	union {
> +		struct mpstream *mpstream;
> +		struct lua_State *L;
> +	};
> +	/** TODO: Write comment. */
> +	bool is_flatten;

4. This flag should not be here. It is a part of sql_request and
sql_response as I remember. And it should be used in execute.c, not
inside streams. You will see how much simpler the code will be.