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

Remove unused AggregateOptions struct and scalar_update_factor config setting #12241

Merged
merged 4 commits into from
Sep 2, 2024

Conversation

jc4x4
Copy link
Contributor

@jc4x4 jc4x4 commented Aug 29, 2024

Which issue does this PR close?

Closes #11998 .

Rationale for this change

Remove unused config in AggregateOptions. Keep the parent struct open in case we need it later. I am fine to removing the option struct.

What changes are included in this PR?

Are these changes tested?

no new test. just existing cargo test.

Are there any user-facing changes?

Keeping the AggregateOptions struct in case we need it later.

apache#11998
@github-actions github-actions bot added common Related to common crate execution Related to the execution crate labels Aug 29, 2024
@findepi
Copy link
Member

findepi commented Aug 29, 2024

Looking at the build errors, it might be necessary to fully remove the AggregateOptions struct now?

@github-actions github-actions bot added documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt) labels Aug 30, 2024
@jc4x4 jc4x4 requested a review from findepi August 30, 2024 23:24
Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

LGTM except for docs/source/user-guide/configs.md which i didn't review yet

@alamb alamb changed the title Remove scalar_update_factor from AggregateOptions Remove unused AggregateOptions struct as well as scalar_update_factor from AggregateOptions Sep 1, 2024
@alamb alamb added the api change Changes the API exposed to users of the crate label Sep 1, 2024
@alamb alamb changed the title Remove unused AggregateOptions struct as well as scalar_update_factor from AggregateOptions Remove unused AggregateOptions struct and scalar_update_factor config setting Sep 1, 2024
@alamb
Copy link
Contributor

alamb commented Sep 1, 2024

Thank you @jc4x4 and @findepi -- this is a nice cleanup 🧹

I also marked this PR as an API change as the options structure is changed

Copy link
Member

@lewiszlw lewiszlw left a comment

Choose a reason for hiding this comment

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

👍

@lewiszlw lewiszlw merged commit dd32089 into apache:main Sep 2, 2024
27 checks passed
@jc4x4 jc4x4 deleted the scalar branch September 3, 2024 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change Changes the API exposed to users of the crate common Related to common crate documentation Improvements or additions to documentation execution Related to the execution crate sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unused config value scalar_update_factor
4 participants