datafusion icon indicating copy to clipboard operation
datafusion copied to clipboard

The config values aren't checked

Open xudong963 opened this issue 3 months ago • 9 comments

> set datafusion.explain.format = indnet;
0 row(s) fetched.
Elapsed 0.002 seconds.

> set datafusion.explain.format = indent;
0 row(s) fetched.
Elapsed 0.002 seconds.

The indnet doesn't belong to format, obviously, it's a wrong value. But I didn't get any hints for it.


It'll be great UX if we can give some hints for the wrong config values.

xudong963 avatar Sep 10 '25 06:09 xudong963

@Weijun-H do you think we can make the follow-up changes as good first issues?

xudong963 avatar Sep 15 '25 07:09 xudong963

Yes, I think it’s a good first issue because it need wrap up the current logic and is a good way for new contributors to become familiar with DataFusion.

Weijun-H avatar Sep 16 '25 07:09 Weijun-H

take

etolbakov avatar Sep 16 '25 10:09 etolbakov

Hey @xudong963 @Weijun-H

I was not able to reproduce this issue with the latest build:

Image

What do you think if some more improvements are required?

P.S. I did some digging and found that suggest_valid_function is already available. So a potential improvement could be something like "did you mean" suggestions.

etolbakov avatar Sep 19 '25 15:09 etolbakov

To close this ticket I think we'd need to apply this fix to all other config values which expect a certain set of values (i.e. enums) but still parse as string. For example:

https://github.com/apache/datafusion/blob/b9517a1a08c573af2ee0e8814413eb5d96b2e5bd/datafusion/common/src/config.rs#L283-L291

This option specifies a list of valid values but it accepts arbitrary strings.

Jefffrey avatar Sep 19 '25 15:09 Jefffrey

@etolbakov Maybe you missed it, but there was already a PR submitted and merged above that resolved this for the explain format configuration, only. At the end of that PR, there was this comment (https://github.com/apache/datafusion/pull/17549#pullrequestreview-3221955466), asking to implement it for others. That PR serves as a good example of how to implement it for other config values.

petern48 avatar Sep 19 '25 15:09 petern48

Thanks @Jefffrey @petern48 Now it makes sense!

etolbakov avatar Sep 19 '25 15:09 etolbakov

Hey @xudong963 @Weijun-H

I was not able to reproduce this issue with the latest build:

Image What do you think if some more improvements are required?

P.S. I did some digging and found that suggest_valid_function is already available. So a potential improvement could be something like "did you mean" suggestions.

I think 'did you mean' is a good idea too.

2010YOUY01 avatar Sep 21 '25 15:09 2010YOUY01

The timezone config option would be another to add validation to.

Omega359 avatar Dec 11 '25 14:12 Omega359