Minor: allow to build RuntimeEnv from RuntimeConfig #12151
Conversation
github-actions
bot
added
core
| let runtime_config = RuntimeConfig::new() | ||
| .with_memory_pool(Arc::new(GreedyMemoryPool::new(pool_size))); | ||
| let runtime = Arc::new(RuntimeEnv::new(runtime_config).unwrap()); | ||
| let runtime_env = RuntimeConfig::new() |
There was a problem hiding this comment.
not sure why it is env, config is more appropriate imho
There was a problem hiding this comment.
That's because a newly introduced method build returns env. Please check the rest of this expression in diff below.
| let runtime = Arc::new(RuntimeEnv::new(runtime_config).unwrap()); | ||
| let runtime_env = RuntimeConfig::new() | ||
| .with_memory_pool(Arc::new(GreedyMemoryPool::new(pool_size))).build(); | ||
| let runtime = Arc::new(runtime_env.unwrap()); |
There was a problem hiding this comment.
we should avoid calling .unwrap() although its not related to the PR
There was a problem hiding this comment.
Sure. Even for tests, if we have complex chains of unwraps, we can use expect with a more detailed failure explanation.
There was a problem hiding this comment.
(note this is a test so I think it is ok to call unwrap() here)
|
Thanks @theirix for you contribution, I started the pipeline |
There was a problem hiding this comment.
Thank you @theirix and @comphead -- I think this looks good to me
@theirix -- any chance you can update other uses of RuntimeEnv::new() in the codebase to use the new pattern. There appear to be a few more:
https://github.com/search?q=repo%3Aapache%2Fdatafusion%20RuntimeEnv%3A%3Anew&type=code
| let runtime = Arc::new(RuntimeEnv::new(runtime_config).unwrap()); | ||
| let runtime_env = RuntimeConfig::new() | ||
| .with_memory_pool(Arc::new(GreedyMemoryPool::new(pool_size))).build(); | ||
| let runtime = Arc::new(runtime_env.unwrap()); |
There was a problem hiding this comment.
(note this is a test so I think it is ok to call unwrap() here)
| let runtime_config = RuntimeConfig::new() | ||
| .with_memory_pool(Arc::new(GreedyMemoryPool::new(pool_size))); | ||
| let runtime = Arc::new(RuntimeEnv::new(runtime_config).unwrap()); | ||
| let runtime_env = RuntimeConfig::new() |
|
Thank you! I think tests would benefit from this change, and I see @devanbenz is up to improving tests. |
Which issue does this PR close?
Closes #12137
Rationale for this change
Allow to create
RuntimeEnvdirectly from the config instance.What changes are included in this PR?
Are these changes tested?
To test this change, I've changed one existing test sort_fuzz to use a new API.
Are there any user-facing changes?
No