zabetak commented on code in PR #5294: URL: https://github.com/apache/hive/pull/5294#discussion_r1684143833
########## ql/src/java/org/apache/hadoop/hive/ql/parse/RowResolver.java: ########## @@ -125,6 +125,22 @@ private boolean putInternal(String tabAlias, String colAlias, ColumnInfo colInfo return false; } + /** + * Puts a resolver entry corresponding to a aggregate expression and its + * column alias which is used for alias recognition in the having clause. + * @param exprToColumnAlias + * The map containing the expression to alias mapping + * @return + * @throws SemanticException + */ + public void putAggregateAlias(Map<ASTNode, String> exprToColumnAlias) throws SemanticException { Review Comment: The name of the method is unnecessarily tight to aggregates and aliases while the implementation seems much more generic than that. Given that we just call `put` for every entry in the map maybe a better name would be `putAll` or maybe `replaceAll` since we only perform the action if an entry is already there. Will this logic really add an entry in the resolver or it just changes the alias for ColumnInfo that is registered previously? Depending on the answer another potential name would be `replaceAliases`. A well chosen name will better indicate what the method really does without requiring checking the implementation. ########## ql/src/test/queries/clientpositive/having.q: ########## @@ -18,4 +19,7 @@ EXPLAIN SELECT key, max(value) FROM src GROUP BY key HAVING max(value) > "val_25 SELECT key, max(value) FROM src GROUP BY key HAVING max(value) > "val_255"; EXPLAIN SELECT key, COUNT(value) FROM src GROUP BY key HAVING count(value) >= 4; -SELECT key, COUNT(value) FROM src GROUP BY key HAVING count(value) >= 4; \ No newline at end of file +SELECT key, COUNT(value) FROM src GROUP BY key HAVING count(value) >= 4; + +EXPLAIN CBO SELECT count(value) as c, max(key) as m from src GROUP BY key HAVING c > 3 and m > 0; +SELECT count(value) as c, max(key) as m from src GROUP BY key HAVING c > 3 and m > 0; Review Comment: Since we are talking about `max(key)` does the `m > 0` condition filter anything? If not then maybe changing to `m > 400` would be a safer choice. -- 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-unsubscr...@hive.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org For additional commands, e-mail: gitbox-h...@hive.apache.org