Hi, Sanja! See below. Just a couple of small API-related comments. And one JSON bug.
On Dec 21, [email protected] wrote: > At file:///home/bell/maria/bzr/work-maria-10.0-cassandra/ > > ------------------------------------------------------------ > revno: 3480 > revision-id: [email protected] > parent: [email protected] > committer: [email protected] > 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 _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
