[CALCITE-4102] Some improvements to aggregate related operations by liyafan82 · Pull Request #2053 · apache/calcite

liyafan82 · 2020-07-02T04:40:36Z

Improve the use of data structures
Extract common methods
Avoid expensive operations

zinking · 2020-07-02T12:19:20Z

looks all equivalent rewrites for the refactoring,
any explanation for the improvements

yanlin-Lynn · 2020-07-03T01:35:01Z

core/src/main/java/org/apache/calcite/util/ImmutableBitSet.java

+   * Checks if all bit sets contain a particular bit.
+   */
+  public static boolean allContain(Collection<ImmutableBitSet> bitSets,
+                                   int bit) {


we do not format like this

Revised. Thank you for your kind reminder.

liyafan82 · 2020-07-03T06:12:01Z

looks all equivalent rewrites for the refactoring,
any explanation for the improvements

Sure. Thanks a lot for your feedback. Below are some explanations:

We extract a common method for ImmutableBitSet, which is repeated multiple times in the code base. We also find it useful in our code base. This makes the code simpler and easier to maintain.
In Aggregate#getRollup, we replace the LinkedHashSet with an ArrayList, because we can make sure that the newly added element must be distinct (according to the definition of a rollup). This makes adding data faster (LinkedHashSet takes O(logn) time, whereas ArrayList takes O(1)).
When checking if a group set is a roll up, we replace the except operation with cardinality minus, which is much faster. We can make this optimization, because when a set contains another, the cardinality of the difference is the difference of the cardiality.
In Util#firstDuplicate method, we replace the HashMap with a HashSet, because the value in the HashMap is not really used. This makes the logic clearer.

yanlin-Lynn · 2020-07-07T10:00:31Z

make sense.
Can you update the commit log to describe the content of the PR shortly,
and add your name in the end.

See the guide in https://calcite.apache.org/develop/#contributing

liyafan82 · 2020-07-08T11:54:51Z

make sense.
Can you update the commit log to describe the content of the PR shortly,
and add your name in the end.

See the guide in https://calcite.apache.org/develop/#contributing

@yanlin-Lynn Thanks a lot for your kind reminder.
I have updated the commit message. Please check it.

julianhyde · 2020-07-09T02:09:20Z

FWIW, I intentionally used HashMap rather than HashSet. HashSet uses a HashMap under the covers. I figured I'd save an object allocation. Yours is a bit clearer, a bit less efficient.

ret is not a great name for a variable; also declare it as List<Integer> not ArrayList<Integer>.

liyafan82 · 2020-07-09T03:34:47Z

FWIW, I intentionally used HashMap rather than HashSet. HashSet uses a HashMap under the covers. I figured I'd save an object allocation. Yours is a bit clearer, a bit less efficient.

Thanks a lot for your feedback. It is clever to HashMap to avoid object allocation.
Fortunately, the value object for HashSet is declared static, so it is allocated once, and shared by all HashSet objects.

ret is not a great name for a variable; also declare it as List<Integer> not ArrayList<Integer>.

Good suggestion. I have renamed ret to rollUpBits.

chunweilei · 2020-07-14T02:32:06Z

core/src/main/java/org/apache/calcite/rel/core/Aggregate.java

          // Each subsequent items must be a subset with one fewer bit than the
          // previous item
-          set.addAll(g.except(bitSet).toList());
+          ImmutableBitSet diff = g.except(bitSet);


Would there be duplicate values since Set is changed to List?

@chunweilei Thanks a lot for your feedback.
According to the definition of Rollup (https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/core/Aggregate.java#L508-L513), there can be no duplicate.

hsyuan · 2020-07-15T16:38:46Z

FWIW, I intentionally used HashMap rather than HashSet. HashSet uses a HashMap under the covers. I figured I'd save an object allocation. Yours is a bit clearer, a bit less efficient.

Thanks a lot for your feedback. It is clever to HashMap to avoid object allocation.
Fortunately, the value object for HashSet is declared static, so it is allocated once, and shared by all HashSet objects.

I think what @julianhyde means is that the HashSet holds a pointer to HashMap, with HashSet object header, it will use 16 bytes more than bare HashMap.

…a Fan) 1. Extract a common method for ImmutableBitSet 2. Refine the use of data structures 3. Replace expensive operations with cheap ones

liyafan82 · 2020-07-16T02:25:28Z

FWIW, I intentionally used HashMap rather than HashSet. HashSet uses a HashMap under the covers. I figured I'd save an object allocation. Yours is a bit clearer, a bit less efficient.

Thanks a lot for your feedback. It is clever to HashMap to avoid object allocation.
Fortunately, the value object for HashSet is declared static, so it is allocated once, and shared by all HashSet objects.

I think what @julianhyde means is that the HashSet holds a pointer to HashMap, with HashSet object header, it will use 16 bytes more than bare HashMap.

@hsyuan Thanks for your kind reminder. I have reverted the code to use HashMap, and made it clear in the comment that this is by design. BTW, why it is 16 more bytes?

hsyuan · 2020-07-16T02:29:01Z

I have reverted the code to use HashMap, and made it clear in the comment that this is by design. BTW, why it is 16 more bytes?

Suppose in Hotspot 64-bit VM, with COOPS enabled, the object header is 12 bytes, the pointer to HashMap uses 4 bytes. So it is 16 bytes.

liyafan82 · 2020-07-16T02:31:05Z

I have reverted the code to use HashMap, and made it clear in the comment that this is by design. BTW, why it is 16 more bytes?

Suppose in Hotspot 64-bit VM, with COOPS enabled, the object header is 12 bytes, the pointer to HashMap uses 4 bytes. So it is 16 bytes.

Awesome! Thanks for your clarification.

yanlin-Lynn

+1

liyafan82 force-pushed the fly_0702_opt branch 2 times, most recently from 1c39e51 to 5b3e872 Compare July 2, 2020 06:07

yanlin-Lynn reviewed Jul 3, 2020

View reviewed changes

liyafan82 force-pushed the fly_0702_opt branch from 5b3e872 to 4903337 Compare July 3, 2020 06:01

liyafan82 force-pushed the fly_0702_opt branch from 4903337 to 555f332 Compare July 8, 2020 11:53

liyafan82 force-pushed the fly_0702_opt branch from 555f332 to add8757 Compare July 9, 2020 03:30

chunweilei reviewed Jul 14, 2020

View reviewed changes

[CALCITE-4102] Some improvements to aggregate related operations (Liy…

f7ce021

…a Fan) 1. Extract a common method for ImmutableBitSet 2. Refine the use of data structures 3. Replace expensive operations with cheap ones

liyafan82 force-pushed the fly_0702_opt branch from add8757 to f7ce021 Compare July 16, 2020 02:23

yanlin-Lynn approved these changes Jul 17, 2020

View reviewed changes

vlsi force-pushed the master branch from e4458fa to 5462be9 Compare July 17, 2020 20:19

yanlin-Lynn added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jul 20, 2020

hsyuan merged commit ce2ae64 into apache:master Jul 29, 2020

Conversation

liyafan82 commented Jul 2, 2020

Uh oh!

zinking commented Jul 2, 2020

Uh oh!

yanlin-Lynn Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

liyafan82 Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

liyafan82 commented Jul 3, 2020

Uh oh!

yanlin-Lynn commented Jul 7, 2020

Uh oh!

liyafan82 commented Jul 8, 2020

Uh oh!

julianhyde commented Jul 9, 2020

Uh oh!

liyafan82 commented Jul 9, 2020

Uh oh!

chunweilei Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

liyafan82 Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

hsyuan commented Jul 15, 2020

Uh oh!

liyafan82 commented Jul 16, 2020

Uh oh!

hsyuan commented Jul 16, 2020

Uh oh!

liyafan82 commented Jul 16, 2020

Uh oh!

yanlin-Lynn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants