Hi, Sanja! See below. Just a couple of small API-related comments. And one JSON bug. On Dec 21, sanja@askmonty.org wrote:
At file:///home/bell/maria/bzr/work-maria-10.0-cassandra/
------------------------------------------------------------ revno: 3480 revision-id: sanja@askmonty.org-20121221192211-93tc13j3a6c85qea parent: sanja@montyprogram.com-20121119121604-5h5tu0zn11em0sb3 committer: sanja@askmonty.org 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