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

Support HAVING in new SQL engine #798

Conversation

dai-chen
Copy link
Member

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

Issue #, if available:

Description of changes:

  1. Change SQL grammar and build Filter AST node to integrate with analyzer in core engine. (To replace alias in HAVING condition, add a new AstHavingFilterBuilder which extends AstExpressionBuilder so it doesn't need to replace and clone conditions later)
  2. Optimize filter conditions in analyzer because now there may be aggregator in condition that needs to be replaced with a reference.

Documentation: https://github.com/dai-chen/sql/blob/support-having-in-new-sql-engine/docs/user/dql/aggregations.rst#having-clause

Testing: Add new UT and comparison IT.

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 added the SQL label Oct 23, 2020
@dai-chen dai-chen self-assigned this Oct 23, 2020
@dai-chen dai-chen marked this pull request as ready for review October 26, 2020 23:38
@dai-chen dai-chen requested review from penghuo and chloe-zh October 26, 2020 23:38
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 6015604 into opendistro-for-elasticsearch:develop Oct 27, 2020
@dai-chen dai-chen deleted the support-having-in-new-sql-engine branch October 27, 2020 22:48
penghuo pushed a commit that referenced this pull request Dec 15, 2020
* Change grammar and ast builder with UT

* Optimize condition expression in filter

* Support alias

* Add comparison test

* Support having without group by

* Add doctest

* Prepare PR

* Fix doctest
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.

3 participants