You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "tanclary (via GitHub)" <gi...@apache.org> on 2023/05/17 21:52:52 UTC
[GitHub] [calcite] tanclary opened a new pull request, #3210: [CALCITE-5564] Add parsing and validation for PERCENTILE_CONT/PERCENTILE_DISC functions (enabled in BigQuery)
tanclary opened a new pull request, #3210:
URL: https://github.com/apache/calcite/pull/3210
Calcite currently supports parsing and validation for the standard PERCENTILE_CONT and PERCENTILE_DISC functions. BigQuery has their own form of these functions with different syntax (further explained on [CALCITE-5564](https://issues.apache.org/jira/browse/CALCITE-5564). This PR allows for the BigQuery syntax to be parsed and validated as well.
I tried to test these changes as thoroughly as I could given that the actual implementation has not be done yet (that is the task of [CALCITE-4666](https://issues.apache.org/jira/browse/CALCITE-4666).
Any comments, suggestions, or questions are welcome as always.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] tanclary commented on pull request #3210: [CALCITE-5564] Add parsing and validation for PERCENTILE_CONT/PERCENTILE_DISC functions (enabled in BigQuery)
Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on PR #3210:
URL: https://github.com/apache/calcite/pull/3210#issuecomment-1558709335
> looks good. a few cosmetic changes required.
>
> did you add tests to big-query.iq? If you can't execute the queries, at least you could validate them using `!type`.
>
> If you need examples, you should use `emp` and `sal` and `deptno`. It's a lot more meaningful than `unnest(array ...)`.
Hey @julianhyde , I amended my commit to include most of your fixes. The only one I couldn't quite do was your suggestion to add tests to big-query.iq. Even just checking the type throws the following error:
`Unable to get aggregate implementation for aggregate PERCENTILE_CONT in window context`
Of course this makes sense given that there is no aggregate implementation, but if there's a workaround for me to still include these tests please let me know.
Thanks for the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3210: [CALCITE-5564] Add parsing and validation for PERCENTILE_CONT/PERCENTILE_DISC functions (enabled in BigQuery)
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3210:
URL: https://github.com/apache/calcite/pull/3210#issuecomment-1569169877
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3210)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3210&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3210&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3210&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3210&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3210&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3210&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3210&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3210&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3210&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3210&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3210&resolved=false&types=CODE_SMELL) [10 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3210&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3210&metric=new_coverage&view=list) [90.5% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3210&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3210&metric=new_duplicated_lines_density&view=list) [16.5% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3210&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] asfgit closed pull request #3210: [CALCITE-5564] Add parsing and validation for PERCENTILE_CONT/PERCENTILE_DISC functions (enabled in BigQuery)
Posted by "asfgit (via GitHub)" <gi...@apache.org>.
asfgit closed pull request #3210: [CALCITE-5564] Add parsing and validation for PERCENTILE_CONT/PERCENTILE_DISC functions (enabled in BigQuery)
URL: https://github.com/apache/calcite/pull/3210
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a diff in pull request #3210: [CALCITE-5564] Add parsing and validation for PERCENTILE_CONT/PERCENTILE_DISC functions (enabled in BigQuery)
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3210:
URL: https://github.com/apache/calcite/pull/3210#discussion_r1198012023
##########
core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java:
##########
@@ -593,6 +593,48 @@ private boolean hasFractionalPart(BigDecimal bd) {
}
};
+ public static final SqlSingleOperandTypeChecker NUMERIC_UNIT_INTERVAL_NUMERIC_LITERAL =
Review Comment:
add javadoc. also add a blank line after the declaration.
##########
core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties:
##########
@@ -131,6 +131,7 @@ AggregateInWithinGroupIllegal=WITHIN GROUP must not contain aggregate expression
AggregateInWithinDistinctIllegal=WITHIN DISTINCT must not contain aggregate expression
AggregateMissingWithinGroupClause=Aggregate expression ''{0}'' must contain a WITHIN GROUP clause
WithinGroupClauseIllegalInAggregate=Aggregate expression ''{0}'' must not contain a WITHIN GROUP clause
+percentileFunctionsArgumentLimit=Percentile functions may only have 1 or 2 arguments
AggregateIllegalInOrderBy=Aggregate expression is illegal in ORDER BY clause of non-aggregating SELECT
Review Comment:
s/p/P/
##########
site/_docs/reference.md:
##########
@@ -2836,6 +2836,8 @@ Dialect-specific aggregate functions.
| b | LOGICAL_OR(condition) | Synonym for `SOME`
| s | MAX_BY(value, comp) | Synonym for `ARG_MAX`
| s | MIN_BY(value, comp) | Synonym for `ARG_MIN`
+| b | PERCENTILE_CONT(value, fraction) | 2-argument synonym for the standard `PERCENTILE_CONT` that does not use WITHIN GROUP clause and allows for null treatment
+| b | PERCENTILE_DISC(value, fraction) | 2-argument synonym for the standard `PERCENTILE_DISC` that does not use WITHIN GROUP clause and allows for null treatment
Review Comment:
does it make sense to add `[ RESPECT NULLS ]` to the syntax?
is it possible to concisely show the mapping, e.g. that `PERCENTILE_CONT(value, fraction)` is a synonym for `PERCENTILE_CONT(fraction) WITHIN GROUP (ORDER BY value)` (or whatever)
##########
core/src/main/codegen/templates/Parser.jj:
##########
@@ -4056,6 +4056,59 @@ SqlCall StringAggFunctionCall() :
}
}
+/**
Review Comment:
please include some brief example syntax.
##########
testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java:
##########
@@ -748,6 +748,50 @@ protected static SortedSet<String> keywords(@Nullable String dialect) {
.fails("(?s)Encountered \"\\*\" at .*");
}
+ @Test void testPercentileCont() {
+ sql("select percentile_cont(.5) within group (order by 3) from t")
+ .ok("SELECT PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY 3)\n"
+ +
Review Comment:
'+' should be indented
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -6139,27 +6139,35 @@ private SqlNode navigationInDefine(SqlNode node, String alpha) {
}
if (op.isPercentile()) {
Review Comment:
This code is now sufficiently complex that a couple of examples would be useful.
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6540,6 +6540,64 @@ void testGroupExpressionEquivalenceParams() {
.fails("'PERCENTILE_DISC' requires precisely one ORDER BY key");
}
+ @Test void testPercentileFunctionsBigQuery() {
+ final SqlOperatorTable opTable = operatorTableFor(SqlLibrary.BIG_QUERY);
Review Comment:
the examples over `unnest(array)` are appropriate for `big-query.iq` but are not typical of actual usage. for these tests can you switch to queries that use `emp`. Compute the percentile of sal of each employee within their department and within the whole org.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org