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

Correctly record the rawInputBytes metric in TableScan #8545

Closed
wants to merge 2 commits into from

Conversation

JkSelf
Copy link
Collaborator

@JkSelf JkSelf commented Jan 26, 2024

Follow up #8536 and #8147.

This PR adds a check to set the size to 0 if the difference between the size and overread is less than 0.

Copy link

netlify bot commented Jan 26, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 4e56dd8
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/65b6fef31e8f0a00085bcc00

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 26, 2024
@JkSelf
Copy link
Collaborator Author

JkSelf commented Jan 26, 2024

@Yuhta Can you help to review again? Thanks for your help.

@@ -274,6 +274,7 @@ std::vector<cache::CachePin> DirectCoalescedLoad::loadData(bool isPrefetch) {
}
input_->read(buffers, requests_[0].region.offset, LogType::FILE);
ioStats_->read().increment(size);
ioStats_->incRawBytesRead((size > overread) ? size - overread : 0);
Copy link
Contributor

@Yuhta Yuhta Jan 26, 2024

Choose a reason for hiding this comment

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

I think we need to fix the calculation of size here. size += request.loadSize instead of size += std::min<int32_t>(loadQuantum_, region.length) above probably. Do you think that makes sense? Can you think about a test case to cover that?

Copy link

stale bot commented Apr 28, 2024

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions!

@stale stale bot added the stale label Apr 28, 2024
@stale stale bot closed this May 12, 2024
facebook-github-bot pushed a commit that referenced this pull request Jun 17, 2024
…redInput (#9801)

Summary:
Follow up #8545

Pull Request resolved: #9801

Reviewed By: Yuhta

Differential Revision: D58596780

Pulled By: kagamiori

fbshipit-source-id: ff2b15b57a3214ae76db29b8791e3a1a84512e0a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants