Skip to content

Conversation

tobymao
Copy link
Owner

@tobymao tobymao commented Oct 11, 2025

…r, unexpect kwargs shouldn't really happen in the parser outside of implementing new code, so it will be caught in unit testing

make validation faster, unexpect kwargs shouldn't really happen in the parser outside of implementing new code

so we only catch these errors in unittesting.
@georgesittas
Copy link
Collaborator

georgesittas commented Oct 11, 2025

unexpect kwargs shouldn't really happen in the parser outside of implementing new code

I'm not sure about this. For example, user code could use certain args, which are renamed in a breaking SQLGlot change. The user won't be notified about this at all after this PR is merged.

I generally feel like this is a useful validation to keep. You may have a typo, or make a mistake when constructing expressions, or whatever. It's useful to get a signal that you did something wrong.

Can't we refactor this somehow to make it faster? I wonder if keeping the self.args loop and doing a self._mandatory_args <= self.args is faster because you use set operations instead of rolling our own loop.

@tobymao
Copy link
Owner Author

tobymao commented Oct 11, 2025

another thing to do is if we do type validation, it should cover these cases

@georgesittas
Copy link
Collaborator

georgesittas commented Oct 13, 2025

Type validation is not relevant to this, right? This should be a parse-time validation, not tied to an optional rule.

@tobymao
Copy link
Owner Author

tobymao commented Oct 13, 2025

@georgesittas why isn't it relavent? it's very similar

@georgesittas
Copy link
Collaborator

Because in one case you validate whether / which args are set at parse time, and in the other you validate the types inferred at optimization time. They're separate passes with different purposes and the latter is optional.

@tobymao
Copy link
Owner Author

tobymao commented Oct 13, 2025

yea but there could be cases where args require type inference, and so i argue it may not always be correct

@georgesittas
Copy link
Collaborator

I'm not aware of such an example, and even if there are, I'd expect them to be outliers. But that's besides my point. Losing parse-time validations is not a good idea imo due to:

user code could use certain args, which are renamed in a breaking SQLGlot change. The user won't be notified about this at all after this PR is merged.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants