[
https://issues.apache.org/jira/browse/HIVE-23627?focusedWorklogId=442064&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-442064
]
ASF GitHub Bot logged work on HIVE-23627:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 05/Jun/20 22:59
Start Date: 05/Jun/20 22:59
Worklog Time Spent: 10m
Work Description: zabetak commented on a change in pull request #1067:
URL: https://github.com/apache/hive/pull/1067#discussion_r436199146
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java
##########
@@ -90,13 +91,11 @@
// so aggregationIsDistinct is a boolean array instead of a single number.
private transient boolean[] aggregationIsDistinct;
// Map from integer tag to distinct aggrs
- private transient Map<Integer, Set<Integer>> distinctKeyAggrs =
- new HashMap<Integer, Set<Integer>>();
+ private transient Map<Integer, Set<Integer>> distinctKeyAggrs = new
HashMap<>();
// Map from integer tag to non-distinct aggrs with key parameters.
- private transient Map<Integer, Set<Integer>> nonDistinctKeyAggrs =
- new HashMap<Integer, Set<Integer>>();
+ private transient Map<Integer, Set<Integer>> nonDistinctKeyAggrs = new
HashMap<>();
// List of non-distinct aggrs.
- private transient List<Integer> nonDistinctAggrs = new ArrayList<Integer>();
+ private transient Set<Integer> nonDistinctAggrs = new LinkedHashSet<>();
Review comment:
I assume that LinkedHashSet was chosen in order to retain the order of
the elements and avoid calling contains on a List.
However, I have the impression that we can avoid the call to `List#contains`
rather easy since the way we populate the list is controlled.
Moreover, I think that the order of inserting the elements does not really
matter since what we are storing inside is a pointer to an aggregate and when
it is used it is used with indirection. The same pointer that we are putting in
this list we are putting also to the `nonDistinctKeyAggrs` and
`distinctKeyAggrs` inside the sets, so it is another indicator that retaining
the order is useless.
So instead of using the `LinkedHashSet` some other alternatives would be to
would be to keep the ArrayList with a small refactoring to avoid
`List#contains` or use a plain `HashSet`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Issue Time Tracking
-------------------
Worklog Id: (was: 442064)
Time Spent: 1.5h (was: 1h 20m)
> Review of GroupByOperator
> -------------------------
>
> Key: HIVE-23627
> URL: https://issues.apache.org/jira/browse/HIVE-23627
> Project: Hive
> Issue Type: Improvement
> Reporter: David Mollitor
> Assignee: David Mollitor
> Priority: Minor
> Labels: pull-request-available
> Time Spent: 1.5h
> Remaining Estimate: 0h
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
