feat: Upgrade velox submodule to y-scope/velox@7c55762. #59
Conversation
WalkthroughUpdates the presto-native-execution/velox subproject pointer from commit d78134d58d09d2c138bec52fde924d3b120f7fb6 to 7c55762bc37edc3f93cb8f5f1ff625e06cc10deb. No code or API signatures changed. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 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: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 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). (15)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-plan-determinism)
- GitHub Check: test (8.0.442, :presto-main-base)
- GitHub Check: test (17.0.13, :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-tpch-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-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: test (8.0.442, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: prestocpp-linux-build-for-test
- GitHub Check: maven-checks (17.0.13)
- GitHub Check: maven-checks (8.0.442)
- GitHub Check: prestissimo-worker-images-build
🔇 Additional comments (2)
presto-native-execution/velox (2)
1-1: Run native/connector smoke tests to catch ABI or behaviour drift
Velox upgrades may impact vector encodings, function registry, memory management and CLP integration. Ensure Maven is installed, then run:mvn -pl :presto-native-execution -am test mvn -Dtest=*CLP*Test* testShare any test failures for triage.
1-1: Confirmed Velox submodule at SHA 613630d includes all intended CLP connector updates (PRs #15, #16, #33, #34, #50, #55).
| @@ -1 +1 @@ | |||
| Subproject commit d78134d58d09d2c138bec52fde924d3b120f7fb6 | |||
| Subproject commit 7c55762bc37edc3f93cb8f5f1ff625e06cc10deb | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Revisit NOTICE/LICENCE and third‑party attributions if CLP/licencing changed.
Vendor/licence shifts in Velox/CLP may require updating NOTICE/LICENCE and any THIRD_PARTY files in this repo.
I can draft updates once you confirm any upstream licence deltas.
🤖 Prompt for AI Agents
In presto-native-execution/velox around lines 1 to 1, confirm whether upstream
Velox or its Contributor License Policy (CLP)/licensing has changed; if so,
update this repo's NOTICE, LICENSE, and any THIRD_PARTY/THIRD_PARTY_NOTICES
files to reflect the new licensing and vendor attributions. Specifically,
document the exact upstream licence delta(s), add or modify copyright and
attribution entries for affected third‑party components, include any required
license texts or notices, run any repository license audit/tooling to verify
coverage, and commit the updated NOTICE/LICENSE/THIRD_PARTY files with a short
commit message referencing the upstream change.
🧹 Nitpick (assertive)
Cache keys should include Velox SHA.
If CI caches native build artefacts, ensure keys incorporate the new submodule SHA to avoid stale objects.
🤖 Prompt for AI Agents
In presto-native-execution/velox around lines 1 to 1, the CI cache key for
native build artifacts doesn't include the Velox submodule SHA which can cause
stale cached outputs; update the workflow to append the Velox submodule commit
id (e.g. obtained via git submodule status --recursive or git rev-parse
HEAD:velox) to the cache key (or include a computed velox_sha step output) so
the cache key changes whenever the velox submodule SHA changes, ensuring cached
artifacts are invalidated when the submodule updates.
🛠️ Refactor suggestion
Ensure recursive submodule initialisation in CI and local build scripts.
Missing a recursive update will lead to mismatches if Velox itself references nested deps.
Add (or confirm) this in all CI jobs and build docs before configure/build steps:
git submodule sync --recursive
git submodule update --init --recursive🤖 Prompt for AI Agents
In presto-native-execution/velox around lines 1-1, CI and local build steps
currently don't enforce recursive git submodule init/update which can cause
mismatched nested dependencies; update all CI job scripts (GitHub Actions
workflows, CI shell scripts) and local build/bootstrap scripts as well as
README/build docs to run a sync and recursive init/update before any
configure/build steps — specifically call git submodule sync --recursive
followed by git submodule update --init --recursive (or ensure existing wrappers
invoke these) so nested submodules are always fetched and in sync prior to
building.
🧹 Nitpick (assertive)
Pin to a tag or document the rationale for SHA pinning.
If a stable tag exists for this Velox change, prefer pinning to it; otherwise, note the SHA and upstream PR in the PR body for traceability.
🤖 Prompt for AI Agents
In presto-native-execution/velox around lines 1–1, the dependency is pinned to a
raw commit SHA with no rationale or traceability; either change the pin to the
appropriate stable tag for this Velox change (update the submodule/reference to
that tag) or, if tagging is not available, add documentation: include the exact
SHA, the upstream PR/commit URL, and a brief rationale for SHA pinning in the PR
body and a comment next to the pin in the repo so future reviewers can trace it.
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
feat: Upgrade velox submodule to y-scope/velox@7c55762.
anlowee
changed the title
2 participants
Description
This PR is to update the Velox submodule to sync the merged PR y-scope/velox#26
Checklist
breaking change.
Validation performed
Passed the CI.
Summary by CodeRabbit