-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add least and greatest functions #8100
Conversation
e254f1e
to
72fc7d6
Compare
Codecov Report
@@ Coverage Diff @@
## master #8100 +/- ##
============================================
+ Coverage 64.71% 71.31% +6.59%
+ Complexity 4306 4302 -4
============================================
Files 1572 1624 +52
Lines 82006 84108 +2102
Branches 12330 12591 +261
============================================
+ Hits 53071 59978 +6907
+ Misses 25166 20024 -5142
- Partials 3769 4106 +337
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
return Double.max(a, b); | ||
} | ||
|
||
@Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this deprecation warning will not appear to users of sql, so probably we need a release note or a doc update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just remove the deprecation and tolerate some duplication. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should deprecate them as they are not standard sql. Let's add some release note in the PR description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
min/max were never actually possible to use in SQL because they clashed with the aggregation function. Ingestion transforms aren't specified in SQL, so I'm not sure they really need to be SQL compliant.
.../org/apache/pinot/core/operator/transform/function/TupleSelectionTransformFunctionsTest.java
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java
Outdated
Show resolved
Hide resolved
return Double.max(a, b); | ||
} | ||
|
||
@Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should deprecate them as they are not standard sql. Let's add some release note in the PR description
...a/org/apache/pinot/core/operator/transform/function/SelectTupleElementTransformFunction.java
Outdated
Show resolved
Hide resolved
...a/org/apache/pinot/core/operator/transform/function/SelectTupleElementTransformFunction.java
Outdated
Show resolved
Hide resolved
...a/org/apache/pinot/core/operator/transform/function/SelectTupleElementTransformFunction.java
Outdated
Show resolved
Hide resolved
218aaa5
to
c3816b7
Compare
c291d5c
to
885b010
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with minor comments
Closes #8085
Closes #8084
Parameter type promotion logic is as follows: