Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Support ORDER BY in new SQL engine #782

Conversation

dai-chen
Copy link
Member

@dai-chen dai-chen commented Oct 16, 2020

Issue #, if available:

  1. Error when the order by is by function that is not included into select list for aggregation query #123: Order by aggregate function even if it's not in SELECT
  2. order by is not working as expected when more than one group by is used #674: Order by inner field of group by
  3. Aggregation with Sort doesn't work as expected #277: Order by aggregate function
  4. Support sort on aggregation function without using alias #250: similar as above
  5. Query with order by aggregation function fails to return jdbc format output #423: similar as above
  6. Group By Oder by * SQL ERROR #402: similar as above

Description of changes:

  1. Relax QualifiedName to UnresolvedExpression in Field so we can support order by any expression for SQL.
  2. Add AstSortBuilder that shares replaceIfAliasOrOrdinal() logic with AstAggregationBuilder.
  3. Improve test framework to support comparison test for ORDER BY queries.

With these changes, we can support ORDER BY field, scalar function, aggregate function, alias and ordinal. Please see the documentation below for more details.

Documentation: https://github.com/dai-chen/sql/blob/support-order-by-in-new-sql-engine/docs/user/dql/basics.rst#example-3-ordering-by-aggregate-functions

Testing:

  1. Add new UT and comparison test for basic cases
  2. Add comparison tests for issues above for regression test
  3. Bypass legacy OrderIT and dateFunctionNameCaseInsensitiveTest(): the former tests explain output and the latter tests DATE_FORMAT which has different signature in legacy and new engine.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dai-chen dai-chen self-assigned this Oct 16, 2020
@dai-chen dai-chen added the SQL label Oct 16, 2020
@dai-chen dai-chen marked this pull request as ready for review October 16, 2020 00:29
Copy link
Contributor

@penghuo penghuo left a comment

Choose a reason for hiding this comment

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

Thanks for the change.

@dai-chen dai-chen merged commit 1c49217 into opendistro-for-elasticsearch:develop Oct 20, 2020
@dai-chen dai-chen deleted the support-order-by-in-new-sql-engine branch October 20, 2020 19:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants