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

UserDefindedLogicalNode::from_template does not return a Result<...>. #10571

Closed
LorrensP-2158466 opened this issue May 18, 2024 · 3 comments · Fixed by #10575
Closed

UserDefindedLogicalNode::from_template does not return a Result<...>. #10571

LorrensP-2158466 opened this issue May 18, 2024 · 3 comments · Fixed by #10575
Labels
enhancement New feature or request

Comments

@LorrensP-2158466
Copy link
Contributor

Is your feature request related to a problem or challenge?

This is really a feature request but more of a question.

Currently UserDefinedLogicalNode::from_template only returns Arc<dyn Self> but i have noticed that LogicalPlan::with_new_exprs does return a Result. So when it calls from_template on an Extension node, it just wraps it in an Ok().
In the example of UserDefinedLogicalNodeCore where TopK is implemented, the implementation of from_template first asserts the inputs and exprs.
I have also tried to implement a custom LogicalPlan node and my own implementation of from_template also has several error cases (not just asserting the new values) which i unfortunately have to unwrap.

Is this expected behaviour, or is it maybe nicer to return Results

Describe the solution you'd like

No response

Describe alternatives you've considered

No response

Additional context

No response

@LorrensP-2158466 LorrensP-2158466 added the enhancement New feature or request label May 18, 2024
@alamb
Copy link
Contributor

alamb commented May 19, 2024

I agree it would be much better to change the API and return an Result<..>

Thank you for the report

@LorrensP-2158466
Copy link
Contributor Author

LorrensP-2158466 commented May 20, 2024

I don't see this as a really difficult API change. Is it ok if I do this?
Edit: there is a PR already,did not see it, sorry.

@alamb
Copy link
Contributor

alamb commented May 20, 2024

I don't see this as a really difficult API change. Is it ok if I do this? Edit: there is a PR already,did not see it, sorry.

@lewiszlw beats us to it!

(BTW if anyone wants a workaround for this in older versions that does not requier a panic, they could potentially return a node that would generate a runtime error if the arguments to from_template didn't error)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants