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

removed count in favour of invocation #143

Merged
merged 1 commit into from
May 15, 2023

Conversation

matthias-Q
Copy link
Collaborator

@matthias-Q matthias-Q commented May 15, 2023

Closes #140

This PR removed the count node in favor of the more generic `invocation.

I have a question:
We have _expression, term and _select_expression (which is term or all_fields). I may have forgotten this, but why do we have these? They look almost identical and offer only another layer in the AST hierarchy.

@DerekStride
Copy link
Owner

We have _expression, term and _select_expression (which is term or all_fields). I may have forgotten this, but why do we have these? They look almost identical and offer only another layer in the AST hierarchy.

I think these have converged recently. We could remove _select_expression and just use an explicit choice rule or see if it's possible to add term to _expression.

@matthias-Q
Copy link
Collaborator Author

Now that I think a little bit about it, I think _select_expression is fine, since it combines termandall_fields`, which is used multiple times.

term adds the optional alias and _expression is also fine, since it collects all the different variations (columns, subqueries etc)

@matthias-Q matthias-Q merged commit ea9ed48 into DerekStride:main May 15, 2023
@matthias-Q matthias-Q deleted the refactor_count branch May 15, 2023 21:17
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.

Refactor: remove count node
2 participants