Copilot commented on code in PR #6831:
URL: https://github.com/apache/hive/pull/6831#discussion_r4187764279
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/SortedDynPartitionOptimizer.java:
##########
@@ -730,6 +740,21 @@ public ReduceSinkOperator getReduceSinkOp(List<Integer>
partitionPositions, List
partCols.add(allCols.get(idx).clone());
}
+ // Universally spread dynamic partition data across reducers to prevent
data skew bottlenecks.
+ // IMPORTANT: We must NOT do this if the table is explicitly bucketed
(bucketColumns is not
+ // empty), otherwise we will randomly scatter rows that belong to strict
buckets, corrupting
+ // the bucket hash!
+ if (CollectionUtils.isEmpty(bucketColumns)) {
+ ExprNodeDesc randExpr = RAND_EXPRESSION.apply(allCols);
+ partCols.add(randExpr);
+ // Append to the absolute end to avoid scrambling custom
sort/partition exprs
+ keyCols.add(randExpr);
+ orderStr += (order == 1) ? "+" : "-";
+ nullOrderStr.append(nullOrder);
+ LOG.info(
+ "SortedDynPartitionOptimizer: Injected rand() to spread partition
data across reducers.");
+ }
Review Comment:
The PR title/description says `shouldDo()` was modified to return false when
stats are missing, but the actual code change here introduces unconditional
`rand()` injection into ReduceSink partitioning/keys (for non-bucketed tables)
and updates many golden outputs accordingly. Please update the PR
title/description to match this behavior change, or remove/relocate these
changes so the PR aligns with HIVE-30101’s stated intent.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/SortedDynPartitionOptimizer.java:
##########
@@ -116,12 +117,23 @@ public class SortedDynPartitionOptimizer extends
Transform {
}
};
+ private static final Function<List<ExprNodeDesc>, ExprNodeDesc>
RAND_EXPRESSION =
+ cols -> {
+ try {
+ List<ExprNodeDesc> args = Collections.singletonList(new
ExprNodeConstantDesc(1234567L));
+ return ExprNodeGenericFuncDesc.newInstance(
+ FunctionRegistry.getFunctionInfo("rand").getGenericUDF(), args);
+ } catch (SemanticException e) {
+ throw new RuntimeException(e);
+ }
+ };
Review Comment:
`RAND_EXPRESSION` accepts `cols` but never uses it, and it converts a
`SemanticException` into a generic `RuntimeException` inside the optimizer path
(which can make failures harder to diagnose/handle consistently). Consider
changing this to a parameterless factory (e.g., a private method or
`Supplier<ExprNodeDesc>`) and either propagate `SemanticException` through the
optimizer flow or throw a more specific unchecked exception that includes
context (e.g., function name and phase).
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/SortedDynPartitionOptimizer.java:
##########
@@ -730,6 +740,21 @@ public ReduceSinkOperator getReduceSinkOp(List<Integer>
partitionPositions, List
partCols.add(allCols.get(idx).clone());
}
+ // Universally spread dynamic partition data across reducers to prevent
data skew bottlenecks.
+ // IMPORTANT: We must NOT do this if the table is explicitly bucketed
(bucketColumns is not
+ // empty), otherwise we will randomly scatter rows that belong to strict
buckets, corrupting
+ // the bucket hash!
+ if (CollectionUtils.isEmpty(bucketColumns)) {
+ ExprNodeDesc randExpr = RAND_EXPRESSION.apply(allCols);
+ partCols.add(randExpr);
+ // Append to the absolute end to avoid scrambling custom
sort/partition exprs
+ keyCols.add(randExpr);
+ orderStr += (order == 1) ? "+" : "-";
+ nullOrderStr.append(nullOrder);
Review Comment:
The sort order/null-order for the injected `rand()` key is derived from
existing local variables (`order`, `nullOrder`) rather than being explicitly
defined for `rand()`. This can produce inconsistent `orderStr`/`nullOrderStr`
semantics (e.g., inheriting descending order from a prior key) and makes the
injected key behavior dependent on surrounding logic. Recommendation:
explicitly append the intended ordering for `rand()` (typically ascending, and
a consistent null-order code even though `rand()` is non-null) rather than
reusing unrelated variables.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/SortedDynPartitionOptimizer.java:
##########
@@ -730,6 +740,21 @@ public ReduceSinkOperator getReduceSinkOp(List<Integer>
partitionPositions, List
partCols.add(allCols.get(idx).clone());
}
+ // Universally spread dynamic partition data across reducers to prevent
data skew bottlenecks.
+ // IMPORTANT: We must NOT do this if the table is explicitly bucketed
(bucketColumns is not
+ // empty), otherwise we will randomly scatter rows that belong to strict
buckets, corrupting
+ // the bucket hash!
+ if (CollectionUtils.isEmpty(bucketColumns)) {
+ ExprNodeDesc randExpr = RAND_EXPRESSION.apply(allCols);
+ partCols.add(randExpr);
+ // Append to the absolute end to avoid scrambling custom
sort/partition exprs
+ keyCols.add(randExpr);
+ orderStr += (order == 1) ? "+" : "-";
+ nullOrderStr.append(nullOrder);
+ LOG.info(
+ "SortedDynPartitionOptimizer: Injected rand() to spread partition
data across reducers.");
Review Comment:
Logging at INFO for every injected ReduceSink can be very noisy in
production (especially for multi-insert / complex plans) and may spam logs
without actionable value. Consider demoting this to DEBUG (or adding a
rate-limited/once-per-query log) and/or including key context only when
troubleshooting is enabled (e.g., table name, queryId).
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/SortedDynPartitionOptimizer.java:
##########
@@ -34,6 +34,7 @@
import java.util.stream.Collectors;
import java.util.stream.Stream;
+import org.apache.commons.collections4.CollectionUtils;
Review Comment:
This introduces a new dependency usage for a trivial null/empty check. If
commons-collections4 isn’t already a common dependency in this module, prefer a
standard `bucketColumns == null || bucketColumns.isEmpty()` check to minimize
dependency surface and keep the optimizer codebase lean.
--
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]
