Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++][Compute] Add FunctionOptions::Validate #45760

Open
pitrou opened this issue Mar 13, 2025 · 10 comments
Open

[C++][Compute] Add FunctionOptions::Validate #45760

pitrou opened this issue Mar 13, 2025 · 10 comments

Comments

@pitrou
Copy link
Member

pitrou commented Mar 13, 2025

Describe the enhancement requested

Compute function options can have validity constraints, for example a quantile should be between 0.0 and 1.0.

By exposing a Status Validate() virtual method on FunctionOptions, we would allow easier debugging and integration with higher-level languages.

Component(s)

C++

@pitrou
Copy link
Member Author

pitrou commented Mar 13, 2025

@zanmato1984 @WillAyd @kou Does this sound useful to you?

@pitrou
Copy link
Member Author

pitrou commented Mar 13, 2025

In some cases, validation could require the input types, though.

@kou
Copy link
Member

kou commented Mar 13, 2025

+1

@zanmato1984
Copy link
Contributor

+1.

Having a unified interface of input option validation could be very useful - I think many functions are doing this kind of checking in either the resolution or the function body.

Just that, as you mentioned, the validation may depend on the input types, so where to call it could be subtle. Perhaps near

RETURN_NOT_OK(CheckOptions(func, options));

?

@pitrou
Copy link
Member Author

pitrou commented Mar 13, 2025

Just that, as you mentioned, the validation may depend on the input types, so where to call it could be subtle.

Actually, the part that depends on input types might be done in the kernel's output resolver?

@zanmato1984
Copy link
Contributor

Actually, the part that depends on input types might be done in the kernel's output resolver?

True. But that may make some checking of the function option (I can't name one though) that depends on the input types have to be separated into the output resolver. And this might weaken the meaning of having an unified interface of validating the function option?

@pitrou
Copy link
Member Author

pitrou commented Mar 13, 2025

And this might weaken the meaning of having an unified interface of validating the function option?

Agreed. So perhaps it's not that useful? (we can of course already check options in the output resolver)

@zanmato1984
Copy link
Contributor

I would say it's a "nice to have". It's not necessary in terms of functionality. But it surely helps on better code structure and readability - as long as enough context (input types e.g.) are given so we can do all kinds of validation here rather than having to do it in output resolver.

@pitrou
Copy link
Member Author

pitrou commented Mar 14, 2025

My original idea was to be able to validate options independent of their usage. For example, this would raise an exception in Python because quantiles must be between 0 and 1:

>>> pc.QuantileOptions(q=[2.0])
QuantileOptions(q=[2], interpolation=LINEAR, skip_nulls=true, min_count=0)

@zanmato1984
Copy link
Contributor

My original idea was to be able to validate options independent of their usage. For example, this would raise an exception in Python because quantiles must be between 0 and 1:

pc.QuantileOptions(q=[2.0])
QuantileOptions(q=[2], interpolation=LINEAR, skip_nulls=true, min_count=0)

Oh, that's a different use case than what I've been imagining. Sorry I don't use Python that much so I would stand neutral on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants