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 two_buckets_to_str() procedure #3334

Merged
merged 1 commit into from Jun 15, 2021

Conversation

afiskon
Copy link
Contributor

@afiskon afiskon commented Jun 14, 2021

Remove two_buckets_to_str() procedure

The two_buckets_to_str() procedure relies on fixed bucket_width
which is not going to be fixed in the future. Since the procedure
is used only to generate a hint that accompanies an error message,
the simplest solution is to remove this procedure. We can improve
error messages later if that would be necessary.

@afiskon afiskon requested a review from a team as a code owner June 14, 2021 11:38
@afiskon afiskon enabled auto-merge (rebase) June 14, 2021 11:46
@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #3334 (856b3ff) into master (2e44484) will increase coverage by 0.38%.
The diff coverage is 97.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3334      +/-   ##
==========================================
+ Coverage   90.17%   90.56%   +0.38%     
==========================================
  Files         215      211       -4     
  Lines       35391    35479      +88     
==========================================
+ Hits        31914    32130     +216     
+ Misses       3477     3349     -128     
Impacted Files Coverage Δ
src/chunk_append/chunk_append.c 97.41% <ø> (-0.04%) ⬇️
src/chunk_append/exec.c 95.84% <ø> (ø)
src/chunk_append/planner.c 93.38% <ø> (ø)
src/chunk_append/transform.c 96.66% <ø> (ø)
src/compat.h 100.00% <ø> (ø)
src/constraint.c 69.56% <ø> (ø)
src/debug_guc.c 92.63% <ø> (ø)
src/estimate.c 87.12% <ø> (ø)
src/extension_utils.c 81.39% <ø> (ø)
src/func_cache.c 91.01% <ø> (-0.10%) ⬇️
... and 160 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98110af...856b3ff. Read the comment docs.

@afiskon afiskon requested review from nikkhils and pmwkaa June 14, 2021 12:11
Copy link
Contributor

@erimatnor erimatnor left a comment

Choose a reason for hiding this comment

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

I guess it is fine, but it is a little bit unclear why this error and function cannot be replaced with the variable size whenever that's available. Whey does it need to be removed now?

@afiskon
Copy link
Contributor Author

afiskon commented Jun 14, 2021

I guess it is fine, but it is a little bit unclear why this error and function cannot be replaced with the variable size whenever that's available. Whey does it need to be removed now?

I generally try to keep the code / pull requests / feature implementation as small as possible (== MVP). If we keep this code it means that in further pull requests I will have to modify it as well which seems like an unnecessary time investment for me and reviewers, and next to zero gain for the users. This being said I'm fine with keeping the code if the team believes it's worth it.

@afiskon afiskon requested a review from erimatnor June 14, 2021 12:24
@afiskon afiskon force-pushed the remove_two_buckets_to_str_procedure branch from 658a61e to 3b93695 Compare June 14, 2021 19:38
The two_buckets_to_str() procedure relies on fixed bucket_width
which is not going to be fixed in the future. Since the procedure
is used only to generate a hint that accompanies an error message,
the simplest solution is to remove this procedure. We can improve
error messages later if that would be necessary.
@afiskon afiskon force-pushed the remove_two_buckets_to_str_procedure branch from 3b93695 to 856b3ff Compare June 14, 2021 20:19
@afiskon afiskon merged commit 4dc4e09 into master Jun 15, 2021
@svenklemm svenklemm deleted the remove_two_buckets_to_str_procedure branch September 20, 2021 07:04
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.

None yet

4 participants