maria-developers team mailing list archive

[Timour Katchaounov <timour@xxxxxxxxxxxx>]

Hi Sergey,

> Hello,
>
> This is to inform you that today I've pushed MWL#90 into 5.3-main. There was a
> buildbot run on 5.3-subqueries-mwl90 tree before the push, and it did not show
> any failures that weren't also present in 5.3-main.

On one hand it is good that you pushed sooner than later,
however there are quite a few things in your code I really
don't agree with. Could you please send me one full diff for
review.

Things I noticed at a first glance while merging:
- You have made more class members "public"
  (e.g. in class Item_subselect),
- There are functions that are obvious class members, for instance
  double get_post_group_estimate(JOIN* join, double join_op_rows);
- There are some functions that are non-obvious class methods,
  bool make_in_exists_conversion(THD *thd, JOIN *join, Item_in_subselect *item)
- There are functions without any comments, or functions with
  old-style comments.

Indeed, when some procedure is not designed as a method of a
proper class, but it needs some class members of various
classes, then the easiest solution is to make everything
public.

We have had several discussions in the past about the reasons
of doing proper OO design of new code. I am very disappointed
that you have ignored all these discussions without any good
technical reason. I also know that in the past Igor requested
similar changes which you also ignored.

Breaking the principles of encapsulation and proper abstraction
has proven to lead to bugs and harder to maintain code. Therefore
I am *strongly* against pushing such code.

> The result of my merge has an issue that conceptually it is not a full merge,
> because MWL#90 code is not able yet to take advantage of the possibily of
> dynamic choice between IN->EXISTS (which was introduced in MWL#89 which Timour
> pushed two weeks ago).
>
> Adding support for MWL#89 and MWL#90 working together seems easy. Actually, I
> initially intended to do it while merging, but then decided to do one thing at
> a time.
>
> My further plans are:
> - look at the open bugs I have
> - look at making MWL#89 and MWL#90 work together.

Please, please, add one extra item to your list:
- fixing design

> BR
>   Sergey

Timour