[vlc-devel] [PATCH] Prepared statements for SQL

[Laurent Aimar]

Hi,
On Tue, Oct 27, 2009, Srikanth Raju wrote:
>      > +#define VLC_SQL_ROW 1
>      > +#define VLC_SQL_DONE 2
>       What are they used for? A doxy comment would be usefull.
>    They're used as return values for sql_Run(). Documented.
>      >  What are they used for? A doxy comment would be usefull.
> 
>      > +struct sql_value_t
>      > +{
>      > +    int length;
>      > +    union
>      > +    {
>      > +        int i;
>      > +        double dbl;
>      > +        char* psz;
>      > +        void* ptr;
>      > +    } value;
>      > +};
>       You could merge it with:
>      > +typedef struct sql_value_t sql_value_t;
> 
>    I didn't because the other typedefs arent.
 The others cannot be easily merged while this one can.

>      > +struct sql_stmt_t
>      > +{
>      > +    sql_t* p_sql;
>      > +    sql_stmt_sys_t *p_sys;
>      > +};
>       I don't think that explicitly declaring sql_stmt_t is needed.
>      It seems that is only needed by the plugin implementation.
> 
>    Needed only by the plugin implementation meaning? It's required by users
>    of
>    the sql module.
 A user will already have the p_sql pointer somewhere, so it leaves only
sql_stmt_sys_t and so making sql_stmt_t opaque will replace it and is simpler.

>      > +/**
>      > + * @brief Bind arguments to a sql_stmt_t object
>      > + * @param p_sql The SQL object
>      > + * @param p_stmt Statement Object
>      > + * @param type Data type of the value
>      > + * @param p_value Value to be bound
>      > + * @param i_length For values which require a length
>      > + * @param i_pos Position at which the parameter should be bound
>      > + * @return VLC_SUCCESS or VLC_EGENERIC
>      > + */
>      > +static inline int sql_BindGeneric( sql_t* p_sql, sql_stmt_t* p_stmt,
>      > +        sql_type_e type, sql_value_t* p_value, int i_pos )
>       I think using int instead of sql_type_e is better for binary
>      compatibility.
> 
>    Type safety?
 The problem with sql_type_e is that the size of the storage of an enum is 
compiler dependant and so the ABI would easily be broken.
 Now, I don't feel strongly about it, anyone cares to comment ?

>      > +#define sql_GetIntegerColumn( A, B, C, D ) sql_GetColumn( A, B, C,
>      SQL_INT, D )
>      > +#define sql_GetDoubleColumn( A, B, C, D ) sql_GetColumn( A, B, C,
>      SQL_DOUBLE, D )
>      > +#define sql_GetTextColumn( A, B, C, D ) sql_GetColumn( A, B, C,
>      SQL_TEXT, D )
>      > +#define sql_GetBlobColumn( A, B, C, D ) sql_GetColumn( A, B, C,
>      SQL_BLOB, D )
>       I think more advanced wrapper would be helpful, they could completly
>      hide sql_value_t.
> 
>     
>    I'm not sure why one would want to hide the type of the value, because
>    that was one of the reasons the prepared statement interface is being
>    included. So that there may be stronger type checking.
 That's why better wrappers will help.
 Something like that for example:
static inline int sql_GetColumnInteger( sql_t* p_sql, sql_stmt_t* p_stmt, int i_col, int *pi_res)
{
 sql_value_t v;
 if(sql_GetColumn(..., &v))
  return VLC_EGENERIC;
 *pi_res = v.i_int;
 return VLC_SUCCESS;
}
 It will make sure that you don't do a sql_GetColumnInteger(&v) and then uses
v.dbl for example.

> +    /** Create a statement object */
> +    sql_stmt_t* (*pf_prepare) ( sql_t* p_sql, const char* psz_fmt,
> +                                int i_length );
 Please, change the psz_ prefix into p_ to avoid confusion as it is not always
a 0 terminated string.

> +    /** Bind parameters to a statement */
> +    int (*pf_bind) ( sql_t* p_sql, sql_stmt_t* p_stmt, sql_type_e type,
> +                             sql_value_t* p_value, int i_pos );
 How is p_value used ? As an input or as a pointer where the output value
will be stored ? (If as an input, a const is probably needed).

> +
> +    /** Get the datatype for a specified column */
> +    int (*pf_gettype) ( sql_t* p_sql, sql_stmt_t* p_stmt, int i_col,
> +                        sql_type_e* type );
> +
> +    /** Get the data from a specified column */
> +    int (*pf_getcolumn) ( sql_t* p_sql, sql_stmt_t* p_stmt, int i_col,
> +                          sql_type_e type, sql_value_t *p_res );
>  };
 As a general remark, I don't think the order of the arguments of
pf_bind/pf_gettype/pf_getcolumn are consistant (or I missed the logic).

Regards,

-- 
fenrir