okumin commented on code in PR #5245:
URL: https://github.com/apache/hive/pull/5245#discussion_r1597554417
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/opconventer/HiveGBOpConvUtil.java:
##########
@@ -1028,32 +1028,65 @@ private static OpAttr genReduceSideGB1NoMapGB(OpAttr
inputOpAf, GBInfo gbInfo,
List<ExprNodeDesc> reduceValues = rs.getConf().getValueCols();
ArrayList<AggregationDesc> aggregations = new ArrayList<AggregationDesc>();
int udafColStartPosInOriginalGB = gbInfo.gbKeys.size();
- // the positions in rsColInfoLst are as follows
- // --grpkey--,--distkey--,--values--
- // but distUDAF may be before/after some non-distUDAF,
- // i.e., their positions can be mixed.
- // so for all UDAF we first check to see if it is groupby key, if not is
it distinct key
- // if not it should be value
- Map<Integer, List<ExprNodeDesc>> indexToParameter = new TreeMap<>();
+
+ final List<List<ColumnInfo>> paramColInfoTable = new
ArrayList<>(gbInfo.udafAttrs.size());
+ final List<List<String>> distinctColumnNameTable = new
ArrayList<>(gbInfo.udafAttrs.size());
+ final Map<ColumnInfo, String> distinctColumnMapping = new HashMap<>();
for (int i = 0; i < gbInfo.udafAttrs.size(); i++) {
- UDAFAttrs udafAttr = gbInfo.udafAttrs.get(i);
- ArrayList<ExprNodeDesc> aggParameters = new ArrayList<ExprNodeDesc>();
+ final UDAFAttrs udafAttr = gbInfo.udafAttrs.get(i);
+ final List<ColumnInfo> paramColInfo = new
ArrayList<>(udafAttr.udafParams.size());
+ final List<String> distinctColNames = new
ArrayList<>(udafAttr.udafParams.size());
- ColumnInfo rsUDAFParamColInfo;
- ExprNodeDesc udafParam;
- ExprNodeDesc constantPropDistinctUDAFParam;
for (int j = 0; j < udafAttr.udafParams.size(); j++) {
- int argPos = getColInfoPos(udafAttr.udafParams.get(j), gbInfo);
- rsUDAFParamColInfo = rsColInfoLst.get(argPos);
- String rsUDAFParamName = rsUDAFParamColInfo.getInternalName();
+ final int argPos = getColInfoPos(udafAttr.udafParams.get(j), gbInfo);
+ final ColumnInfo rsUDAFParamColInfo = rsColInfoLst.get(argPos);
+ paramColInfo.add(rsUDAFParamColInfo);
+ final String distinctColumnName;
if (udafAttr.isDistinctUDAF && lastReduceKeyColName != null) {
- rsUDAFParamName = Utilities.ReduceField.KEY.name() + "." +
lastReduceKeyColName
+ distinctColumnName = Utilities.ReduceField.KEY.name() + "." +
lastReduceKeyColName
+ ":" + numDistinctUDFs + "." +
SemanticAnalyzer.getColumnInternalName(j);
+ distinctColumnMapping.putIfAbsent(rsUDAFParamColInfo,
distinctColumnName);
+ } else {
+ distinctColumnName = null;
+ }
+ distinctColNames.add(distinctColumnName);
+ }
+
+ paramColInfoTable.add(paramColInfo);
+ distinctColumnNameTable.add(distinctColNames);
+
+ if(udafAttr.isDistinctUDAF) {
+ numDistinctUDFs++;
+ }
+ }
+
+ // the positions in rsColInfoLst are as follows
+ // --grpkey--,--distkey--,--values--
+ // but distUDAF may be before/after some non-distUDAF,
+ // i.e., their positions can be mixed.
+ // so for all UDAF we first check to see if it is groupby key, if not is
it distinct key
+ // if not it should be value
+ final Map<Integer, List<ExprNodeDesc>> indexToParameter = new TreeMap<>();
+ for (int i = 0; i < paramColInfoTable.size(); i++) {
+ final ArrayList<ExprNodeDesc> aggParameters = new ArrayList<>();
+
+ for (int j = 0; j < paramColInfoTable.get(i).size(); j++) {
+ final ColumnInfo rsUDAFParamColInfo = paramColInfoTable.get(i).get(j);
+
+ final String rsUDAFParamName;
+ if (distinctColumnNameTable.get(i).get(j) != null) {
+ rsUDAFParamName = distinctColumnNameTable.get(i).get(j);
+ } else if (distinctColumnMapping.get(rsUDAFParamColInfo) != null) {
+ // This UDAF is not labeled with DISTINCT, but it refers to a
DISTINCT key.
+ // The original internal name is already obsolete as any DISTINCT
keys are renamed.
+ rsUDAFParamName = distinctColumnMapping.get(rsUDAFParamColInfo);
+ } else {
+ rsUDAFParamName = rsUDAFParamColInfo.getInternalName();
Review Comment:
As far as I tested, `getInternalName` retains `KEY.col0:{index of RS
keys}.col0`. It conflicts with the name generated by the following logic, and a
non-distinct UDAF can refer to a wrong column.
```
distinctColumnName = Utilities.ReduceField.KEY.name() + "." +
lastReduceKeyColName
+ ":" + numDistinctUDFs + "." +
SemanticAnalyzer.getColumnInternalName(j);
```
Non-Return Path resolves the correct column name by letting RowResolver
resolve the correct name before creating `ExprNodeColumnDesc`. This PR would
implement the logic by traversing params with two paths.
Why do we need the two paths? That's because the non-distinct UDAF may refer
to the input of DISTINCT UDAF which comes later like below.
```
SELECT
SUM(DISTINCT col1),
COUNT(DISTINCT col1),
SUM(col2), -- This has to refer to the key for `SUM(DISTINCT col2)`
MAX(DISTINCT col1),
SUM(DISTINCT col2),
MIN(DISTINCT col1)
FROM test;
```
Note that this method is used only when `hive.map.aggr` is disabled. With it
enabled, non-distinct UDAFs don't refer to DISTINCT keys as they are
deduplicated.
--
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]
