Skip to content

[CALCITE-4868] Elasticsearch adapter fails if GROUP BY is followed by ORDER BY#4874

Open
xuzifu666 wants to merge 3 commits intoapache:mainfrom
xuzifu666:4868
Open

[CALCITE-4868] Elasticsearch adapter fails if GROUP BY is followed by ORDER BY#4874
xuzifu666 wants to merge 3 commits intoapache:mainfrom
xuzifu666:4868

Conversation

@xuzifu666
Copy link
Copy Markdown
Member

@snuyanzin
Copy link
Copy Markdown
Contributor

mostly LGTM
the question I have is whether it makes sense to consider test cases with ordinals and aliases
like SqlConformance#isGroupByAlias and SqlConformance#isGroupByOrdinal, same for ORDER BY

@xuzifu666
Copy link
Copy Markdown
Member Author

mostly LGTM the question I have is whether it makes sense to consider test cases with ordinals and aliases like SqlConformance#isGroupByAlias and SqlConformance#isGroupByOrdinal, same for ORDER BY

Thanks for the review @snuyanzin . I have added additional test cases; the current set of tests should now be sufficient to verify the fix for CALCITE-4868, for the following reasons:

  1. The bug in question concerns a failure in the Elasticsearch adapter triggered specifically when an ORDER BY clause follows a GROUP BY clause.
  2. The current tests successfully verify the core functionality: that aggregation combined with ORDER BY aliases works correctly.
  3. Different SQL conformance settings primarily influence how aliases and ordinals are processed during the parsing phase, whereas the fix for the Elasticsearch adapter addresses the sorting logic during the execution phase.

@xiedeyantu
Copy link
Copy Markdown
Member

Perhaps we can add test case for GROUP BY alias.

@xuzifu666
Copy link
Copy Markdown
Member Author

Perhaps we can add test case for GROUP BY alias.

OK, had added related test case.

@sonarqubecloud
Copy link
Copy Markdown

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants