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

Sort field push down #848

Merged

Conversation

penghuo
Copy link
Contributor

@penghuo penghuo commented Nov 23, 2020

New added features.

  1. Add the fitler-sort push down optimizaiton rule in core engine. Rule: PushFilterUnderSort
    e.g. Filter - Sort - AnyChild will be optimized as Sort - Filter - AnyChild
  2. Add the sort operator push down on Elasticseach storage engine. Rule: MergeSortAndIndexScan and MergeSortAndIndexRelation
    e.g. Sort - Relation will be optimized as IndexScan(sort)

Limitation

Currently, the storage engine layer optimzation depend on the Elasticsearch DSL to push down to sort expression. But there are some limitations of Elasticsearch DSL.

  • Regardless the sort order (ASC/DESC), the doc has the field with NULL value always come before the doc missing the field.
  • The script based sort can’t handle MISSING/NULL value
  • The script based sort doesn’t support MISSING/NULL first/last paramater.

Thus, currently, the Sort Operator will push down to Elasticsearch query only when the sort by list include fields only. if the Sort operator include sort expression, the optimization will do nothing, it will still fall back to default memory based implementation.
e.g. source=index | sort abs(a) the expression abs(a) can't be push down to Elasticsearch DSL.

Changes

  1. Remove the count from PPL sort command.
  2. Remove the count from LogicalSort/PhysicalSort

Todo

  1. Aggregation with Sort.

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

@penghuo penghuo self-assigned this Nov 23, 2020
@penghuo penghuo requested review from dai-chen and chloe-zh November 23, 2020 23:13
@penghuo penghuo added the enhancement New feature or request label Nov 23, 2020
@codecov
Copy link

codecov bot commented Nov 23, 2020

Codecov Report

Merging #848 (f537a5e) into develop (023f65a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #848   +/-   ##
==========================================
  Coverage      99.85%   99.85%           
- Complexity      2102     2115   +13     
==========================================
  Files            209      214    +5     
  Lines           4678     4729   +51     
  Branches         308      308           
==========================================
+ Hits            4671     4722   +51     
  Misses             5        5           
  Partials           2        2           
Impacted Files Coverage Δ Complexity Δ
...endistroforelasticsearch/sql/executor/Explain.java 100.00% <ø> (ø) 27.00 <0.00> (ø)
...elasticsearch/sql/planner/logical/LogicalSort.java 100.00% <ø> (ø) 2.00 <0.00> (ø)
...ch/sql/planner/optimizer/LogicalPlanOptimizer.java 100.00% <ø> (ø) 7.00 <0.00> (ø)
...tor/protector/ElasticsearchExecutionProtector.java 100.00% <ø> (ø) 16.00 <0.00> (ø)
...ical/ElasticsearchLogicalPlanOptimizerFactory.java 100.00% <ø> (ø) 1.00 <0.00> (ø)
...arch/storage/script/filter/FilterQueryBuilder.java 100.00% <ø> (ø) 10.00 <0.00> (ø)
...troforelasticsearch/sql/ppl/parser/AstBuilder.java 100.00% <ø> (ø) 33.00 <0.00> (ø)
...orelasticsearch/sql/ppl/utils/ArgumentFactory.java 98.38% <ø> (-0.17%) 28.00 <0.00> (-4.00)
...orelasticsearch/sql/sql/parser/AstSortBuilder.java 100.00% <ø> (ø) 7.00 <0.00> (ø)
...ndistroforelasticsearch/sql/analysis/Analyzer.java 100.00% <100.00%> (ø) 43.00 <0.00> (ø)
... and 21 more

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 023f65a...f537a5e. Read the comment docs.

Copy link
Member

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@penghuo penghuo merged commit 42d79c2 into opendistro-for-elasticsearch:develop Nov 24, 2020
penghuo added a commit that referenced this pull request Dec 15, 2020
* init

* remove the count option from sort

* add filter push down rule

* fix compile issue

* add integ-test

* update
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants