[chore](thirdparty) Upgrade snappy and xxhash#60519
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
There was a problem hiding this comment.
Pull request overview
This PR upgrades two third-party dependencies: snappy from version 1.1.8 to 1.1.10, and xxhash from version 0.8.1 to 0.8.3. The upgrade includes necessary build configuration changes for snappy, including RTTI enablement and improved CMake flags, plus a new patch to fix a sign-compare compiler warning in snappy 1.1.10.
Changes:
- Upgraded snappy dependency from 1.1.8 to 1.1.10 with updated download URL and MD5 hash
- Upgraded xxhash dependency from 0.8.1 to 0.8.3 with updated download URL and MD5 hash
- Added patch file for snappy 1.1.10 to fix sign-compare warning
- Updated snappy build script to enable RTTI and use standard CMake boolean flags
- Updated libuuid download URL to use generic SourceForge mirror
- Updated CHANGELOG with version modifications
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| thirdparty/vars.sh | Updated version numbers, download URLs, and MD5 checksums for snappy (1.1.10), xxhash (0.8.3), and libuuid download URL |
| thirdparty/patches/snappy-1.1.10-sign-compare.patch | Added new patch to fix sign-compare compiler warning by adding explicit cast to ptrdiff_t |
| thirdparty/build-thirdparty.sh | Modified snappy build to enable RTTI via sed, changed CMake flags from numeric (0) to boolean (OFF), added SNAPPY_BUILD_BENCHMARKS=OFF flag |
| thirdparty/CHANGELOG.md | Added entry for version 20260202 documenting the snappy and xxhash upgrades |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cd "${TP_SOURCE_DIR}/${SNAPPY_SOURCE}" | ||
|
|
||
| # Enable RTTI for snappy (required by Doris BE for SnappySlicesSource inheritance) | ||
| sed -i 's/-fno-rtti/-frtti/g' CMakeLists.txt |
There was a problem hiding this comment.
The sed command to enable RTTI is executed every time the build function runs. While this works because the source is only unpacked once, there's a potential issue: if someone manually modifies CMakeLists.txt and then reruns the build, the sed command could fail or have unexpected behavior if the pattern no longer matches.
Consider making this more robust by either:
- Checking if the modification has already been applied before running sed
- Applying this change as a proper patch in download-thirdparty.sh (similar to how other libraries are patched)
Example for option 1:
if grep -q '\-fno-rtti' CMakeLists.txt; then
sed -i 's/-fno-rtti/-frtti/g' CMakeLists.txt
fiOption 2 would be more consistent with how other libraries (like glog, mysql, etc.) are patched in this codebase.
| sed -i 's/-fno-rtti/-frtti/g' CMakeLists.txt | |
| if grep -q '\-fno-rtti' CMakeLists.txt; then | |
| sed -i 's/-fno-rtti/-frtti/g' CMakeLists.txt | |
| fi |
| SNAPPY_DOWNLOAD="https://github.com/google/snappy/archive/1.1.10.tar.gz" | ||
| SNAPPY_NAME=snappy-1.1.10.tar.gz | ||
| SNAPPY_SOURCE=snappy-1.1.10 | ||
| SNAPPY_MD5SUM="70153395ebe6d72febe2cf2e40026a44" |
There was a problem hiding this comment.
The LICENSE-dist.txt file still references the old versions of snappy (1.1.8) and xxhash (0.8.1) on lines 1497 and 1522 respectively. These should be updated to reflect the new versions (snappy 1.1.10 and xxhash 0.8.3) being introduced in this PR.
This file documents the versions of third-party dependencies included in the distribution and should be kept in sync with actual dependency versions.
| } | ||
| } while (ip < ip_limit_min_slop && | ||
| - (op + deferred_length) < op_limit_min_slop); | ||
| + static_cast<ptrdiff_t>(op + deferred_length) < op_limit_min_slop); |
There was a problem hiding this comment.
A new patch file snappy-1.1.10-sign-compare.patch has been added, but there is no corresponding patch application logic in thirdparty/download-thirdparty.sh.
Looking at the existing pattern in download-thirdparty.sh (e.g., lines 252-269 for glog, lines 272-280 for mysql), you need to add a snappy patch section after the unpacking phase. The section should follow this pattern:
# snappy patch
if [[ " ${TP_ARCHIVES[*]} " =~ " SNAPPY " ]]; then
if [[ "${SNAPPY_SOURCE}" == "snappy-1.1.10" ]]; then
cd "${TP_SOURCE_DIR}/${SNAPPY_SOURCE}"
if [[ ! -f "${PATCHED_MARK}" ]]; then
patch -p1 <"${TP_PATCH_DIR}/snappy-1.1.10-sign-compare.patch"
touch "${PATCHED_MARK}"
fi
cd -
fi
echo "Finished patching ${SNAPPY_SOURCE}"
fi
Without this, the patch file will not be applied during the build process, and the sign-compare issue it's meant to fix will persist.
| + static_cast<ptrdiff_t>(op + deferred_length) < op_limit_min_slop); | |
| + reinterpret_cast<ptrdiff_t>(op + deferred_length) < op_limit_min_slop); |
|
|
||
| # libuuid | ||
| LIBUUID_DOWNLOAD="https://nchc.dl.sourceforge.net/project/libuuid/libuuid-1.0.3.tar.gz" | ||
| LIBUUID_DOWNLOAD="https://downloads.sourceforge.net/project/libuuid/libuuid-1.0.3.tar.gz" |
There was a problem hiding this comment.
The libuuid download URL has been changed from a specific mirror (nchc.dl.sourceforge.net) to the generic SourceForge downloads URL (downloads.sourceforge.net). While this is a good change for reliability, it is not mentioned in the PR description or CHANGELOG. Consider either:
- Adding this to the CHANGELOG as a separate bullet point
- Mentioning it in the PR description
This helps with transparency and makes it easier to track what changed in this version.
TPC-H: Total hot run time: 31212 ms |
ClickBench: Total hot run time: 28.72 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
TPC-H: Total hot run time: 31286 ms |
ClickBench: Total hot run time: 28.78 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)