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 superfluous checks in partialize_agg #6327

Merged
merged 1 commit into from
Nov 21, 2023
Merged

Remove superfluous checks in partialize_agg #6327

merged 1 commit into from
Nov 21, 2023

Conversation

svenklemm
Copy link
Member

@svenklemm svenklemm commented Nov 19, 2023

Remove checks that will always return true in current implementation to make refactoring easier. Aggref->args will always be list of TargetEntry, and the bulk_decompression check will always return true as well.

Disable-check: force-changelog-file

Copy link

@mkindahl, @mahipv: please review this pull request.

Powered by pull-review

Copy link

codecov bot commented Nov 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d1c8cb0) 65.44% compared to head (53c7b96) 65.39%.
Report is 2 commits behind head on main.

❗ Current head 53c7b96 differs from pull request most recent head 6c081dd. Consider uploading reports for the commit 6c081dd to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6327      +/-   ##
==========================================
- Coverage   65.44%   65.39%   -0.05%     
==========================================
  Files         249      249              
  Lines       57984    57916      -68     
  Branches    12918    12891      -27     
==========================================
- Hits        37946    37876      -70     
- Misses      18198    18201       +3     
+ Partials     1840     1839       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jnidzwetzki
Copy link
Contributor

@svenklemm Could you elaborate what kind of refactoring this function is currently preventing?

It is correct, the function never returns false currently. However, the function is part of the initiative to make the vectoized sum() aggregation implementation more generic and introduce infrastructure for all kinds of aggregates. As soon as we support more data types, we need this kind of check in the planner.

@svenklemm
Copy link
Member Author

Any code that references FormData_hypertable_compression needs to be adjusted cause the corresponding table is gonna get removed. The structure of the new settings will also be very different (no more positional indexes) so any place that references it now usually needs quite a few changes.

@mkindahl mkindahl removed their request for review November 20, 2023 13:15
@svenklemm
Copy link
Member Author

If we depend on this providing the correct answer about used algorithm the current implementation is insufficient. To get a definitive answer about algorithms used you have to check the batch header as the compressor will deviate from whatever is stored in Hypertable_compression.

@svenklemm svenklemm requested a review from akuzm November 21, 2023 09:47
Remove checks that will always return true in current implementation
to make refactoring easier. Aggref->args will always be list of
TargetEntry, and the bulk_decompression check will always return true
as well.
@svenklemm svenklemm merged commit 03ef1ab into main Nov 21, 2023
38 checks passed
@svenklemm svenklemm deleted the deadcode branch November 21, 2023 16:26
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.

4 participants