Hi Alexey, Thank you very much for your review! Please see my comments below: On 05/23/2017 12:16 AM, Alexey Botchkov wrote:
Firstly I'd say i don't like the idea of the 'virtual check_arguments()' function. I belive it's more natural to have a member 'm_check_arguments_function' that would be set in constructor (or be an argument to the constructor) and the non-virtual 'check_arguments()' that would just call that member. This would make Item class structure much nicer and would even save us memory.
If you have good arguments for your approach, I have some comments/questions to your patch :)
I'm not sure that a function member is better. 1. It will look like a C fragment in a C++ code. Why is it more natural than a virtual method? 2. It will make the Item size bigger. 3. It will need more coding. Now we have to declare only virtual functions. With a function member, we'll have to do: - declare function anyway - add a code into constructors 4. It will need more CPU for m_check_arguments_function initialization. On the contrary, having a virtual methods costs nothing, except a small initialization in VMTs during mysqld startup. What is your main concern? Are you afraid that I'll have to modify class hierarchy for other Item_xxx_func? It should not be necessary. We'll just have a set of protected methods on the Item_func level, for most typical cases.
Why this change? +++ b/mysql-test/t/gis-rtree.test @@ -61,7 +61,7 @@ while ($1) let $2=10; while ($2) { - eval DELETE FROM t2 WHERE Within(g, Envelope(GeometryFromWKB(Point($1 * 10 - 9, $2 * 10 - 9), Point($1 * 10, $2 * 10)))); + eval DELETE FROM t2 WHERE Within(g, Envelope(GeometryFromWKB(Point($1 * 10 - 9, $2 * 10 - 9), 0))); SELECT count(*) FROM t2; dec $2;
It passed POINT() as the second argiment to GeometryFromWKB. The second argument is for SRID. After my changes it's not allowed.
}
It looks weird when the 'check_something' functions returns FALSE if the argument fits the check. I'd make them returning TRUE in this case as people normally expect.
We have a convention that functions return true on error and false on success. Please have a look into: check_db_name() check_for_broken_triggers() check_unique_table() They all return true on error. I did the same.
+bool Item_func::check_argument_types_scalar(uint start, uint end) const +{ + for (uint i= start; i < end; i++) + { + DBUG_ASSERT(i < arg_count);
In code like this i'd rather do bool Item_func::check_argument_types_scalar(uint start, uint end) const { DBUG_ASSERT(end < arg_count);
for (uint i= start; i < end; i++) { // no DBUG_ASSERT here ...
Agree.
Some typenames feel misleading. I'd suggest changes Item_real_func_geometry_property -> Item_real_func_arg_geometry Item_int_func_geometry_property -> Item_int_func_arg_geometry Item_bool_func_geometry_property -> Item_bool_func_arg_geometry Item_str_ascii_func_geometry_property -> Item_ascii_func_arg_geometry Item_geometry_func_geometry_property -> Item_geometry_func_arg_geometry
This looks like a property. The first argument is an object of some complex type (GEOMETRY in our case). The other arguments are simple type additional optional parameters. In C++ this would look like: geom->IsSimple(); geom->Buffer(10); OpenGIS also calls these methods "properties", so do we in the documentation: https://dev.mysql.com/doc/refman/5.7/en/gis-class-point.html
Item_binary_func_geometry_property - do we really need this type?
Right, there is only one binary string property: AsWKB(). I added Item_xxx_geometry_property for regularity (to have a symmetric class hierarchy with other "properties" ). Here's the class structure: Item_str_func Item_geometry_func Item_geometry_func_geometry_property Item_func_centroid Item_func_envelope Item_func_boundary Item_func_spatial_decomp Item_func_spatial_decomp_n Item_func_buffer Item_func_pointonsurface Item_geometry_func_importer Item_func_geometry_from_text Item_func_geometry_from_wkb Item_func_geometry_from_json Item_func_point Item_func_spatial_collection Item_func_spatial_operation Item_binary_func_geometry_property Item_func_as_wkb Item_real_func Item_real_func_geometry_property Item_func_x Item_func_y Item_func_area Item_func_glength Item_real_func_geometry_dyadic_operation Item_func_distance Item_int_func Item_int_func_geometry_property Item_func_issimple Item_func_isring Item_func_isclosed Item_func_dimension Item_func_numgeometries Item_func_numinteriorring Item_func_numpoints Item_func_srid Item_func_gis_debug Item_bool_func Item_bool_func_geometry_property Item_func_isempty Item_bool_func_geometry_dyadic_operation Item_func_spatial_relate Item_str_ascii_func Item_str_ascii_func_geometry_property Item_func_as_wkt Item_func_as_geojson Item_func_geometry_type Btw, do you need a program to print this hierarchy? :) I wrote it a few years ago, and use it almost every day.
Item_geometry_func_importer - do we need this type?
I can remove this. It does not have shared methods indeed.
Item_real_func_geometry_dyadic_operation -> Item_real_func_arg_2_geometries Item_bool_func_geometry_dyadic_operation -> Item_bool_func_arg_2_geometries
It's an operation on two geometries. My names is quite self-descriptive. Also, it tells that: - there is no any other GEOMETRY arguments - there can be optional non-GEOMETRY additional arguments
+ bool check_arguments() const + { + DBUG_ASSERT(arg_count >= 2); + return check_argument_types_or_binary(&type_handler_geometry, 0, 2);
the check_argument_types_or_binary() does the check of the arg_count so no need for the DBUG_ASSERT(). But i don't insist on removing this line :)
I can remove it :)
On Mon, May 15, 2017 at 5:56 PM, Alexander Barkov <bar@mariadb.org <mailto:bar@mariadb.org>> wrote:
Hello Alexey,
can you please review a patch for MDEV-12803?
Thanks!