zabetak commented on code in PR #5505:
URL: https://github.com/apache/hive/pull/5505#discussion_r1925215923
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/opconventer/HiveSortExchangeVisitor.java:
##########
@@ -50,25 +55,45 @@ OpAttr visit(HiveSortExchange exchangeRel) throws
SemanticException {
}
RelDistribution distribution = exchangeRel.getDistribution();
- if (distribution.getType() != Type.HASH_DISTRIBUTED) {
- throw new SemanticException("Only hash distribution supported for
LogicalExchange");
+ ArrayList<ExprNodeDesc> partitionKeyList;
Review Comment:
nit: consider using `List` instead of `ArrayList`
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/opconventer/HiveSortExchangeVisitor.java:
##########
@@ -50,25 +55,45 @@ OpAttr visit(HiveSortExchange exchangeRel) throws
SemanticException {
}
RelDistribution distribution = exchangeRel.getDistribution();
- if (distribution.getType() != Type.HASH_DISTRIBUTED) {
- throw new SemanticException("Only hash distribution supported for
LogicalExchange");
+ ArrayList<ExprNodeDesc> partitionKeyList;
+ if (distribution.getType() == Type.HASH_DISTRIBUTED) {
+ partitionKeyList = new
ArrayList<>(exchangeRel.getDistribution().getKeys().size());
+ for (int index = 0; index <
exchangeRel.getDistribution().getKeys().size(); index++) {
+ RexInputRef keyRef =
exchangeRel.getCluster().getRexBuilder().makeInputRef(
+ exchangeRel.getInput(),
exchangeRel.getDistribution().getKeys().get(index));
+ partitionKeyList.add(HiveOpConverterUtils.convertToExprNode(
+ keyRef,
+ exchangeRel.getInput(), inputOpAf.tabAlias,
inputOpAf.vcolsInCalcite));
+ }
+ } else if (distribution.getType() != Type.ANY) {
+ throw new SemanticException("Only hash distribution supported for
HiveSortExchange");
Review Comment:
The exception is a bit misleading since in now we support the ANY
distribution in `HiveSortExchange`. The code may be easier to follow if we
refactor as follows:
```java
} else if (distribution.getType() == Type.ANY) {
partitionKeyList = Collections.emptyList();
} else {
throw new SemanticException(distribution.getType()+" distribution is not
supported for HiveSortExchange");
}
```
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/opconventer/HiveSortExchangeVisitor.java:
##########
@@ -50,25 +55,45 @@ OpAttr visit(HiveSortExchange exchangeRel) throws
SemanticException {
}
RelDistribution distribution = exchangeRel.getDistribution();
- if (distribution.getType() != Type.HASH_DISTRIBUTED) {
- throw new SemanticException("Only hash distribution supported for
LogicalExchange");
+ ArrayList<ExprNodeDesc> partitionKeyList;
+ if (distribution.getType() == Type.HASH_DISTRIBUTED) {
+ partitionKeyList = new
ArrayList<>(exchangeRel.getDistribution().getKeys().size());
+ for (int index = 0; index <
exchangeRel.getDistribution().getKeys().size(); index++) {
+ RexInputRef keyRef =
exchangeRel.getCluster().getRexBuilder().makeInputRef(
+ exchangeRel.getInput(),
exchangeRel.getDistribution().getKeys().get(index));
+ partitionKeyList.add(HiveOpConverterUtils.convertToExprNode(
+ keyRef,
+ exchangeRel.getInput(), inputOpAf.tabAlias,
inputOpAf.vcolsInCalcite));
+ }
+ } else if (distribution.getType() != Type.ANY) {
+ throw new SemanticException("Only hash distribution supported for
HiveSortExchange");
+ } else {
+ partitionKeyList = new ArrayList<>(0);
Review Comment:
If we don't plan to modify the list then its safer ad more efficient to use
`Collections.emptyList()`.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/opconventer/HiveSortExchangeVisitor.java:
##########
@@ -50,25 +55,45 @@ OpAttr visit(HiveSortExchange exchangeRel) throws
SemanticException {
}
RelDistribution distribution = exchangeRel.getDistribution();
- if (distribution.getType() != Type.HASH_DISTRIBUTED) {
- throw new SemanticException("Only hash distribution supported for
LogicalExchange");
+ ArrayList<ExprNodeDesc> partitionKeyList;
+ if (distribution.getType() == Type.HASH_DISTRIBUTED) {
+ partitionKeyList = new
ArrayList<>(exchangeRel.getDistribution().getKeys().size());
+ for (int index = 0; index <
exchangeRel.getDistribution().getKeys().size(); index++) {
Review Comment:
We can use a `foreach` instead of a classic for-loop to improve readability.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
