Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Surprising behavior of ORDER BY ALL in some cases #59151

Closed
2 tasks
rschu1ze opened this issue Jan 24, 2024 · 4 comments · Fixed by #59450 or #59462
Closed
2 tasks

Surprising behavior of ORDER BY ALL in some cases #59151

rschu1ze opened this issue Jan 24, 2024 · 4 comments · Fixed by #59450 or #59462

Comments

@rschu1ze
Copy link
Member

  • SELECT * FROM order_by_all ORDER BY all should work but it does not
  • To reduce ambiguity, we should unconditionally (independently of setting enable_order_by_all) throw an error when ALL (case-insensitive: also all) appears in ORDER BY together with other columns/aliases/expressions. Right now, SELECT a, b, all FROM order_by_all ORDER BY all, a works and all loses its special meaning.

Also see #57875 (comment)

@rschu1ze
Copy link
Member Author

@zhongyuankai In case you want to check, if you don't have time I can also do it later.

@zhongyuankai
Copy link
Contributor

@rschu1ze Okay, I'll fix that.

@zhongyuankai
Copy link
Contributor

SELECT * FROM order_by_all ORDER BY all should work but it does not

@rschu1ze Hello, how can I reproduce it? I understand that the exception caused by all with the column name is what we expect, https://fiddle.clickhouse.com/2c29cfe2-3d7c-4447-8d72-b88ccb59e9fc

  • To reduce ambiguity, we should unconditionally (independently of setting enable_order_by_all) throw an error when ALL (case-insensitive: also all) appears in ORDER BY together with other columns/aliases/expressions. Right now, SELECT a, b, all FROM order_by_all ORDER BY all, a works and all loses its special meaning.

Does it mean we don’t need enable_order_by_all setting?, Both SELECT a, b, all FROM order_by_all ORDER BY all and SELECT a, b, all FROM order_by_all ORDER BY ALL need to be replaced by select expressions.

@rschu1ze
Copy link
Member Author

@rschu1ze Hello, how can I reproduce it? I understand that the exception caused by all with the column name is what we expect, https://fiddle.clickhouse.com/2c29cfe2-3d7c-4447-8d72-b88ccb59e9fc

You are right. I missed that in your example and my repro, we run SELECT * [...] ORDER BY ALL on a table which contains a column all. Indeed, an exception is expected in this case. Let's keep this behavior.

I guess I originally meant a slight modification of the example (here, note that the table has no all column), where an unexpected exception is thrown with the analyzer enabled. The same example works without analyzer. It would be great if you could check this one.

Does it mean we don’t need enable_order_by_all setting?, Both SELECT a, b, all FROM order_by_all ORDER BY all and SELECT a, b, all FROM order_by_all ORDER BY ALL need to be replaced by select expressions.

I actually thought further about the ambiguity between ORDER BY keyword ALL and column name all. Interestingly, no such ambiguity exists for positional arguments (ORDER BY 2, 1) because column names must not be numbers (i.e. CREATE tab (1 String, [...]) is illegal). What about this:

  • We completely disallow using ORDER BY ALL (or ORDER BY all) when one of the columns/aliases/expressions in the SELECT clause is all (or a case-insensitive variant). Basically throw an exception in this case.
  • This will obviously remove all ambiguities and we also no longer need setting enable_order_by_all.
  • I would consider columns named all an edge case. If a table nevertheless has a column named all and the user really really really wants to use ORDER BY ALL, then one can use this syntax: SELECT all AS all_alias, col2, col3 FROM [..] ORDER BY ALL, i.e. we force the usage of an alias as a workaround. What will no longer work (but that is fine) is this: SELECT * FROM [...] ORDER BY ALL where the table has a column called all and because of * we can't define an alias.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment