You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "zabetak (via GitHub)" <gi...@apache.org> on 2025/11/25 10:33:48 UTC
[PR] HIVE-29339: Remove DISTINCT indicator from SqlAggFunctions [hive]
zabetak opened a new pull request, #6212:
URL: https://github.com/apache/hive/pull/6212
### What changes were proposed in this pull request?
1. Drop CanAggregateDistinct and refactor dependent code accordingly.
2. Remove isDistinct indicator from all classes extending SqlAggFunction.
3. Move the part handling window functions from SqlFunctionConverter#buildAST to ASTConverter
4. Generalize the generation of TOK_FUNCTIONSTAR for aggregate functions by exploiting SqlOperator#getSqlSyntax
5. Replace CalciteUDAF with SqlBasicAggFunction.create since the former does not bring any additional info (operandTypeInference is removed but it is not used anyways from Hive).
### Why are the changes needed?
1. Simpler and more generic code
2. Better code coverage
3. Easier maintenance (calcite upgrades, new rules, extensions, etc.)
For full motivation check HIVE-29339
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing tests and in particular those around aggregate and window functions.
```
mvn test -Dtest=TestMiniLlapLocalCliDriver -Dqfile_regex=".*ptf.*
mvn test -Dtest=TestMiniLlapLocalCliDriver -Dqfile_regex=".*window.*
```
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29339: Remove DISTINCT indicator from SqlAggFunctions [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6212:
URL: https://github.com/apache/hive/pull/6212#issuecomment-3575215458
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6212) **Quality Gate passed**
Issues
 [33 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6212&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6212&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6212&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6212&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6212&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6212)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29339: Remove DISTINCT indicator from SqlAggFunctions [hive]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #6212:
