soumyakanti3578 commented on code in PR #5505:
URL: https://github.com/apache/hive/pull/5505#discussion_r1874072247
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveFilterSortTransposeRule.java:
##########
@@ -22,31 +22,36 @@
import org.apache.calcite.rel.RelNode;
import org.apache.hadoop.hive.ql.optimizer.calcite.HiveCalciteUtil;
import org.apache.hadoop.hive.ql.optimizer.calcite.reloperators.HiveFilter;
+import
org.apache.hadoop.hive.ql.optimizer.calcite.reloperators.HiveSortExchange;
import org.apache.hadoop.hive.ql.optimizer.calcite.reloperators.HiveSortLimit;
import com.google.common.collect.ImmutableList;
-public class HiveFilterSortTransposeRule extends RelOptRule {
+import static java.util.Collections.singletonList;
- public static final HiveFilterSortTransposeRule INSTANCE =
- new HiveFilterSortTransposeRule();
+public class HiveFilterSortTransposeRule<T extends RelNode> extends RelOptRule
{
+
+ public static final HiveFilterSortTransposeRule<HiveSortLimit>
SORT_LIMIT_INSTANCE =
+ new HiveFilterSortTransposeRule<>(HiveSortLimit.class);
+ public static final HiveFilterSortTransposeRule<HiveSortExchange>
SORT_EXCHANGE_INSTANCE =
+ new HiveFilterSortTransposeRule<>(HiveSortExchange.class);
//~ Constructors -----------------------------------------------------------
/**
* Creates a HiveFilterSortTransposeRule.
*/
- private HiveFilterSortTransposeRule() {
+ private HiveFilterSortTransposeRule(Class<T> clazz) {
Review Comment:
Here `T` is unbounded. Do you think it is beneficial to limit it's scope if
possible?
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java:
##########
@@ -5611,4 +5365,282 @@ private enum TableType {
JDBC
}
+ class OrderByRelBuilder {
Review Comment:
This class is only used in `CalcitePlanner` - probably we can make it
`private`? Also some of its methods are missing access modifiers.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveProjectSortExchangeTransposeRule.java:
##########
@@ -88,6 +88,11 @@ public void onMatch(RelOptRuleCall call) {
RelCollation newCollation =
RelCollationTraitDef.INSTANCE.canonize(RelCollationImpl.of(fieldCollations));
List<Integer> newDistributionKeys = getNewRelDistributionKeys(project,
sortExchange.getDistribution());
+ if (newDistributionKeys == null) {
Review Comment:
Can `newDistributionKeys` be `isEmpty()`, and do we need to handle it
specifically?
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java:
##########
@@ -520,6 +510,26 @@ private void convertFieldCollationsToASTNode(
hiveAST.order = orderAst;
}
+ private ASTNode convertSortKey(RelNode node, Schema schema, Map<Integer,
RexNode> obRefToCallMap, int fieldIndex) {
+ RexNode obExpr = null;
+ if (obRefToCallMap != null) {
+ obExpr = obRefToCallMap.get(fieldIndex);
Review Comment:
Since `obRefToCallMap` is only used to extract `obExpr`, maybe we can only
pass `obExpr` to the method instead of the Map?
And if you do that, then we could also just pass the `ColumnInfo` and get
rid of `fieldIndex`? What do you think?
--
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]
