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

Account for CPU tuple cost in DecompressChunk #7551

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

akuzm
Copy link
Member

@akuzm akuzm commented Dec 19, 2024

It was always zero due to a typo.

Disable-check: force-changelog-file

akuzm added a commit that referenced this pull request Dec 23, 2024
This makes things flakier.


Tested here: #7551
Copy link

codecov bot commented Jan 6, 2025

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.32%. Comparing base (59f50f2) to head (e139896).
Report is 688 commits behind head on main.

Files with missing lines Patch % Lines
tsl/src/nodes/decompress_chunk/decompress_chunk.c 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7551      +/-   ##
==========================================
+ Coverage   80.06%   82.32%   +2.25%     
==========================================
  Files         190      238      +48     
  Lines       37181    43724    +6543     
  Branches     9450    10972    +1522     
==========================================
+ Hits        29770    35994    +6224     
- Misses       2997     3397     +400     
+ Partials     4414     4333      -81     

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

Copy link
Contributor

@mkindahl mkindahl left a comment

Choose a reason for hiding this comment

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

You are removing the costs from most of the plans (which I think is a good thing since it reduces the risk of flakes and the costs are not part of these tests), but I do not see any test that demonstrates that your fix actually works, i.e., that the cost is non-zero for some query.

@akuzm
Copy link
Member Author

akuzm commented Jan 9, 2025

You are removing the costs from most of the plans (which I think is a good thing since it reduces the risk of flakes and the costs are not part of these tests), but I do not see any test that demonstrates that your fix actually works, i.e., that the cost is non-zero for some query.

All the plan changes demonstrate that it actually has an effect, no? This is how we normally check the cost changes.

@mkindahl
Copy link
Contributor

mkindahl commented Jan 9, 2025

You are removing the costs from most of the plans (which I think is a good thing since it reduces the risk of flakes and the costs are not part of these tests), but I do not see any test that demonstrates that your fix actually works, i.e., that the cost is non-zero for some query.

All the plan changes demonstrate that it actually has an effect, no? This is how we normally check the cost changes.

Well, the fact that there are plan changes demonstrate that there is a cost changes, but not in what direction. For example, if the CPU cost is zero for DecompressChunk why is it then replacing the index scan under the DecompressChunk with a sequence scan?

Also, if I look at this line from the patch (it is a removed row), it does not show up as zero, so it is not clear what this does.

->  Custom Scan (DecompressChunk) on _timescaledb_internal._hyper_1_1_chunk  (cost=1.06..63.44 rows=3000 width=12)

@akuzm
Copy link
Member Author

akuzm commented Jan 9, 2025

Also, if I look at this line from the patch (it is a removed row), it does not show up as zero, so it is not clear what this does.

It shows the cost of the underlying plan.

I can add a trivial test that shows costs, but not sure what use it has, and it's going to make the tests more fragile because the costs sometimes fluctuate because of floating point errors and so on.

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