query planner regression: Invalid use of group function

derekperkins commented 8 months ago

Overview of the Issue

We have a query that was using the v3 query planner comment directive that broke when upgrading to v18, with that removal. Vitess isn't handling an ambiguous column alias in the ORDER BY clause the same way MySQL does.

vttablet: rpc error: code = InvalidArgument desc = Invalid use of group function (errno 1111) (sqlstate HY000)

Possibly related issue:

https://github.com/vitessio/vitess/issues/11641

Reproduction Steps

Unsharded keyspace

create table tbl1(id bigint, group_id bigint, deadline_at timestamp, primary key(id));

select /*vt+ PLANNER=v3 */
group_id,
from_unixtime(min(deadline_at)) as deadline_at
from tbl1
group by group_id
order by min(deadline_at)
limit 100;

The query above works if you change the column alias to deadline_at2 or anything that doesn't shadow the original column name.

Binary Version

All components v18.0.2

Operating System and Environment details

GKE v1.27

Log Fragments

No response

systay commented 7 months ago

EDIT: This is wrong. Read the next comment.

Hi @derekperkins!

Thanks for the writeup. I don't think the problem here is with the ambigous column names, since in the ORDER BY you are wrapping the column with MIN. I think the problem is that MySQL has much better support for functional dependencies than Vitess has. https://dev.mysql.com/doc/refman/8.2/en/group-by-functional-dependence.html

Thanks to this, MySQL can see that {min(deadline_at)} -> {from_unixtime(min(deadline_at))}, to borrow the notation from that page.

This is not going to be straight forward to implement. As a work-around, I'm pretty sure that changing the query to use order by deadline_at will give you the same results and not be a problem for the column binder.

systay commented 7 months ago

Hmmm... Reading and experimenting more, I think you were actually totally correct in your assessment of the issue. Working on solving it in https://github.com/vitessio/vitess/pull/14935

systay commented 7 months ago

derekperkins commented 7 months ago

Thanks @systay! We changed the query alias to make it work when I reported it, so we're in no rush

vitessio / vitess