maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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@xxxxxxxxxxx
> <mailto:bar@xxxxxxxxxxx>> wrote:
> 
>     Hello Alexey,
> 
>     can you please review a patch for MDEV-12803?
> 
>     Thanks!
> 
>