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 code #18749

Merged
merged 1 commit into from
Dec 1, 2022
Merged

Conversation

kaikalur
Copy link
Contributor

@kaikalur kaikalur commented Dec 1, 2022

We tried to get distinct+limit only queries to work via various different hacks until we finally figured out the right way in:

#17640

So removing this deadcode that was introduced in two PRs:

#17199
#17536

Test plan - tests exist

== RELEASE NOTES ==

General Changes
* Removed two unused session params - `hash_based_distinct_limit_enabled` and `hash_based_distinct_limit_threshold` and the corresponding implementation in favor of the `quick_distinct_limit_enabled` feature.

@kaikalur kaikalur requested a review from a team as a code owner December 1, 2022 05:26
@kaikalur kaikalur requested a review from presto-oss December 1, 2022 05:26
@kaikalur kaikalur force-pushed the cleanup-distinct-limit-fixes branch from c6a3acd to 773ebd6 Compare December 1, 2022 05:31
@jainxrohit
Copy link
Contributor

Add release note for removing the existing config?

@jainxrohit jainxrohit self-requested a review December 1, 2022 05:33
@jainxrohit
Copy link
Contributor

can we add some description in commit on what is being removed?

@kaikalur kaikalur force-pushed the cleanup-distinct-limit-fixes branch from 773ebd6 to 44fd7ca Compare December 1, 2022 05:35
@kaikalur
Copy link
Contributor Author

kaikalur commented Dec 1, 2022

can we add some description in commit on what is being removed?

done

@kaikalur
Copy link
Contributor Author

kaikalur commented Dec 1, 2022

Add release note for removing the existing config?

Hmm ok maybe. I don't think anyone uses these - these are very specific enable/disable "feature" flags.

@jainxrohit
Copy link
Contributor

Add release note for removing the existing config?

Hmm ok maybe. I don't think anyone uses these - these are very specific enable/disable "feature" flags.

Since we dont visibility in to external usage, it may be safe to assume that they might be?

@kaikalur
Copy link
Contributor Author

kaikalur commented Dec 1, 2022

Add release note for removing the existing config?

Hmm ok maybe. I don't think anyone uses these - these are very specific enable/disable "feature" flags.

Since we dont visibility in to external usage, it may be safe to assume that they might be?

Done

@kaikalur kaikalur force-pushed the cleanup-distinct-limit-fixes branch from 44fd7ca to 5572c07 Compare December 1, 2022 06:06
@kaikalur kaikalur requested a review from rschlussel December 1, 2022 15:43
@kaikalur kaikalur merged commit d9c2c0a into prestodb:master Dec 1, 2022
@wanglinsong wanglinsong mentioned this pull request Jan 12, 2023
30 tasks
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.

3 participants