[native] Add caching of parsed Types #21325
Conversation
kevinwilfong
force-pushed
the
cache_type_conversion
branch
2 times, most recently
from
2d8fc42
to
17b39fa
Compare
| @@ -61,6 +67,7 @@ class VeloxExprConverter { | |||
| const protocol::CallExpression& pexpr) const; | |||
|
|
|||
| velox::memory::MemoryPool* pool_; | |||
There was a problem hiding this comment.
Mark pool_ and typeParser_ as consts? Thanks!
| @@ -35,7 +37,7 @@ class VeloxQueryPlanConverterBase { | |||
| explicit VeloxQueryPlanConverterBase( | |||
There was a problem hiding this comment.
NYC: drop explicit as the ctor takes more than one input? Thanks!
| @@ -218,6 +220,7 @@ class VeloxQueryPlanConverterBase { | |||
| velox::memory::MemoryPool* pool_; | |||
There was a problem hiding this comment.
NYC: mark poo_ and queryCtx_ as consts?
velox::memory::MemoryPool* const pool_;
velox::core::QueryCtx* const queryCtx_;
17b39fa
to
a239894
Compare
We've seen cases of queries that spend a large amount of time just parsing types when converting the Presto Plan to Velox. This seems to be because it parses the same large Row Types that are used across many field accesses. Adding caching within a request shows a substantial decrease in the amount of time it takes to do the conversion. Notably, this helps with timeouts we're seeing making calls from the coordinator to create tasks on the Workers.
kevinwilfong
force-pushed
the
cache_type_conversion
branch
from
a239894
to
20e0ac1
Compare
|
@kevinwilfong I am adding Presto type parser support using Flex/Bison in Velox. facebookincubator/velox#7568 |
| velox::TypePtr parse(const std::string& text) const; | ||
|
|
||
| private: | ||
| mutable std::unordered_map<std::string, velox::TypePtr> cache_; |
There was a problem hiding this comment.
Any reason not to use the SimpleLRUCache from Velox?
We use that to cache file handles
https://github.com/facebookincubator/velox/blob/main/velox/connectors/hive/FileHandle.h#L62
|
I am worried that without a bound, the cache might grow too big in a production system. |
We've seen cases of queries that spend a large amount of time just parsing types when converting the Presto Plan to Velox. This seems to be because it parses the same large Row Types that are used across many field accesses.
Adding caching within a request shows a substantial decrease in the amount of time it takes to do the conversion.
Notably, this helps with timeouts we're seeing making calls from the coordinator to create tasks on the Workers.