Re: [Maria-developers] MDEV-12803 Improve function parameter data type control

Hi Alexey, Thank you very much for your review! Please see my comments below: On 05/23/2017 12:16 AM, Alexey Botchkov wrote:
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.
It passed POINT() as the second argiment to GeometryFromWKB. The second argument is for SRID. After my changes it's not allowed.
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.
Agree.
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
I can remove it :)
participants (1)
-
Alexander Barkov