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

Add Flow control function IF(expr1, expr2, expr3) #990

Merged
merged 9 commits into from Jan 25, 2021

Conversation

harold-wang
Copy link
Contributor

Issue #861 , if available:

Description of changes:

Add flow control function IF(expr1, expr2, expr3)

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 14, 2021

Codecov Report

Merging #990 (f25cd5d) into develop (6899363) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #990   +/-   ##
==========================================
  Coverage      99.87%   99.87%           
- Complexity      2400     2406    +6     
==========================================
  Files            234      234           
  Lines           5517     5524    +7     
  Branches         357      357           
==========================================
+ Hits            5510     5517    +7     
  Misses             5        5           
  Partials           2        2           
Impacted Files Coverage Δ Complexity Δ
...opendistroforelasticsearch/sql/expression/DSL.java 100.00% <100.00%> (ø) 133.00 <1.00> (+1.00)
...h/sql/expression/function/BuiltinFunctionName.java 100.00% <100.00%> (ø) 3.00 <0.00> (ø)
...ion/operator/predicate/UnaryPredicateOperator.java 100.00% <100.00%> (ø) 28.00 <8.00> (+5.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 6899363...f25cd5d. Read the comment docs.

@dai-chen
Copy link
Member

@harold-wang harold-wang requested review from penghuo and removed request for penghuo and chloe-zh January 15, 2021 15:44
Comment on lines 111 to 114
List<SerializableFunction<FunctionName, org.apache.commons.lang3.tuple.Pair<FunctionSignature,
FunctionBuilder>>> functionsTwo = typeList.stream().map(v ->
impl((UnaryPredicateOperator::exprIf), v, UNKNOWN, v, v))
.collect(Collectors.toList());
Copy link
Member

Choose a reason for hiding this comment

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

Same as my previous question for line 130-133, why is this required?

Copy link
Contributor

Choose a reason for hiding this comment

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

could we consider the solution which could for case...when and if. i think they both control flow statement which may not fit into our function defintion.

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 idea.

@harold-wang harold-wang merged commit 43f41bd into opendistro-for-elasticsearch:develop Jan 25, 2021
@harold-wang
Copy link
Contributor Author

Todo issue: merge if function with case when function.

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.

None yet

3 participants