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 4 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 @@ -29,6 +29,21 @@ set(CMAKE_CXX_STANDARD_REQUIRED True)
message("Appending CMAKE_CXX_FLAGS with ${SCRIPT_CXX_FLAGS}")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${SCRIPT_CXX_FLAGS}")

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

@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 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)))

@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)

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

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
target_compile_options(log_surgeon PRIVATE "-Wno-stringop-overflow")
endif()

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

⚠️ Potential issue

Fix: target_compile_options is invoked before target creation (configure-time error).

CMake errors out if you call target_compile_options() on a target that hasn’t been created yet. At this location, log_surgeon is very likely not defined (add_subdirectory(velox) is at Line 234 and add_subdirectory(presto_cpp) is at Line 270). Move this block to after the target is created and guard it with if(TARGET log_surgeon).

Apply this diff to remove the premature block here:

-# 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.
-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)))
-    target_compile_options(log_surgeon PRIVATE "-Wno-stringop-overflow")
-endif()
+# (moved) GCC-specific -Wstringop-overflow suppression for log_surgeon is applied after the target is created.

Then, insert this block AFTER add_subdirectory(presto_cpp) (Line 270) so the target exists:

# Disable -Wstringop-overflow to avoid a false positive in the following compiler versions.
# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117983
# NOTE: VELOX_SUPPRESS_STRINGOP applies to Velox sources; for 3rd-party targets use target-level flags.
if(TARGET log_surgeon)
  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)))
    target_compile_options(log_surgeon PRIVATE -Wno-stringop-overflow)
  endif()
endif()

Alternatively (cleaner), keep it target-local and compiler/version-gated using generator expressions:

if(TARGET log_surgeon)
  target_compile_options(log_surgeon PRIVATE
    $<$<AND:$<CXX_COMPILER_ID:GNU>,$<VERSION_GREATER_EQUAL:$<CXX_COMPILER_VERSION>,12>,$<VERSION_LESS:$<CXX_COMPILER_VERSION>,12.5>>:-Wno-stringop-overflow>
    $<$<AND:$<CXX_COMPILER_ID:GNU>,$<VERSION_GREATER_EQUAL:$<CXX_COMPILER_VERSION>,13>,$<VERSION_LESS:$<CXX_COMPILER_VERSION>,13.4>>:-Wno-stringop-overflow>
    $<$<AND:$<CXX_COMPILER_ID:GNU>,$<VERSION_GREATER_EQUAL:$<CXX_COMPILER_VERSION>,14>,$<VERSION_LESS:$<CXX_COMPILER_VERSION>,14.3>>:-Wno-stringop-overflow>
  )
endif()
🤖 Prompt for AI Agents 
presto-native-execution/CMakeLists.txt lines 37-45: the target_compile_options()
call is applied to log_surgeon before that target is created, causing
configure-time errors; remove this premature block here and instead add a
guarded block after add_subdirectory(presto_cpp) (around line 270) so the target
exists, e.g. wrap the options in if(TARGET log_surgeon) and perform the
GCC-version checks before calling target_compile_options, or preferably use
target-local, compiler/version-gated generator expressions to conditionally add
-Wno-stringop-overflow for the specified GCC version ranges.


# Known warnings that are benign can be disabled:
#
# * `restrict` since it triggers a bug in gcc 12. See
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