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

Support NULLS FIRST/LAST in new engine #843

Conversation

dai-chen
Copy link
Member

@dai-chen dai-chen commented Nov 20, 2020

Issue #, if available: N/A

Description of changes:

  1. Change SQL grammar to add NULLS FIRST and NULLS LAST clause.
  2. Build AST Sort node with sort options given.
  3. Analyze sort options by determining default nulls order if NULLS clause absent.
  4. Change comparison test data and framework to support nulls.

TODO: Add support in window function OVER clause. Because AST window function is unresolved expression instead of operator, regular ORDER BY and that in over clause cannot share same logic.

Documentation:

  1. Add doctest: https://github.com/dai-chen/sql/blob/support-order-by-null-order-2/docs/user/dql/basics.rst#example-2-specifying-order-for-null
  2. Update README with all features only available in new engine to avoid user confusion: https://github.com/dai-chen/sql/tree/support-order-by-null-order-2#experimental

Testing:

  1. Add UT and comparison test
  2. Remove one comparison test query because it is ordering on SUM(FlightDelayMin) which is 0 for many rows and the order is not determined.
  3. Change alias names in can_build_from_subquery() because new SQL doesn't support keyword as alias name.

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 Nov 20, 2020
@dai-chen dai-chen added the SQL label Nov 20, 2020
@codecov
Copy link

codecov bot commented Nov 20, 2020

Codecov Report

Merging #843 (173911f) into develop (4ebe901) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #843   +/-   ##
==========================================
  Coverage      99.84%   99.85%           
- Complexity      2094     2102    +8     
==========================================
  Files            209      209           
  Lines           4658     4678   +20     
  Branches         301      308    +7     
==========================================
+ Hits            4651     4671   +20     
  Misses             5        5           
  Partials           2        2           
Impacted Files Coverage Δ Complexity Δ
...ndistroforelasticsearch/sql/analysis/Analyzer.java 100.00% <100.00%> (ø) 43.00 <6.00> (+5.00)
...orelasticsearch/sql/sql/parser/AstSortBuilder.java 100.00% <100.00%> (ø) 7.00 <4.00> (+3.00)
...rch/sql/sql/parser/context/QuerySpecification.java 100.00% <100.00%> (ø) 15.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ebe901...173911f. Read the comment docs.

@dai-chen dai-chen marked this pull request as ready for review November 20, 2020 23:56
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 023f65a into opendistro-for-elasticsearch:develop Nov 23, 2020
@dai-chen dai-chen deleted the support-order-by-null-order-2 branch November 23, 2020 19:17
penghuo pushed a commit that referenced this pull request Dec 15, 2020
* Change grammar

* Add ast building and UT

* Pass jacoco

* Handle nullfirst option in analyzer

* Add comparison test

* Fix broken UT

* Add comparison test with null

* Add more comparison test

* Add doctest

* Update readme with features only avaiable in new engine for clarity

* Prepare PR

* Update doc

* Update doc

* Update doc

* Update doc
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