Reduce memory usage of TypedSet #4123
Conversation
presto-main/src/main/java/io/prestosql/operator/aggregation/TypedSet.java
Outdated
Show resolved
Hide resolved
| else if (type instanceof VarcharType) { | ||
| // If bound on length of varchar is smaller than defaultSize, use that as expected size | ||
| return ((VarcharType) type).getLength() | ||
| .map(length -> Math.min(length, defaultSize)) |
There was a problem hiding this comment.
Varchars usually won't occupy full declared length.
There was a problem hiding this comment.
varchar length is "character count", andvarchar is encoded in UTF-8 which means up to 4 bytes per character. If this is just an expected size, assuming ascii is reasoanble, but we should note that in a comment here.
There was a problem hiding this comment.
Added a comment for UTF-8
There was a problem hiding this comment.
I still don't think that assuming that varchar will occupy entire length is the correct one. It seems very pessimistic. What do you think @dain?
There was a problem hiding this comment.
@sopel39 that's why we do min here, right?
There was a problem hiding this comment.
I'm not sure what contract of defaultSize is here. Could it be excessively large?
There was a problem hiding this comment.
In current usage defaultSize is 16, 32 or 100 depending on where it's getting called from. This change should just reduce the expected size estimate when a smaller bound is known (E.g. varchar(10)). This would help reduce memory usage in cases where a large no. of TypeSet are generated but each set has small no. of entries which are a few characters long.
There was a problem hiding this comment.
I think we should assume the default size is one byte. I would update the comment a bit:
It can take up to 4 bytes per character due to UTF-8 encoding, but we assume the data is ASCII and only needs one byte.
There was a problem hiding this comment.
Updated the comments in code
raunaqmorarka
force-pushed
the
typed_set_opt
branch
from
10ab1b3
to
ddbf3ed
Compare
raunaqmorarka
force-pushed
the
typed_set_opt
branch
from
ddbf3ed
to
937403b
Compare
| else if (type instanceof VarcharType) { | ||
| // If bound on length of varchar is smaller than defaultSize, use that as expected size | ||
| return ((VarcharType) type).getLength() | ||
| .map(length -> Math.min(length, defaultSize)) |
There was a problem hiding this comment.
I still don't think that assuming that varchar will occupy entire length is the correct one. It seems very pessimistic. What do you think @dain?
presto-spi/src/main/java/io/prestosql/spi/type/VarcharType.java
Outdated
Show resolved
Hide resolved
presto-spi/src/main/java/io/prestosql/spi/type/VarcharType.java
Outdated
Show resolved
Hide resolved
raunaqmorarka
force-pushed
the
typed_set_opt
branch
from
937403b
to
76cd2f6
Compare
| else if (type instanceof VarcharType) { | ||
| // If bound on length of varchar is smaller than defaultSize, use that as expected size | ||
| return ((VarcharType) type).getLength() | ||
| .map(length -> Math.min(length, defaultSize)) |
There was a problem hiding this comment.
I think we should assume the default size is one byte. I would update the comment a bit:
It can take up to 4 bytes per character due to UTF-8 encoding, but we assume the data is ASCII and only needs one byte.
76cd2f6
to
3fa8534
Compare
|
merged, thanks! |
No description provided.