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

Support Struct Data Query In SQL/PPL #1018

Merged
merged 11 commits into from
Jan 30, 2021

Conversation

penghuo
Copy link
Contributor

@penghuo penghuo commented Jan 28, 2021

Issue #, if available: #795

Description of changes:

  1. Support Struct Data Query In SQL/PPL. https://github.com/penghuo/sql/blob/object-query-pr/docs/experiment/ppl/general/datatypes.rst#query-struct-data-types
  2. Add the Paths in ReferenceExpression.

To Reviewer

There are 2 ITs in legacy engine have been disabled.

  1. ObjectFieldSelectIT.testSelectObjectFieldOfArrayValuesItself, the issue is tracking by [Enhancement] Handle the multivalue field properly #1017
  2. PrettyFormatResponseIT. the issue is tracking by Disable access to the field keyword in the new SQL engine. #1019

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

@codecov
Copy link

codecov bot commented Jan 28, 2021

Codecov Report

Merging #1018 (8004a80) into develop (5c19cca) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #1018   +/-   ##
==========================================
  Coverage      99.87%   99.87%           
- Complexity      2406     2410    +4     
==========================================
  Files            234      234           
  Lines           5526     5540   +14     
  Branches         357      358    +1     
==========================================
+ Hits            5519     5533   +14     
  Misses             5        5           
  Partials           2        2           
Impacted Files Coverage Δ Complexity Δ
...orelasticsearch/sql/sql/parser/AstSortBuilder.java 100.00% <ø> (ø) 7.00 <0.00> (ø)
...elasticsearch/sql/analysis/ExpressionAnalyzer.java 100.00% <100.00%> (ø) 31.00 <1.00> (-2.00)
...relasticsearch/sql/analysis/QualifierAnalyzer.java 100.00% <100.00%> (ø) 8.00 <1.00> (+2.00)
...orelasticsearch/sql/data/model/ExprTupleValue.java 100.00% <100.00%> (ø) 19.00 <2.00> (ø)
...stroforelasticsearch/sql/data/model/ExprValue.java 100.00% <100.00%> (ø) 20.00 <1.00> (+1.00)
...sticsearch/sql/expression/ReferenceExpression.java 100.00% <100.00%> (ø) 9.00 <5.00> (+5.00)
...rch/sql/storage/bindingtuple/LazyBindingTuple.java 100.00% <100.00%> (ø) 1.00 <1.00> (ø)
...troforelasticsearch/sql/utils/ExpressionUtils.java 100.00% <100.00%> (ø) 2.00 <1.00> (+1.00)
...troforelasticsearch/sql/ppl/parser/AstBuilder.java 100.00% <100.00%> (ø) 33.00 <0.00> (ø)
...ticsearch/sql/ppl/parser/AstExpressionBuilder.java 100.00% <100.00%> (ø) 28.00 <4.00> (ø)
... and 2 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 5c19cca...8004a80. Read the comment docs.

@penghuo penghuo requested review from dai-chen and chloe-zh January 28, 2021 21:04
@penghuo penghuo marked this pull request as ready for review January 28, 2021 22:12
@penghuo penghuo self-assigned this Jan 29, 2021
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.

Just minor comments, thanks for the changes!

@@ -49,4 +69,44 @@ public ExprType type() {
public String toString() {
return attr;
}

/**
* Resolve the ExprValue from {@link ExprTupleValue} using paths.*
Copy link
Member

Choose a reason for hiding this comment

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

np: is paths.* intentional or a typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

typo. removed.

return resolve(value, paths);
}

private ExprValue resolve(ExprValue value, List<String> paths) {
Copy link
Member

Choose a reason for hiding this comment

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

Can this / do we need to handle project.year is another object field? ex. project.year.month

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. add more cases in doc and UT.
We don't expect object filed name include "." which is not supported now.

@penghuo penghuo merged commit 1dbf244 into opendistro-for-elasticsearch:develop Jan 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants