Set DataFusion runtime configurations through SQL interface

[kumarlokesh]

Which issue does this PR close?

• ClosesSet DataFusion runtime configurations through SQL interface #15552.

Rationale for this change

What changes are included in this PR?

1. Extended the session context to handle runtime configurations.
2. Currently support memory limit configuration by using SET datafusion.runtime.memory_limit = '100M'.
3. Updated documentation.

Are these changes tested?

Are there any user-facing changes?

[berkaysynnada]

Hello @kumarlokesh. Thank you for working on this. I have 2 questions/concerns. Let's discuss on them a bit to get a future-proof design

1. There are also runtime_env: Arc<RuntimeEnv> in SessionState and TaskContext structs, and this RuntimeEnv has a field of pub memory_pool: Arc<dyn MemoryPool>. These MemoryPool implementations have some configurations about memory limit as well. Keeping 2 different things for the same purposes always bring trouble. So, I wonder can we somehow connect those 2 points.

2. Do you plan to practice this configuration in any part of the execution? While bringing such a feature, implementing at least one use-case makes things more clear and becomes a guidance for the further developments.

[kumarlokesh]

Hello @kumarlokesh. Thank you for working on this. I have 2 questions/concerns. Let's discuss on them a bit to get a future-proof design

1. There are also runtime_env: Arc<RuntimeEnv> in SessionState and TaskContext structs, and this RuntimeEnv has a field of pub memory_pool: Arc<dyn MemoryPool>. These MemoryPool implementations have some configurations about memory limit as well. Keeping 2 different things for the same purposes always bring trouble. So, I wonder can we somehow connect those 2 points.
2. Do you plan to practice this configuration in any part of the execution? While bringing such a feature, implementing at least one use-case makes things more clear and becomes a guidance for the further developments.

@berkaysynnada Thank you for the feedback!

1. Have revised the PR to ensure runtime environment configuration like memory-limit is exposed through the already defined RuntimeEnv -> memory_pool setting.

2. As a user can set the configuration datafusion.runtime.memory_limit (and other available runtime environment configurations) through SQL interface, I think having tests around this behaviour should be a good starting point. Can't think of another good use case besides this. Thoughts?

[2010YOUY01]

Running this test can't ensure this config is set, because it can run without setting the memory limit and use the default UnboundedMemoryPool, instead I think it should assert the spill count for a query that has spilled some intermediate data.

SET datafusion.runtime.memory_limit = '1M'
set datafusion.execution.sort_spill_reservation_bytes = 0;
select * from generate_series(1, 100000) as t1(v1) order by v1;
-- And assert spill-count from the query is > 0

You can check this

datafusion/datafusion/core/tests/memory_limit/mod.rs

Line 525 in 4ae0ede

let spill_count = metrics.spill_count().unwrap();

for how to assert the spill file count.

Later after all configurations are added, I think we should make this test case stronger by setting more runtime configs, and do some property test to ensure all of them are properly set.

BTW, I tried the above test locally, and the generate_series UDTF seems not registered in the PR branch, but it works in the main branch.

[kumarlokesh]

Running this test can't ensure this config is set, because it can run without setting the memory limit and use the default UnboundedMemoryPool, instead I think it should assert the spill count for a query that has spilled some intermediate data.

SET datafusion.runtime.memory_limit = '1M'
set datafusion.execution.sort_spill_reservation_bytes = 0;
select * from generate_series(1, 100000) as t1(v1) order by v1;
-- And assert spill-count from the query is > 0

You can check this

datafusion/datafusion/core/tests/memory_limit/mod.rs

Line 525 in 4ae0ede

let spill_count = metrics.spill_count().unwrap();

for how to assert the spill file count.
Later after all configurations are added, I think we should make this test case stronger by setting more runtime configs, and do some property test to ensure all of them are properly set.

BTW, I tried the above test locally, and the generate_series UDTF seems not registered in the PR branch, but it works in the main branch.

@2010YOUY01@berkaysynnada addressed above in 6501f21.

[berkaysynnada]

Maybe we can open this function to external uses?

[berkaysynnada]

Thank you @kumarlokesh for addressing my comments. I don't have further suggestions or concern other than @2010YOUY01 shared

[2010YOUY01]

Thank you. I plan to leave this PR open for 2 days more before merging, in case anyone has additional comments.

[2010YOUY01]

This implementation is all cloning Arcs inside RuntimeEnv, I think it shouldn't have any issue.

[alamb]

Thanks -- I recommend we merge this after the 47.0.0 release is created (likely tomorrow)

[alamb]

This looks really nice -- thank you @kumarlokesh and @2010YOUY01

[alamb]

We made branch-47 so let's get this one in!

[alamb]

I merged up from main to rerun CI to be sure all tests pass

[alamb]

🚀 so exciting -- thank you