Skip to content
This repository has been archived by the owner on Jan 28, 2021. It is now read-only.

Try to order by function #692

Merged
merged 1 commit into from
Apr 26, 2019
Merged

Try to order by function #692

merged 1 commit into from
Apr 26, 2019

Conversation

kuba--
Copy link
Contributor

@kuba-- kuba-- commented Apr 23, 2019

Signed-off-by: kuba-- kuba@sourced.tech

Fixes #691

Is there any better place to resolve this functions/expressions for order by?
So far I resolve order by with literals which exist in schema.

@kuba-- kuba-- added the bug Something isn't working label Apr 23, 2019
@kuba-- kuba-- requested a review from a team April 23, 2019 20:42
engine_test.go Outdated
@@ -201,6 +201,20 @@ var queries = []struct {
{int64(1), "third row"},
},
},
{
`SELECT COUNT(*), fi FROM (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you rewrite this as SELECT fi, COUNT(*) FROM ( to see if it still works?
COUNT in orderby receives a row in Eval (which should be the buffer) and since the first item is the result of the count, it will work. But it won't if you change the order.

engine_test.go Outdated
@@ -442,6 +456,14 @@ var queries = []struct {
{float64(4), int64(3)},
},
},
{
"SELECT SUM(i), i FROM mytable GROUP BY i ORDER BY SUM(i) DESC",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I've added more tests ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and it still works

Copy link
Contributor

@erizocosmico erizocosmico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the following test case?

SELECT s FROM mytable GROUP BY s ORDER BY COUNT(*)

@kuba--
Copy link
Contributor Author

kuba-- commented Apr 24, 2019

This one of course doesn't work because we don't have COUNT in projection so we'll get:
OrderBy does not support aggregation expressions
The same as you do:

SELECT s, 1 + COUNT(*) FROM mytable GROUP BY s ORDER BY COUNT(*) + 1

or

SELECT s, COUNT(*) as cnt FROM mytable GROUP BY s ORDER BY COUNT(*)

@erizocosmico
Copy link
Contributor

erizocosmico commented Apr 24, 2019

But that one should work. Order by should to the same as having does and push down expressions to the group by if needed. Rebase against the HAVING branch, so you can test this correctly because you will come across this issue: When HAVING pushes down, it creates a Project node on top of it. You will need to either project that as well and create another project on top of order by or move that project before the order by node.

Basically:

SELECT i FROM mytable GROUP BY i HAVING COUNT(*) > 0 ORDER BY SUM(i)

Will generate:

OrderBy(SUM(i))
|- Project(i)
 |- Having(`count(*)` > 0)
  |- GroupBy(i, count(*))
    |- Table(i)

And your rule should convert this to:

Project(i)
|- OrderBy(`sum(i)`)
 |- Having(`count(*)` > 0)
  |- GroupBy(i, count(*), sum(i))
    |- Table(i)

Perhaps instead of looking for a Project with a Having child a better, and more general, solution would be to change resolve_having rule to not wrap Having node with a project, but the whole node if the schema changed from the initial node.

Signed-off-by: kuba-- <kuba@sourced.tech>
@kuba--
Copy link
Contributor Author

kuba-- commented Apr 25, 2019

@erizocosmico Would be great if we can generalize some rules (especially for group/orderBy), but I think it will require a little bit more refactoring and I'm afraid of destabilization.
So far, this little trick solved the problem.

One important thing regarding tests - I also changed how do we compare results!
Currently in testQueryWithContext we call require.ElementsMatch(expected, rows) because for query results ordering can be random (doesn't matter). But it's not the case for ORDER BY queries. For ORDER BY we must call require.Equal.
Current workaround just checks if query string contains ORDER BY.
In other words, so far all ORDER BY queries pass, does not matter what order was returned and what was expected.

@ajnavarro ajnavarro merged commit c660bf1 into src-d:master Apr 26, 2019
@kuba-- kuba-- deleted the fix-691/order-by branch April 26, 2019 10:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unable to cast "<repository name>" of type string to int64
4 participants