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

refactor(sql): extract aggregate handling out into common utility class #9222

Merged
merged 1 commit into from
May 22, 2024

Conversation

jcrist
Copy link
Member

@jcrist jcrist commented May 21, 2024

This is an initial refactor moving towards #9170.

Previously every backend implemented their own _aggregate function - many of them copy-pasted (with slight variations) of each other.

To add a new order_by kwarg to _aggregate would require editing all 16 copies of this function, which would be annoying. This PR is an attempt to centralize their implementations into a common Gen class. The class takes a config flag to handle the common cases, the uncommon cases are then handled by backend-specific subclasses.

This also extracts the .agg attribute out to be a class variable (err, descriptor) rather than an instance variable.

An alternative implementation would be adding boolean flags directly to the SQLGlotCompiler class, but IIRC we intentionally moved away from those in the SQLAlchemy -> SQLGlot refactor. Could go either way here.

@jcrist jcrist added the ci-run-cloud Run BigQuery, Snowflake, Databricks, and Athena backend tests label May 21, 2024
@ibis-docs-bot ibis-docs-bot bot removed the ci-run-cloud Run BigQuery, Snowflake, Databricks, and Athena backend tests label May 21, 2024
@jcrist jcrist requested a review from gforsyth May 21, 2024 17:54
@jcrist
Copy link
Member Author

jcrist commented May 21, 2024

An alternative implementation would be adding boolean flags directly to the SQLGlotCompiler class

This would be simpler/less magical and would be my slight preference, went with the current extra level of abstraction since I thought I remembered someone else being against flags on the compiler.

Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me -- I also have vague memories of someone having issues with adding flags on the compiler class, but I can't remember any specifics.

I suppose we can always revisit and simplify this implementation if we decide that we want to make the change, but this is a clear win over our existing scattered implementations.

@gforsyth gforsyth merged commit 56e0b38 into ibis-project:main May 22, 2024
87 checks passed
@jcrist jcrist deleted the refactor-sql-aggregate branch May 22, 2024 19:39
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