Re: [Maria-developers] Rev 3480: Post review changes (total (really))

22 Dec 2012

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

    

Sergei Golubchik

tags

participants (1)