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
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 5 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
15 changes: 15 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,21 @@ 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.
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Clarify comment: suppression is target-scoped, not global.

The note says “disable it globally,” but the code applies it only to log_surgeon. Adjust the wording to avoid confusion.

Apply this diff to align the comment with actual scope:

-# 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: Typically, this is disabled using a VELOX_SUPPRESS_STRINGOP guard within Velox sources; that
+# doesn't apply to 3rd-party libraries, so we apply a target-scoped suppression for log_surgeon.
📝 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: Typically, this is disabled using a VELOX_SUPPRESS_STRINGOP guard within Velox sources; that
# doesn't apply to 3rd-party libraries, so we apply a target-scoped suppression for log_surgeon.
🤖 Prompt for AI Agents 
In presto-native-execution/CMakeLists.txt around lines 35 to 36, the comment
incorrectly states the suppression is "global" while the CMake flag is applied
only to the log_surgeon target; update the wording to state the suppression is
target-scoped (applied to the log_surgeon target only) and explain briefly why a
target-scoped suppression is necessary for third-party libraries, replacing
"disable it globally" with language that accurately reflects the limited scope.

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)))
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Style nit: simplify CMake conditionals for readability.

Minor readability tweaks:

  • Use CMAKE_CXX_COMPILER_ID STREQUAL "GNU" (conventional ordering).
  • Keep version ranges as-is, but consider generator expressions if you need this inside target properties elsewhere.

Apply this diff for the condition syntax (when you relocate the block as suggested):

-if("GNU" STREQUAL "${CMAKE_CXX_COMPILER_ID}"
+if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU"
📝 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
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)))
if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU"
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)))
🤖 Prompt for AI Agents 
In presto-native-execution/CMakeLists.txt around lines 37-43, the if()
conditional currently uses "GNU" STREQUAL "${CMAKE_CXX_COMPILER_ID}" which is
unconventional; change it to CMAKE_CXX_COMPILER_ID STREQUAL "GNU" while keeping
the existing version range checks unchanged, and if you later move this check
into target properties use generator expressions instead of a plain if().

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