Hello Alexey, Thanks for your feedback. Sending a new version, as discussed in this email and on IRC. Please find attached. - Removed Item_geometry_func_importer - Renamed Item_xxx_geometry_property to Item_xxx_args_geometry - Renamed Item_xxx_dyadic_operation to Item_xxx_args_geometry_geometry. This is the new hierarchy: Item_str_func Item_geometry_func Item_geometry_func_args_geometry 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_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_args_geometry Item_func_as_wkb Item_real_func Item_real_func_args_geometry Item_func_x Item_func_y Item_func_area Item_func_glength Item_real_func_args_geometry_geometry Item_func_distance Item_long_func Item_long_func_args_geometry 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_bool_func Item_bool_func_args_geometry Item_func_isempty Item_bool_func_args_geometry_geometry Item_func_spatial_relate Item_str_ascii_func Item_str_ascii_func_args_geometry Item_func_as_wkt Item_func_as_geojson Item_func_geometry_type Thanks! On 05/26/2017 11:50 AM, Alexey Botchkov wrote:
1. It will look like a C fragment in a C++ code. Why is it more natural than a virtual method?
The Item hierarchy and 'check_type' hierarchy are parallel. So it seems natural to me if we can assign an already-prepared parameter-validation function to the type.
2. It will make the Item size bigger. But it will make Item** virtual tables smaller.
4. It will need more CPU for m_check_arguments_function initialization. Must be really small overhead.
Are you afraid that I'll have to modify class hierarchy? Yes. My only concern is that the class hierarchy becomes more complicated and with repeating code.
But after a while i sort of used to it so now i'm fine with the virtual method.
We have a convention that functions return true on error and false on success. For the check_something function returning TRUE or FALSE doesn't mean an 'error'. It's just the condition fits or doesn't. But fine, our code has many examples of 'check' functions returning results this way.
Typenames discussed. So otherwise i'm ok with the patch.
Best regards. HF
On Wed, May 24, 2017 at 4:22 PM, Alexander Barkov <bar@mariadb.org <mailto:bar@mariadb.org>> wrote:
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 <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> > <mailto:bar@mariadb.org <mailto:bar@mariadb.org>>> wrote: > > Hello Alexey, > > can you please review a patch for MDEV-12803? > > Thanks! > >