maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #05021
Re: Rev 3480: Post review changes (total (really))
Hi, Sanja!
See below. Just a couple of small API-related comments.
And one JSON bug.
On Dec 21, sanja@xxxxxxxxxxxx wrote:
> At file:///home/bell/maria/bzr/work-maria-10.0-cassandra/
>
> ------------------------------------------------------------
> revno: 3480
> revision-id: sanja@xxxxxxxxxxxx-20121221192211-93tc13j3a6c85qea
> parent: sanja@xxxxxxxxxxxxxxxx-20121119121604-5h5tu0zn11em0sb3
> committer: sanja@xxxxxxxxxxxx
> branch nick: work-maria-10.0-cassandra
> timestamp: Fri 2012-12-21 21:22:11 +0200
> message:
> Post review changes (total (really)).
>
> === modified file 'include/ma_dyncol.h'
> --- a/include/ma_dyncol.h 2012-09-28 12:27:16 +0000
> +++ b/include/ma_dyncol.h 2012-12-21 19:22:11 +0000
> @@ -38,12 +38,13 @@
> how the offset are stored.
> */
> #define MAX_DYNAMIC_COLUMN_LENGTH 0X1FFFFFFFL
> +#define MAX_DYNAMIC_COLUMN_LENGTH_NM 0XFFFFFFFFFL
Hm. Two questions.
1. Where does the number come from?
2. You've introduced a constant, but you aren't using it anywhere.
What's the point?
> /*
> Limits of implementation
> */
> -#define MAX_NAME_LENGTH 255
> #define MAX_TOTAL_NAME_LENGTH 65535
> +#define MAX_NAME_LENGTH (MAX_TOTAL_NAME_LENGTH/4)
>
> /* NO and OK is the same used just to show semantics */
> #define ER_DYNCOL_NO ER_DYNCOL_OK
> @@ -100,6 +101,8 @@ struct st_dynamic_column_value
>
> typedef struct st_dynamic_column_value DYNAMIC_COLUMN_VALUE;
>
> +/* old functions (deprecated) */
I usually prefer code to comments. Kind of a comment that a compiler
can make use of. That means an assert(aaa == 0) instead of /* aaa should
be 0 here */ and instead of comment above, I'd rather have
__attribute__((deprecated))
> +#ifdef MADYNCOL_DEPRECATED
> enum enum_dyncol_func_result
> dynamic_column_create(DYNAMIC_COLUMN *str,
> uint column_nr, DYNAMIC_COLUMN_VALUE *value);
> @@ -133,73 +121,105 @@ dynamic_column_update_many(DYNAMIC_COLUM
...
> enum enum_dyncol_func_result
> -dynamic_column_exists_fmt(DYNAMIC_COLUMN *str, void *key, my_bool string_keys);
> +dynamic_column_get(DYNAMIC_COLUMN *org, uint column_nr,
> + DYNAMIC_COLUMN_VALUE *store_it_here);
> +#endif
>
> -/* List of not NULL columns */
> +/* new functions */
> enum enum_dyncol_func_result
> -dynamic_column_list(DYNAMIC_COLUMN *org, DYNAMIC_ARRAY *array_of_uint);
> +mariadb_dyncol_create_many(DYNAMIC_COLUMN *str,
> + uint column_count,
> + uint *column_numbers,
> + DYNAMIC_COLUMN_VALUE *values,
> + my_bool new_string);
> enum enum_dyncol_func_result
> -dynamic_column_list_str(DYNAMIC_COLUMN *str, DYNAMIC_ARRAY *array_of_lexstr);
> +mariadb_dyncol_create_many_named(DYNAMIC_COLUMN *str,
> + uint column_count,
> + LEX_STRING *column_keys,
> + DYNAMIC_COLUMN_VALUE *values,
> + my_bool new_string);
> +
> +
> enum enum_dyncol_func_result
> -dynamic_column_list_fmt(DYNAMIC_COLUMN *str, DYNAMIC_ARRAY *array, my_bool string_keys);
> +mariadb_dyncol_update_many(DYNAMIC_COLUMN *str,
> + uint add_column_count,
> + uint *column_keys,
> + DYNAMIC_COLUMN_VALUE *values);
> +enum enum_dyncol_func_result
> +mariadb_dyncol_update_many_named(DYNAMIC_COLUMN *str,
> + uint add_column_count,
> + LEX_STRING *column_keys,
> + DYNAMIC_COLUMN_VALUE *values);
> +
> +
> +enum enum_dyncol_func_result
> +mariadb_dyncol_exists(DYNAMIC_COLUMN *org, uint column_nr);
> +enum enum_dyncol_func_result
> +mariadb_dyncol_exists_named(DYNAMIC_COLUMN *str, LEX_STRING *name);
> +
> +/* List of not NULL columns */
> +enum enum_dyncol_func_result
> +mariadb_dyncol_list(DYNAMIC_COLUMN *org, DYNAMIC_ARRAY *array_of_uint);
Sanja, let's be constistent. If mariadb_dyncol_list_named() returns an
array of LEX_STRING's, than mariadb_dyncol_list should return an array
of integers, not a dynamic array.
Besides you don't use DYNAMIC_ARRAY *anywhere* else in the new (not
deprecated) API.
> +enum enum_dyncol_func_result
> +mariadb_dyncol_list_named(DYNAMIC_COLUMN *str, uint *count, LEX_STRING **names);
>
> /*
> if the column do not exists it is NULL
> */
> enum enum_dyncol_func_result
> -dynamic_column_get(DYNAMIC_COLUMN *org, uint column_nr,
> +mariadb_dyncol_get(DYNAMIC_COLUMN *org, uint column_nr,
> DYNAMIC_COLUMN_VALUE *store_it_here);
> enum enum_dyncol_func_result
> -dynamic_column_get_str(DYNAMIC_COLUMN *str, LEX_STRING *name,
> - DYNAMIC_COLUMN_VALUE *store_it_here);
> +mariadb_dyncol_get_named(DYNAMIC_COLUMN *str, LEX_STRING *name,
> + DYNAMIC_COLUMN_VALUE *store_it_here);
>
> -my_bool dynamic_column_has_names(DYNAMIC_COLUMN *str);
> +my_bool mariadb_dyncol_has_names(DYNAMIC_COLUMN *str);
>
> enum enum_dyncol_func_result
> -dynamic_column_check(DYNAMIC_COLUMN *str);
> +mariadb_dyncol_check(DYNAMIC_COLUMN *str);
>
> enum enum_dyncol_func_result
> -dynamic_column_json(DYNAMIC_COLUMN *str, DYNAMIC_STRING *json);
> +mariadb_dyncol_json(DYNAMIC_COLUMN *str, DYNAMIC_STRING *json);
>
> #define dynamic_column_initialize(A) memset((A), 0, sizeof(*(A)))
> #define dynamic_column_column_free(V) dynstr_free(V)
>
> /* conversion of values to 3 base types */
> enum enum_dyncol_func_result
> -dynamic_column_val_str(DYNAMIC_STRING *str, DYNAMIC_COLUMN_VALUE *val,
> +mariadb_dyncol_val_str(DYNAMIC_STRING *str, DYNAMIC_COLUMN_VALUE *val,
> CHARSET_INFO *cs, my_bool quote);
> enum enum_dyncol_func_result
> -dynamic_column_val_long(longlong *ll, DYNAMIC_COLUMN_VALUE *val);
> +mariadb_dyncol_val_long(longlong *ll, DYNAMIC_COLUMN_VALUE *val);
> +enum enum_dyncol_func_result
> +mariadb_dyncol_val_double(double *dbl, DYNAMIC_COLUMN_VALUE *val);
> +
> +
> enum enum_dyncol_func_result
> -dynamic_column_val_double(double *dbl, DYNAMIC_COLUMN_VALUE *val);
> +mariadb_dyncol_unpack(DYNAMIC_COLUMN *str,
> + uint *count,
> + LEX_STRING **names, DYNAMIC_COLUMN_VALUE **vals);
>
> +int mariadb_dyncol_column_cmp_named(const LEX_STRING *s1, const LEX_STRING *s2);
> enum enum_dyncol_func_result
> -dynamic_column_vals(DYNAMIC_COLUMN *str,
> - DYNAMIC_ARRAY *names, DYNAMIC_ARRAY *vals,
> - char **free_names);
> +mariadb_dyncol_column_count(DYNAMIC_COLUMN *str, uint *column_count);
>
> /***************************************************************************
> Internal functions, don't use if you don't know what you are doing...
> ***************************************************************************/
>
> -#define dynamic_column_reassociate(V,P,L, A) dynstr_reassociate((V),(P),(L),(A))
> +#define mariadb_dyncol_reassociate(V,P,L, A) dynstr_reassociate((V),(P),(L),(A))
I'd remove it. You can use dynstr_reassociate() directly in
Item_func_dyncol_create::val_str(), if you need it, but don't encourage
others to do it, by pussing this macro here.
> -#define dynamic_column_value_init(V) (V)->type= DYN_COL_NULL
> +#define dyncol_value_init(V) (V)->type= DYN_COL_NULL
1. you forgot the mariadb_ prefix
2. This macro seems to be unused. Perhaps you can simply delete it?
> === modified file 'mysql-test/r/dyncol.result'
> --- a/mysql-test/r/dyncol.result 2012-09-28 11:01:17 +0000
> +++ b/mysql-test/r/dyncol.result 2012-12-21 19:22:11 +0000
> @@ -1577,3 +1587,53 @@ COLUMN_CHECK('')
> SELECT COLUMN_CHECK(NULL);
> COLUMN_CHECK(NULL)
> NULL
> +#
> +# escaping check
> +#
> +select column_json(column_create("string", "'\"/\\`.,whatever")),hex(column_create("string", "'\"/\\`.,whatever"));
> +column_json(column_create("string", "'\"/\\`.,whatever")) hex(column_create("string", "'\"/\\`.,whatever"))
> +[{"string":"'\"/\\`.,whatever"}] 040100060000000300737472696E670827222F5C602E2C7768617465766572
Please, add a test with two values.
What it will look like:
[{"a":1},{"b":b}] ?
or
{ "a" : 1, "b" : 2 } ?
I don't see a reason why it should be an array of one-value objects,
instead of one object, with many values. But perhaps I'm missing
something, why did you prefer an array?
> +#
> +# embedding test
> +#
> +select column_json(column_create("val", "val", "emb", column_create("val2", "val2")));
> +column_json(column_create("val", "val", "emb", column_create("val2", "val2")))
> +[{"emb":[{"val2":"val2"}],{"val":"val"}]
ouch. invalid json, curly braces aren't properly paired.
> +select column_json(column_create(1, "val", 2, column_create(3, "val2")));
> +column_json(column_create(1, "val", 2, column_create(3, "val2")))
> +[{"1":"val"},{"2":[{"3":"val2"}]]
same here.
> === modified file 'sql/item_func.h'
> --- a/sql/item_func.h 2012-09-27 23:06:56 +0000
> +++ b/sql/item_func.h 2012-12-21 19:22:11 +0000
> @@ -58,7 +58,7 @@ public:
> NOW_FUNC, TRIG_COND_FUNC,
> SUSERVAR_FUNC, GUSERVAR_FUNC, COLLATE_FUNC,
> EXTRACT_FUNC, CHAR_TYPECAST_FUNC, FUNC_SP, UDF_FUNC,
> - NEG_FUNC, GSYSVAR_FUNC };
> + NEG_FUNC, GSYSVAR_FUNC, DYNCOL };
DYNCOL_FUNC
> enum optimize_type { OPTIMIZE_NONE,OPTIMIZE_KEY,OPTIMIZE_OP, OPTIMIZE_NULL,
> OPTIMIZE_EQUAL };
> enum Type type() const { return FUNC_ITEM; }
Regards,
Sergei
