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
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
presto-native-execution/velox(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-local-queries)
- GitHub Check: test (17.0.13, :presto-main-base)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-plan-determinism)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: test (8.0.442, :presto-main)
- GitHub Check: test (17.0.13, :presto-tests -P presto-tests-general)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-resource-manager)
- GitHub Check: test (8.0.442, :presto-main-base)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-local-queries)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-tpch-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-general)
- GitHub Check: prestissimo-worker-images-build
- GitHub Check: maven-checks (17.0.13)
- GitHub Check: maven-checks (8.0.442)
- GitHub Check: prestocpp-linux-build-for-test
🔇 Additional comments (2)
presto-native-execution/velox (2)
1-1: LGTM: Submodule bump matches the stated objective.Pointer-only change to Velox submodule; aligns with “add config to support searching archives from S3.” No other files touched.
1-1: Submodule SHA correctly matches y-scope/velox#21 merge commit
- The pinned SHA
d78134d58d09d2c138bec52fde924d3b120f7fb6equals the PR’smerge_commit_sha.- The commit is present in the upstream
y-scope/veloxrepository.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
presto-native-execution/velox (1)
1-1: Downstream wiring and tests for S3 archive search config appear absent.This mirrors a prior request: ensure the new Velox S3 archive search config is surfaced in Presto and covered by an integration test.
Use this scan to quickly check for wiring and tests outside the Velox submodule:
#!/usr/bin/env bash set -euo pipefail echo "🔎 Searching Presto (excluding submodule) for likely config and usage identifiers..." rg -n -i -S --glob '!presto-native-execution/velox/**' \ '(S3ArchiveSearch|archiveSearch|archive\s*search|S3.*archive|archive.*S3)' -C 3 || true echo echo "🔎 Searching Java tests for any archive-related coverage..." rg -n -i --glob '!presto-native-execution/velox/**' --glob '*Test.java' 'archive|S3.*archive' -C 3 || trueIf you’d like, I can draft:
- a minimal config plumbing plan (names/keys across layers), and
- a basic product/integration test exercising S3 archive search behind a feature flag.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
presto-native-execution/CMakeLists.txt(1 hunks)presto-native-execution/velox(1 hunks)
🔇 Additional comments (3)
presto-native-execution/velox (2)
1-1: Velox submodule pointer bump verified – all checks pass
- Submodule commit at HEAD matches expected SHA: d78134d58d09d2c138bec52fde924d3b120f7fb6
- .gitmodules correctly references
• path: presto-native-execution/velox
• URL: https://github.com/y-scope/velox.gitDocumentation updated to ensure reproducible builds.
1-1: Confirmed GCC warning suppression for log_surgeon is presentThe
presto-native-execution/CMakeLists.txtfile includes a version‐guarded block that applies-Wno-stringop-overflowto thelog_surgeontarget:
- File: presto-native-execution/CMakeLists.txt
- Lines: 37–45
No further action required.
presto-native-execution/CMakeLists.txt (1)
38-43: Verify placement and GCC version gatingFile: presto-native-execution/CMakeLists.txt
Lines: 38–43Snippet:
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)))Please address the following:
Locate
log_surgeontarget
I wasn’t able to find theadd_library(log_surgeon …)oradd_executable(log_surgeon …)definition. Confirm which CMakeLists file defines it so we can wrap the version guard around that target.Confirm GCC ranges against bug 117983
According to the patch threads, the false positive occurs in GCC 12.x (up to 12.4), 13.x (up to 13.3) and 14.x (up to 14.2), with fixes landing in the next point releases (12.5, 13.4, 14.3). The existing checks (>=12 && <12.5,>=13 && <13.4,>=14 && <14.3) correctly cover all affected versions.References for bug 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. |
There was a problem hiding this comment.
🧹 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.
| # 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))) | ||
| target_compile_options(log_surgeon PRIVATE "-Wno-stringop-overflow") | ||
| endif() |
There was a problem hiding this comment.
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.
| 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))) |
There was a problem hiding this comment.
🧹 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.
| 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().
| @@ -1 +1 @@ | |||
| Subproject commit 28be93b4d05bf6d9faa5058227dd4f8d4688b96d | |||
| Subproject commit d78134d58d09d2c138bec52fde924d3b120f7fb6 | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: Pin to a tagged Velox ref and add a release note.
If a stable tag exists for this commit, consider pinning to the tag in the PR description and adding a small release note in Presto’s changelog for traceability.
🤖 Prompt for AI Agents
In presto-native-execution/velox around lines 1 to 1, the PR currently
references a commit without a stable tag; update the PR description (or the
dependency reference) to pin Velox to the nearest stable tag or release ref
instead of a raw commit SHA, and add a brief release-note entry to Presto’s
changelog mentioning the Velox version/tag and the reason for the update for
traceability.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
presto-native-execution/CMakeLists.txt (1)
46-46: Style nit: prefer conventional CMake comparison form.Swap to CMAKE_CXX_COMPILER_ID STREQUAL "GNU" for readability and consistency.
Apply this minimal change if you keep a configure-time check:
-if("GNU" STREQUAL "${CMAKE_CXX_COMPILER_ID}" +if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU"
anlowee
changed the title
anlowee
changed the title
stringop-overflow warnings to mitigate GCC bug which is triggered in 3rd-party code.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
presto-native-execution/CMakeLists.txt (1)
41-56: Avoid global -Wno-stringop-overflow; scope it to the affected target(s) and tighten GCC ranges.Appending -Wno-stringop-overflow to DISABLED_WARNINGS makes it project-wide and risks masking real issues beyond the intended 3rd-party code. Apply the suppression target-locally (e.g., log_surgeon) after the target exists, and gate it narrowly to only the buggy GCC minors (fixes landed in 12.4, 13.3, 14.2).
Action:
- Remove the global block here.
- Add a target-scoped block after add_subdirectory(presto_cpp), guarded with if(TARGET log_surgeon), using compiler/version-gated generator expressions.
Apply this removal within the current block:
-# 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) - )) - set(DISABLED_WARNINGS "${DISABLED_WARNINGS} -Wno-stringop-overflow") -endif()Then, add this after add_subdirectory(presto_cpp) so the target exists:
# Suppress a GCC -Wstringop-overflow false positive for specific versions only. # See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117983 # VELOX_SUPPRESS_STRINGOP covers Velox sources; use target-level flags for 3rd-party targets. if(TARGET log_surgeon) target_compile_options(log_surgeon PRIVATE # GCC 12.0–12.3 $<$<AND:$<CXX_COMPILER_ID:GNU>,$<VERSION_GREATER_EQUAL:$<CXX_COMPILER_VERSION>,12>,$<VERSION_LESS:$<CXX_COMPILER_VERSION>,12.4>>:-Wno-stringop-overflow> # GCC 13.0–13.2 $<$<AND:$<CXX_COMPILER_ID:GNU>,$<VERSION_GREATER_EQUAL:$<CXX_COMPILER_VERSION>,13>,$<VERSION_LESS:$<CXX_COMPILER_VERSION>,13.3>>:-Wno-stringop-overflow> # GCC 14.0–14.1 $<$<AND:$<CXX_COMPILER_ID:GNU>,$<VERSION_GREATER_EQUAL:$<CXX_COMPILER_VERSION>,14>,$<VERSION_LESS:$<CXX_COMPILER_VERSION>,14.2>>:-Wno-stringop-overflow> ) endif()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
presto-native-execution/CMakeLists.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: test (17.0.13, :presto-main-base)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-tpch-distributed-queries)
- GitHub Check: test (17.0.13, :presto-tests -P presto-tests-general)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-resource-manager)
- GitHub Check: test (17.0.13, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-plan-determinism)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-tpch-distributed-queries)
- GitHub Check: test (8.0.442, :presto-main-base)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-local-queries)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-general)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: maven-checks (17.0.13)
- GitHub Check: maven-checks (8.0.442)
- GitHub Check: prestocpp-linux-build-for-test
- GitHub Check: prestissimo-worker-images-build
🔇 Additional comments (1)
presto-native-execution/CMakeLists.txt (1)
47-56: Tighten CMake conditional style and version rangesPlease switch to the idiomatic CMake
if(<var> STREQUAL <value>)form and narrow each range to exclude the fixed minor releases. For example:-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.4) OR + (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 13 AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 13.3) OR + (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 14 AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 14.2) + ))• File:
presto-native-execution/CMakeLists.txt, lines 47–56Please verify the exact GCC point‐release versions in your CI environment (e.g. run
gcc --versioninside theprestodb/presto-native-dependencycontainer) and adjust the upper‐bound cutoffs (12.4, 13.3, 14.2) to match the true false‐positive window.
| # 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. |
There was a problem hiding this comment.
🧹 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.
| # 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.
32e6b12
into
y-scope:release-0.293-clp-connector
3 participants
Description
This PR is to update the Velox submodule to sync the merged PR y-scope/velox#21
Checklist
breaking change.
Validation performed
Passed the CI.
Summary by CodeRabbit
Chores
Impact