Skip to content

feat: Update to the latest Velox submodule to add support for searching archives from S3; Globally ignore stringop-overflow warnings to mitigate GCC bug which is triggered in 3rd-party code. #52

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

Merged
anlowee merged 6 commits into from
Aug 12, 2025
Merged

feat: Update to the latest Velox submodule to add support for searching archives from S3; Globally ignore stringop-overflow warnings to mitigate GCC bug which is triggered in 3rd-party code. #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0597c11
Bump velox
anlowee Aug 12, 2025
25fab3e
Try fix LTO
anlowee Aug 12, 2025
7522652
Revert "Try fix LTO"
kirkrodrigues Aug 12, 2025
0ce1c92
Try disabling stringop-overflow which is buggy in GCC 12.2.
kirkrodrigues Aug 12, 2025
b8470f5
Fix previous commit.
kirkrodrigues Aug 12, 2025
be052cb
Format
anlowee Aug 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions presto-native-execution/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,23 @@ set(DISABLED_WARNINGS
-Wno-deprecated-declarations \
-Wno-restrict")

# Disable -Wstringop-overflow to avoid a false positive in the following
# compiler versions. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117983
#
# NOTE: Typically, this is disabled using a VELOX_SUPPRESS_STRINGOP guard within
# the code; but that doesn't apply to 3rd-party libraries, so we need to disable
# it globally for now.
Comment on lines +44 to +46

@coderabbitai coderabbitai Bot Aug 12, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Clarify the comment to reflect intent and scope trade-offs.

If you keep a project-wide suppression, explicitly call out the risk, and mark a TODO to scope it down later.

-# NOTE: Typically, this is disabled using a VELOX_SUPPRESS_STRINGOP guard within
-# the code; but that doesn't apply to 3rd-party libraries, so we need to disable
-# it globally for now.
+# NOTE: VELOX_SUPPRESS_STRINGOP guards apply within Velox sources only; they do not
+# cover third-party targets. As a stopgap, we apply a project-wide suppression here.
+# TODO: Replace with a target-scoped suppression for the affected 3rd-party target(s).
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# NOTE: Typically, this is disabled using a VELOX_SUPPRESS_STRINGOP guard within
# the code; but that doesn't apply to 3rd-party libraries, so we need to disable
# it globally for now.
# NOTE: VELOX_SUPPRESS_STRINGOP guards apply within Velox sources only; they do not
# cover third-party targets. As a stopgap, we apply a project-wide suppression here.
# TODO: Replace with a target-scoped suppression for the affected 3rd-party target(s).
🤖 Prompt for AI Agents 
In presto-native-execution/CMakeLists.txt around lines 44 to 46, the current
comment explains disabling a string operation guard globally but doesn't call
out the associated risk or future work; update the comment to explicitly state
this is a project-wide suppression, enumerate the risk (e.g., affects all
3rd-party libraries and may hide bugs or performance issues), and add a TODO
noting that the suppression should be scoped to specific targets or dependencies
later with steps to revisit and tighten the guard.

if("GNU" STREQUAL "${CMAKE_CXX_COMPILER_ID}"
AND ((CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 12
AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 12.5)
OR (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 13
AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 13.4)
OR (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 14
AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 14.3)
))
set(DISABLED_WARNINGS "${DISABLED_WARNINGS} -Wno-stringop-overflow")
endif()

# Important warnings that must be explicitly enabled.
set(ENABLE_WARNINGS "-Wreorder")

Expand Down
2 changes: 1 addition & 1 deletion presto-native-execution/velox
Open in desktop
Submodule velox updated 16 files
+1 −1 CMake/resolve_dependency_modules/clp.cmake
+0 −1 velox/connectors/clp/CMakeLists.txt
+39 −1 velox/connectors/clp/ClpConfig.cpp
+12 −7 velox/connectors/clp/ClpConfig.h
+5 −1 velox/connectors/clp/ClpDataSource.cpp
+3 −0 velox/connectors/clp/ClpDataSource.h
+4 −0 velox/connectors/clp/search_lib/CMakeLists.txt
+56 −0 velox/connectors/clp/search_lib/ClpPackageS3AuthProvider.cpp
+50 −0 velox/connectors/clp/search_lib/ClpPackageS3AuthProvider.h
+73 −0 velox/connectors/clp/search_lib/ClpS3AuthProviderBase.cpp
+71 −0 velox/connectors/clp/search_lib/ClpS3AuthProviderBase.h
+1 −1 velox/connectors/clp/tests/CMakeLists.txt
+242 −0 velox/connectors/clp/tests/ClpConfigTest.cpp
+1 −3 velox/connectors/clp/tests/ClpConnectorTest.cpp
+33 −0 velox/docs/configs.rst
+27 −7 velox/docs/develop/connectors.rst
Loading