URL: https://github.com/apache/hive/pull/6212#discussion_r2623450652
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java:
##########
@@ -1000,7 +1001,18 @@ public ASTNode visitOver(RexOver over) {
}
// 1. Translate the UDAF
- final ASTNode wUDAFAst = visitCall(over);
+ ASTNode wUDAFAst = ASTBuilder.createAST(HiveParser.TOK_FUNCTION, "TOK_FUNCTION");
Review Comment:
Added some examples in https://github.com/apache/hive/pull/6212/commits/3e7c15d1e996363af461f45d1918ec2088c805b1
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29339: Remove DISTINCT indicator from SqlAggFunctions [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6212:
URL: https://github.com/apache/hive/pull/6212#issuecomment-3667344318
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6212) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6212&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [2 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6212&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6212&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6212&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6212&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6212)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29339: Remove DISTINCT indicator from SqlAggFunctions [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6212:
URL: https://github.com/apache/hive/pull/6212#issuecomment-3669417760
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6212) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6212&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [2 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6212&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6212&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6212&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6212&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6212)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29339: Remove DISTINCT indicator from SqlAggFunctions [hive]
Posted by "soumyakanti3578 (via GitHub)" <gi...@apache.org>.
soumyakanti3578 commented on code in PR #6212:
URL: https://github.com/apache/hive/pull/6212#discussion_r2579076838
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java:
##########
@@ -1000,7 +1001,18 @@ public ASTNode visitOver(RexOver over) {
}
// 1. Translate the UDAF
- final ASTNode wUDAFAst = visitCall(over);
+ ASTNode wUDAFAst = ASTBuilder.createAST(HiveParser.TOK_FUNCTION, "TOK_FUNCTION");
+ if (over.getOperands().isEmpty() && over.op.getSyntax() == SqlSyntax.FUNCTION_STAR) {
+ wUDAFAst = ASTBuilder.createAST(HiveParser.TOK_FUNCTIONSTAR, "TOK_FUNCTIONSTAR");
+ }
+ if (over.isDistinct()) {
+ wUDAFAst = ASTBuilder.createAST(HiveParser.TOK_FUNCTIONDI, "TOK_FUNCTIONDI");
+ }
+ wUDAFAst.addChild(ASTBuilder.createAST(HiveParser.Identifier, over.op.getName()));
+ wUDAFAst.setTypeInfo(TypeConverter.convert(over.type));
+ for (RexNode operand : over.getOperands()) {
+ wUDAFAst.addChild(operand.accept(this));
+ }
Review Comment:
Adding a few lines of comments explaining this part would be nice as it has changed significantly.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29339: Remove DISTINCT indicator from SqlAggFunctions [hive]
Posted by "soumyakanti3578 (via GitHub)" <gi...@apache.org>.
soumyakanti3578 commented on code in PR #6212:
URL: https://github.com/apache/hive/pull/6212#discussion_r2579094372
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java:
##########
@@ -1000,7 +1001,18 @@ public ASTNode visitOver(RexOver over) {
}
// 1. Translate the UDAF
- final ASTNode wUDAFAst = visitCall(over);
+ ASTNode wUDAFAst = ASTBuilder.createAST(HiveParser.TOK_FUNCTION, "TOK_FUNCTION");
Review Comment:
Maybe add a few lines of comments to explain what it is doing, but not strictly required.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java:
##########
@@ -1000,7 +1001,18 @@ public ASTNode visitOver(RexOver over) {
}
// 1. Translate the UDAF
- final ASTNode wUDAFAst = visitCall(over);
+ ASTNode wUDAFAst = ASTBuilder.createAST(HiveParser.TOK_FUNCTION, "TOK_FUNCTION");
+ if (over.getOperands().isEmpty() && over.op.getSyntax() == SqlSyntax.FUNCTION_STAR) {
+ wUDAFAst = ASTBuilder.createAST(HiveParser.TOK_FUNCTIONSTAR, "TOK_FUNCTIONSTAR");
+ }
+ if (over.isDistinct()) {
+ wUDAFAst = ASTBuilder.createAST(HiveParser.TOK_FUNCTIONDI, "TOK_FUNCTIONDI");
+ }
+ wUDAFAst.addChild(ASTBuilder.createAST(HiveParser.Identifier, over.op.getName()));
+ wUDAFAst.setTypeInfo(TypeConverter.convert(over.type));
+ for (RexNode operand : over.getOperands()) {
+ wUDAFAst.addChild(operand.accept(this));
+ }
Review Comment:
Adding a few lines of comments explaining this part would be nice as it has changed significantly.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/jdbc/JDBCAggregationPushDownRule.java:
##########
@@ -62,8 +62,7 @@ public boolean matches(RelOptRuleCall call) {
SqlAggFunction f = relOptRuleOperand.getAggregation();
if (f instanceof HiveSqlCountAggFunction) {
//count distinct with more that one argument is not supported
- HiveSqlCountAggFunction countAgg = (HiveSqlCountAggFunction)f;
- if (countAgg.isDistinct() && 1 < relOptRuleOperand.getArgList().size()) {
+ if (relOptRuleOperand.isDistinct() && 1 < relOptRuleOperand.getArgList().size()) {
Review Comment:
nit: If you are changing this line, can we flip the second predicate so that the constant is on the right side? Makes this more readable in my opinion.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29339: Remove DISTINCT indicator from SqlAggFunctions [hive]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #6212:
URL: https://github.com/apache/hive/pull/6212#discussion_r2623448915
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/jdbc/JDBCAggregationPushDownRule.java:
##########
@@ -62,8 +62,7 @@ public boolean matches(RelOptRuleCall call) {
SqlAggFunction f = relOptRuleOperand.getAggregation();
if (f instanceof HiveSqlCountAggFunction) {
//count distinct with more that one argument is not supported
- HiveSqlCountAggFunction countAgg = (HiveSqlCountAggFunction)f;
- if (countAgg.isDistinct() && 1 < relOptRuleOperand.getArgList().size()) {
+ if (relOptRuleOperand.isDistinct() && 1 < relOptRuleOperand.getArgList().size()) {
Review Comment:
Makes sense.
Fixed in https://github.com/apache/hive/pull/6212/commits/a64e4b3190c5708ec39ddec697c0ed129d3698a1
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29339: Remove DISTINCT indicator from SqlAggFunctions [hive]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on PR #6212:
URL: https://github.com/apache/hive/pull/6212#issuecomment-3674313138
Many thanks for the review @soumyakanti3578 !
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29339: Remove DISTINCT indicator from SqlAggFunctions [hive]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak merged PR #6212:
URL: https://github.com/apache/hive/pull/6212
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29339: Remove DISTINCT indicator from SqlAggFunctions [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6212:
URL: https://github.com/apache/hive/pull/6212#issuecomment-3661384684
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6212) **Quality Gate passed**
Issues
 [6 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6212&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6212&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6212&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6212&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6212&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6212)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org