Change in asterixdb[master]: ASTERIXDB-1566, ASTERIXDB-1733: Hash Group By and Hash Join ...

["Taewoo Kim (Code Review)" <do...@asterixdb.incubator.apache.org>]

Taewoo Kim has posted comments on this change.

Change subject: ASTERIXDB-1566, ASTERIXDB-1733: Hash Group By and Hash Join conform to the memory budget
......................................................................


Patch Set 54:

(10 comments)

Thanks Yingyi.

https://asterix-gerrit.ics.uci.edu/#/c/1056/54/asterixdb/asterix-app/src/main/resources/asterix-build-configuration.xml
File asterixdb/asterix-app/src/main/resources/asterix-build-configuration.xml:

Line 62:     <value>256KB</value>
> keep the number unchanged.
Done


Line 66:     <value>256KB</value>
> keep the number unchanged.
Based on our conversation, we keep this in order to deal with the big object cases.


https://asterix-gerrit.ics.uci.edu/#/c/1056/54/asterixdb/asterix-app/src/test/resources/runtimets/results/api/cluster_state_1/cluster_state_1.1.adm
File asterixdb/asterix-app/src/test/resources/runtimets/results/api/cluster_state_1/cluster_state_1.1.adm:

Line 16:         "compiler.groupmemory": 262144,
> Ideally, those two numbers should be unchanged.
For groupmemory, yes. For join memory, we will keep this number based on our conversation.


https://asterix-gerrit.ics.uci.edu/#/c/1056/54/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/physical/ExternalGroupByPOperator.java
File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/physical/ExternalGroupByPOperator.java:

Line 297:      */
> Add a unit test for this method?  (It feels that this is a place where thin
Done


Line 298:     public static int calculateGroupByTableCardinality(int memoryBudgetInBytes, int numberOfGroupByColumns,
> public -> private
Done


Line 313:         long possibleNumberOfHashEntries = (long) 2 << numberOfBits;
> (long) 2 << numberOfBits  
Done


Line 317:         long groupByTableByteSize = SerializableHashTable.getExpectedTableSizeInByte((int) groupByTableSize, frameSize);
> data loss?
Changed the logic.


https://asterix-gerrit.ics.uci.edu/#/c/1056/54/hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/unit/AbstractExternalGroupbyTest.java
File hyracks-fullstack/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/unit/AbstractExternalGroupbyTest.java:

Line 179:         int numFrames = 50;
> keep 3 as unchanged?
This test can't run well when you count the hash table size for 1001 elements. The number of header frames (which will not be shrink at all under any circumstance) is already 8008 bytes / 256 bytes = 32 frames. Even for this 101 entry, 101 * 4 * 2 = 808 / 256 bytes = 4 frames. I think it's reasonable to keep this number. We can test all cases.


Line 190:         int numFrames = 50;
> keep 3 unchanged?
the same as the above.


Line 201:         int numFrames = 50;
> keep 3 unchanged?
the same as the above.


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/1056
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2b323e9a2141b4c1dd1652a360d2d9354d3bc3f5
Gerrit-PatchSet: 54
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Taewoo Kim <wa...@yahoo.com>
Gerrit-Reviewer: Jenkins <je...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Jianfeng Jia <ji...@gmail.com>
Gerrit-Reviewer: Michael Blow <mb...@apache.org>
Gerrit-Reviewer: Steven Jacobs <sj...@ucr.edu>
Gerrit-Reviewer: Taewoo Kim <wa...@yahoo.com>
Gerrit-Reviewer: Till Westmann <ti...@apache.org>
Gerrit-Reviewer: Wenhai Li <lw...@yahoo.com>
Gerrit-Reviewer: Yingyi Bu <bu...@gmail.com>
Gerrit-HasComments: Yes