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: GCC13 build failures #12517

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

fix: GCC13 build failures #12517

wants to merge 5 commits into from

Conversation

xin-zhang2
Copy link
Contributor

Fix #12497 .

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

netlify bot commented Mar 3, 2025 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 9addb45
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67cefebda1543f0008437e1b

@xin-zhang2 xin-zhang2 changed the title Fix GCC13 build failures fix: Fix GCC13 build failures Mar 3, 2025
@xin-zhang2 xin-zhang2 force-pushed the gcc13 branch 2 times, most recently from cbc2221 to 15aaf7b Compare March 4, 2025 09:54
@xin-zhang2 xin-zhang2 marked this pull request as ready for review March 4, 2025 16:44
@majetideepak majetideepak changed the title fix: Fix GCC13 build failures fix: GCC13 build failures Mar 4, 2025
@majetideepak majetideepak requested a review from czentgr March 4, 2025 17:41
majetideepak
majetideepak reviewed Mar 6, 2025
View reviewed changes
velox/dwio/dwrf/common/Compression.h Outdated
@@ -87,7 +87,7 @@ inline std::unique_ptr<dwio::common::BufferedOutputStream> createCompressor(

inline CompressionOptions getDwrfOrcDecompressionOptions(
common::CompressionKind kind) {
CompressionOptions options;
CompressionOptions options{};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why add {} here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initialize options explicitly to avoid using it uninitialized.

/root/velox/./velox/dwio/dwrf/common/Compression.h:99:10: error: ‘options’ is used uninitialized [-Werror=uninitialized]
   99 |   return options;
      |          ^~~~~~~
/root/velox/./velox/dwio/dwrf/common/Compression.h: In member function ‘virtual void TestSeek_uncompressed_Test::TestBody()’:
/root/velox/./velox/dwio/dwrf/common/Compression.h:90:22: note: ‘options’ declared here
   90 |   CompressionOptions options;
      |                      ^~~~~~~

Copy link
Collaborator

@majetideepak majetideepak Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Let's initialize the fields in the struct CompressionOptions
example: uint32_t compressionThreshold; must be uint32_t compressionThreshold{0};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initialized the fields.
There is no default value for CompressionOptions.format, so I left it empty initialized, which results in format.zlib.windowBits and format.zlib.compressionLevel being initialized to 0.
As for CompressionOptions.compressionThreshold, it is currently used only by DwrfOrcCompression, where its value is configured via orc.compression.threshold, with a default of 256. However, since CompressionOptions is part of the common package, I believe it would be more appropriate to initialize compressionThreshold to 0.
Does the approach seem reasonable?

velox/expression/tests/ExpressionRunner.cpp Outdated
inputVector,
"Input vector is not a RowVector: {}",
inputVector->toString());
VELOX_CHECK_NOT_NULL(inputVector, "Input vector can't be null.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why change this error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When inputVector is nullptr, inputVector->toString() in the previous message will cause a null pointer dereference.

Copy link
Collaborator

Choose a reason for hiding this comment

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

auto vector = restoreVectorFromFile(inputPathsList[i].c_str(), deserializerPool.get());
auto inputVector = std::dynamic_pointer_cast<RowVector>(vector);

use vector->toString() in the null check argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@xin-zhang2 xin-zhang2 requested a review from majetideepak March 10, 2025 09:44
@majetideepak
Copy link
Collaborator

majetideepak commented Mar 10, 2025 •
edited
Loading

@xin-zhang2 The experimental CI job that uses GCC 13 needs to be triggered to test this. Can you add a comment somewhere in .github/workflows/experimental.yml to trigger the CI?
We can revert the comment after testing.
Thanks.

@rui-mo rui-mo mentioned this pull request Mar 10, 2025
Closed
@xin-zhang2
Copy link
Contributor Author

@majetideepak I've added a comment to trigger the CI, but it looks the Experimental Fuzzer Job didn't pick my code.
For example, the error message in the job is

/home/runner/work/velox/velox/velox/velox/vector/tests/VectorMakerTest.cpp:477:65: error: converting to ‘std::optional<std::vector<std::optional<int>, std::allocator<std::optional<int> > > >’ from initializer list would use explicit constructor ‘constexpr std::optional<_Tp>::optional(std::in_place_t, _Args&& ...) [with _Args = {}; typename std::enable_if<__and_v<std::is_constructible<_Tp, _Args ...> >, bool>::type <anonymous> = false; _Tp = std::vector<std::optional<int>, std::allocator<std::optional<int> > >]’
  477 |   VectorPtr expectedVector = maker_.arrayVectorNullable<int32_t>({
      |                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
  478 |       {{1, 2}},
      |       ~~~~~~~~~                                                  
  479 |       {{2, 3, 4}},
      |       ~~~~~~~~~~~~                                               
  480 |       {{std::nullopt, 7}},
      |       ~~~~~~~~~~~~~~~~~~~~                                       
  481 |       {{1, 3, 7, 9}},
      |       ~~~~~~~~~~~~~~~                                            
  482 |       {{}},
      |       ~~~~~                                                      
  483 |       {{std::nullopt}},
      |       ~~~~~~~~~~~~~~~~~                                          
  484 |       {{1, 2, std::nullopt}},
      |       ~~~~~~~~~~~~~~~~~~~~~~~                                    
  485 |       {{}},
      |       ~~~~~                                                      
  486 |       std::nullopt,
      |       ~~~~~~~~~~~~~                                              
  487 |       {{1, 2, 3}},
      |       ~~~~~~~~~~~~                                               
  488 |       {{}},
      |       ~~~~~                                                      
  489 |       {{4, 5}},
      |       ~~~~~~~~~                                                  
  490 |   });
      |   ~~

while I have already made the following change in my PR.
image

Any idea on this?

@majetideepak
Copy link
Collaborator

@xin-zhang2 CI is checking out the main branch. Are you able to test this locally? If yes, we an undo the comment.

@xin-zhang2
Copy link
Contributor Author

@majetideepak Yes, the build runs successfully on my local ubuntu 24.04 enviroment.
I have reverted the comment commit.

@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 Mar 10, 2025
@majetideepak
Copy link
Collaborator

@xin-zhang2 thanks for this fix. Meta will import and merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@majetideepak majetideepak majetideepak approved these changes

@czentgr czentgr czentgr approved these changes

@assignUser assignUser Awaiting requested review from assignUser assignUser is a code owner

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. 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.

Fix build failures with GCC13
4 participants
@xin-zhang2 @majetideepak @czentgr @facebook-github-bot