Storm

Code review comment for lp:~jamesh/storm/bug-242813

bug-242813
Merge into trunk

Revision history for this message

James Henstridge (jamesh) wrote on 2009-10-09:

> The tests pass, and this fixes a problem I've run into, so I'm eager to see
> this pushed into mainline.
>
> A few questions:
>
> Should there be a test to explicitly check that heterogeneous set expressions
> are not flattened? (ie exercising the negative branch of the second if
> statement)

Yep. There should be a test for that.

> Is it necessary to skip expressions containing an order_by? If the order_by is
> the same on each expr, shouldn't the result of the nested and un-nested
> expressions be equivalent? (I may very well be totally wrong about this, but I
> can't seem to cook up a counter-example at the moment)

I guess I wrote the code that way to be on the safe side: except in the case of MySQL's broken query parser, this branch is mainly designed to improve performance.

That said, the test can probably go. If the outer set expression has an order, then the inner set expression's ordering doesn't matter. If the outer expression has no order, then there is no guarantee on the ordering so we should be able to ignore it in that case too.

I'll remove the check.

> It appears that in addition to addressing
> https://bugs.edge.launchpad.net/storm/+bug/242813 this patch also should fix
> some of https://bugs.edge.launchpad.net/storm/+bug/309276 (which is a problem
> I'm having), however I don't think it addresses it fully. The proposed
> solution doesn't tackle heterogeneous nested set queries, which will still
> fail on MySQL. http://bugs.mysql.com/bug.php?id=3347
> http://bugs.mysql.com/bug.php?id=2435 and
> http://bugs.mysql.com/bug.php?id=7360 seem to imply that there is a problem
> with MySQL that basically makes some such nested queries impossible to
> express, however simpler ones like (SELECT ...) UNION (SELECT ...) INTERSECT
> (SELECT ...) are supported in MySQL, but (I believe) can't be achieved through
> Storm itself.

It might be possible to fix up that sort of chaining by playing with the operator precedences in the expression compiler.

> Could/should there be a more graceful failure for these cases than a MySQL
> parse error? (Sorry if this is too far off track. Perhaps this discussion
> would be better continued on the appropriate bug? I've subscribed myself)

I don't think it is worth the trouble. Writing code to detect expressions that will cause the MySQL query parser trouble is probably as much work as adjusting our expression generators to make expressions that don't trigger bugs in the MySQL query parser.

> The tests pass, and this fixes a problem I've run into, so I'm eager to see
> this pushed into mainline.
> 
> A few questions:
> 
> Should there be a test to explicitly check that heterogeneous set expressions
> are not flattened? (ie exercising the negative branch of the second if
> statement)

Yep.  There should be a test for that.

I guess I wrote the code that way to be on the safe side: except in the case of MySQL's broken query parser, this branch is mainly designed to improve performance.

That said, the test can probably go.  If the outer set expression has an order, then the inner set expression's ordering doesn't matter.  If the outer expression has no order, then there is no guarantee on the ordering so we should be able to ignore it in that case too.

I'll remove the check.

It might be possible to fix up that sort of chaining by playing with the operator precedences in the expression compiler.

I don't think it is worth the trouble.  Writing code to detect expressions that will cause the MySQL query parser trouble is probably as much work as adjusting our expression generators to make expressions that don't trigger bugs in the MySQL query parser.

« Back to merge proposal