HIVE-29367: prevent Long overflows in ConvertJoinMapJoin#6237
HIVE-29367: prevent Long overflows in ConvertJoinMapJoin#6237deniskuzZ merged 5 commits intoapache:masterfrom
Conversation
asf-ci-hive
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
konstantinb
changed the title
|
| } | ||
| Operator<? extends OperatorDesc> parentOp = joinOp.getParentOperators().get(pos); | ||
| totalSize += computeOnlineDataSize(parentOp.getStatistics()); | ||
| totalSize = StatsUtils.safeAdd(totalSize, computeOnlineDataSize(parentOp.getStatistics())); |
There was a problem hiding this comment.
I'm not sure it's appropriate to use safeAdd for a table size?
on the other side hashTableDataSizeAdjustment does that as well, so I guess it's fine
cc @zabetak, @thomasrebele
There was a problem hiding this comment.
I think it's fine. The total size here does not need to be 100% correct, it's just an estimation that influences the join decision. Might make sense to rename it to estimatedTotalSize.
| if (cs != null) { | ||
| String colTypeLowerCase = cs.getColumnType().toLowerCase(); | ||
| long nonNullCount = cs.getNumNulls() > 0 ? numRows - cs.getNumNulls() + 1 : numRows; | ||
| long nonNullCount = cs.getNumNulls() > 0 ? Math.max(1L, numRows - cs.getNumNulls() + 1) : numRows; |
There was a problem hiding this comment.
maybe Math.max(0L, numRows - cs.getNumNulls()) + 1
|
@konstantinb, do we need same for |
|
I'm away until the end of the next week. I will respond to review comments when I'm back. Thank you. |
@deniskuzZ, from my analysis, all elements of this calculation are directly derived from Hive configuration settings. I believe that overflow could only occur with (currently) extremely improbable configuration settings, such as 1TB of RAM for the mapjoin conversion threshold, overSubscriptionFactor of 1000 and slotsPerQuery of 8390 I realize that huge amounts of RAM could become available sooner rather than later. At the same time, since this functionality is not data-driven but purely config-driven, should it be better considered a separate fix? |
|
4 participants
What changes were proposed in this pull request?
HIVE-29367: fixing overflows in ConvertJoinMapJoin calculations
Why are the changes needed?
ConvertJoinMapJoin does not use StatsUtils.safeAdd()/saveMult() for all its calculations. There are some real life scenarios when it could perform a catastrophic decision to convert a join to a mapjoin after calculating negative size for the 'small" table, resulting in an OOM during query processing
Does this PR introduce any user-facing change?
No
How was this patch tested?
Via unit testing and with load testing on a custom Hive installation based of 4.0x version
You can see the test output generated by the pre-fix code here:
it clearly confirms the decision of perform a mapjoin despite very large volume of data