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.

Sign up for GitHub

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

Jump to bottom

fix: Fix Compression file check output error messages #12009

Closed
wants to merge 1 commit into from
Closed

fix: Fix Compression file check output error messages #12009

wants to merge 1 commit into from

Conversation

jkhaliqi
Copy link
Contributor

@jkhaliqi jkhaliqi commented Jan 3, 2025

fixes: #11657

@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 3, 2025
@netlify Netlify
Copy link

netlify bot commented Jan 3, 2025 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit c8919a6
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6799a17630fa9a0008c369e0

@jkhaliqi jkhaliqi changed the title fix:Fix check output in Compression.cpp fix:Fix DWIO_ENSURE check output Jan 3, 2025
@jkhaliqi jkhaliqi changed the title fix:Fix DWIO_ENSURE check output fix:Fix Compression file check output Jan 3, 2025
@jkhaliqi jkhaliqi changed the title fix:Fix Compression file check output Fix:Fix Compression file check output Jan 3, 2025
@jkhaliqi jkhaliqi changed the title Fix:Fix Compression file check output fix: Fix Compression file check output Jan 3, 2025
@jkhaliqi jkhaliqi force-pushed the jk_dwio_ensure branch 2 times, most recently from f95ea89 to b4a6a93 Compare January 3, 2025 17:36
@prestodb-ci
Copy link

@ethanyzhang imported this issue into IBM GitHub Enterprise

zuyu
zuyu reviewed Jan 7, 2025
View reviewed changes
velox/dwio/common/compression/Compression.cpp Outdated
"{} decompression failed, input len is too small: {}",
kind_,
compressedSize);
fmt::format(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: plz add fmt header

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in thank you!

zuyu
zuyu reviewed Jan 9, 2025
View reviewed changes
velox/dwio/common/compression/Compression.cpp Outdated
compressedSize);
fmt::format(
"{} decompression failed, input len is too small: {}",
kind_,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace all occurrences of kind_ w/ ::facebook::velox::common::compressionKindToString(kind_), instead of kind_

@jkhaliqi jkhaliqi force-pushed the jk_dwio_ensure branch 2 times, most recently from 6b07bfa to 11f2585 Compare January 10, 2025 21:41
@aditi-pandit aditi-pandit changed the title fix: Fix Compression file check output fix: Fix Compression file check output error messages Jan 17, 2025
aditi-pandit
aditi-pandit approved these changes Jan 17, 2025
View reviewed changes
Copy link
Collaborator

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @jkhaliqi

majetideepak
majetideepak reviewed Jan 21, 2025
View reviewed changes
velox/dwio/common/compression/Compression.cpp Outdated
Comment on lines 250 to 251
"{} decompression failed, input len is too small: {}",
::facebook::velox::common::compressionKindToString(kind_),
Copy link
Collaborator

@majetideepak majetideepak Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

CompressionKind has an fmt formatter. Can we skip this function call?

velox/velox/common/compression/Compression.h

Line 50 in 54c6d9e

struct fmt::formatter<facebook::velox::common::CompressionKind>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the find @majetideepak! @zuyu Looks like function call can be skipped so made all occurrences switch to just kind_.

@majetideepak majetideepak added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Jan 22, 2025
@jkhaliqi jkhaliqi force-pushed the jk_dwio_ensure branch 6 times, most recently from 19bd93f to fa1b902 Compare January 28, 2025 03:19
@facebook-github-bot
Copy link
Contributor

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

bikramSingh91
bikramSingh91 reviewed Jan 29, 2025
View reviewed changes
velox/dwio/common/compression/Compression.cpp Outdated
kind_,
remainingOutputSize,
decompressedBlockSize);
fmt::format(
Copy link
Contributor

Choose a reason for hiding this comment

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

Internally someone pointed out that DWIO_ENSURE_GE eventually uses folly::toString which concatenates consecutive strings. So it should be used kind of like using std::count. so the right way would be:

    DWIO_ENSURE_GE(
        remainingOutputSize,
        decompressedBlockSize,
        ::facebook::velox::common::compressionKindToString(kind_),
        " decompression failed, remainingOutputSize is less than "
        "decompressedBlockSize, remainingOutputSize: ",
        remainingOutputSize,
        ", decompressedBlockSize: ",
        decompressedBlockSize);

which is translated to:

({
  auto const& _tmp = (remainingOutputSize >= decompressedBlockSize);
  _tmp ? _tmp
       : throw facebook::velox::dwio::common::exception::LoggedException(
             "_file_name_",
             266,
             "_function_name_",
             "remainingOutputSize >= decompressedBlockSize",
             ::folly::to<std::string>(
                 "[Range Constraint Violation : ",
                 remainingOutputSize,
                 ">=",
                 decompressedBlockSize,
                 "] : ",
                 ::facebook::velox::common::compressionKindToString(kind_),
                 " decompression failed, remainingOutputSize is less than "
                 "decompressedBlockSize, remainingOutputSize: ",
                 remainingOutputSize,
                 ", decompressedBlockSize: ",
                 decompressedBlockSize),
             ::facebook::velox::error_source::kErrorSourceRuntime,
             ::facebook::velox::error_code::kUnknown);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bikramSingh91 Thanks for letting me know, seems to work as expected with these changes! Updated the code accordingly!

@jkhaliqi jkhaliqi force-pushed the jk_dwio_ensure branch 2 times, most recently from 00b551e to 67ec72d Compare January 29, 2025 03:26
@facebook-github-bot
Copy link
Contributor

@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@bikramSingh91 merged this pull request in 4bd1221.

ArnavBalyan pushed a commit to ArnavBalyan/velox that referenced this pull request Jan 31, 2025
@jkhaliqi @ArnavBalyan
fix: Fix Compression file check output error messages (facebookincuba…
c5d31d4 
…tor#12009)

Summary:
fixes: facebookincubator#11657

Pull Request resolved: facebookincubator#12009

Reviewed By: xiaoxmeng

Differential Revision: D68781620

Pulled By: bikramSingh91

fbshipit-source-id: 4ccd11a4f6fa94bffea3498b55af378abccd656a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zuyu zuyu zuyu approved these changes

@bikramSingh91 bikramSingh91 bikramSingh91 left review comments

@majetideepak majetideepak majetideepak approved these changes

@aditi-pandit aditi-pandit aditi-pandit approved these changes

@xiaoxmeng xiaoxmeng Awaiting requested review from xiaoxmeng

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

dwio/common/compression/Compression.cpp DWIO_ENSURE_LE check output NOT as expected
7 participants
@jkhaliqi @prestodb-ci @facebook-github-bot @zuyu @majetideepak @bikramSingh91 @aditi-pandit