ci: Simplify GH workflows for efficiency (PR from non-fork). #10
Conversation
|
Warning Rate limit exceeded@anlowee has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis update removes several GitHub Actions workflow files related to benchmarking, building, metrics, Docker images, macOS builds, preliminary checks, and experimental fuzzing. The remaining workflows are simplified, with some jobs renamed, triggers and concurrency conditions changed to focus on the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub Actions
participant CI Workflow
User->>GitHub Actions: Push/PR to presto-0.293-clp-connector
GitHub Actions->>CI Workflow: Trigger build (linux-build, scheduled, etc.)
CI Workflow->>CI Workflow: Run release build/test (no adapters, minimal jobs)
CI Workflow-->>GitHub Actions: Report status
sequenceDiagram
participant User
participant Makefile
participant check.py
User->>Makefile: make format-check/header-check (branch: presto-0.293-clp-connector)
Makefile->>check.py: Run check.py with branch arg
check.py->>check.py: Use merge-base with origin/presto-0.293-clp-connector
check.py-->>Makefile: Return results
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/benchmark.yml(0 hunks).github/workflows/breeze.yml(0 hunks).github/workflows/build-metrics.yml(0 hunks).github/workflows/build_pyvelox.yml(0 hunks).github/workflows/docker.yml(0 hunks).github/workflows/docs.yml(0 hunks).github/workflows/experimental.yml(0 hunks).github/workflows/linux-build-base.yml(0 hunks).github/workflows/linux-build.yml(2 hunks).github/workflows/macos.yml(0 hunks).github/workflows/preliminary_checks.yml(0 hunks).github/workflows/scheduled.yml(0 hunks)
💤 Files with no reviewable changes (11)
- .github/workflows/docs.yml
- .github/workflows/preliminary_checks.yml
- .github/workflows/breeze.yml
- .github/workflows/build_pyvelox.yml
- .github/workflows/macos.yml
- .github/workflows/benchmark.yml
- .github/workflows/build-metrics.yml
- .github/workflows/linux-build-base.yml
- .github/workflows/docker.yml
- .github/workflows/scheduled.yml
- .github/workflows/experimental.yml
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build.yml
84-84: property "use-clang" is not defined in object type {}
(expression)
180-180: property "use-clang" is not defined in object type {}
(expression)
🪛 YAMLlint (1.37.1)
.github/workflows/linux-build.yml
[error] 108-108: trailing spaces
(trailing-spaces)
🔇 Additional comments (12)
.github/workflows/linux-build.yml (12)
19-19: Review branch filter scope
The workflow only triggers onpresto-0.293-clp-connector. Confirm this aligns with all active development branches or if additional branches (e.g.,main) should also be included to ensure CI coverage.
52-53: Validate concurrency cancellation logic
Thecancel-in-progresscondition cancels runs whengithub.refis notrefs/heads/presto-0.293-clp-connector. Ensure this inverted logic matches intent—for example, do you want to cancel outdated runs on the same branch or only non-main branches?
57-64: Self-hosted runner and defaults configuration
Using a self-hosted Ubuntu 22.04 runner withbashshell and defaultveloxworking directory is correct for local tooling. Ensure the runner pool label remains accurate and that directory assumptions hold in all environments.
86-89: Ensure stash directory creation
Themkdir -p '${{ env.CCACHE_DIR }}'step correctly guarantees the stash path exists before use.
91-94: Checkout intoveloxsubdirectory
Mapping the repository intovelox/aligns with the working-directory default. This is consistent and maintainable.
95-98: Core dependencies installation
Sourcingscripts/setup-ubuntu.shfor apt packages centralises dependency management. This matches project conventions.
110-112: Reset CCache stats pre-build
ccache -szclears previous statistics, ensuring post-build metrics are meaningful. This is a best practice.
114-133: License header check
Themake header-fixtarget plus diff reporting in the job summary provides clear remediation steps for missing headers.
135-154: Formatting enforcement
make format-fixcombined with summary-patched diffs ensures code style consistency. This inline approach replaces separate checks cleanly.
156-165: Conditional compiler selection & build flags
TheUSE_CLANGcheck and environment-drivenMAKEFLAGS/EXTRA_CMAKE_FLAGSintegrate well. The string-based comparison works as expected in bash.
173-176: Post-build CCache stats
Displayingccache -vsafter the build gives visibility into cache effectiveness.
182-184: CTest invocation
Running tests withctest -j 8 --output-on-failure --no-tests=erroris clear and provides immediate diagnostics on failures.
.github/workflows/linux-build.yml
Outdated
| - uses: assignUser/stash/save@v1 | ||
| with: | ||
| path: '${{ env.CCACHE_DIR }}' | ||
| key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} | ||
|
|
There was a problem hiding this comment.
Fix undefined stash save key reference
Mirroring the restore issue, inputs.use-clang is invalid here. Update to reference the env var:
- key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }}
+ key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }}📝 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.
| - uses: assignUser/stash/save@v1 | |
| with: | |
| path: '${{ env.CCACHE_DIR }}' | |
| key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} | |
| - uses: assignUser/stash/save@v1 | |
| with: | |
| path: '${{ env.CCACHE_DIR }}' | |
| key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }} |
🧰 Tools
🪛 actionlint (1.7.7)
180-180: property "use-clang" is not defined in object type {}
(expression)
🤖 Prompt for AI Agents
In .github/workflows/linux-build.yml at lines 177 to 181, the stash save key
incorrectly references 'inputs.use-clang', which is undefined in this context.
Update the key to use the environment variable equivalent, such as
'env.USE_CLANG', to correctly reflect the clang or gcc choice. Modify the key
expression to conditionally use this environment variable instead of the invalid
input reference.
.github/workflows/linux-build.yml
Outdated
| - name: Get Ccache Stash | ||
| uses: assignUser/stash/restore@v1 | ||
| with: | ||
| path: '${{ env.CCACHE_DIR }}' | ||
| key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} |
There was a problem hiding this comment.
Fix undefined stash key reference
inputs.use-clang is undefined in this workflow context and will break ccache restore. It should reference the USE_CLANG env var instead.
Suggested diff:
- key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }}
+ key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }}📝 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.
| - name: Get Ccache Stash | |
| uses: assignUser/stash/restore@v1 | |
| with: | |
| path: '${{ env.CCACHE_DIR }}' | |
| key: ccache-ubuntu-debug-default-${{ inputs.use-clang && 'clang' || 'gcc' }} | |
| - name: Get Ccache Stash | |
| uses: assignUser/stash/restore@v1 | |
| with: | |
| path: '${{ env.CCACHE_DIR }}' | |
| key: ccache-ubuntu-debug-default-${{ env.USE_CLANG == 'true' && 'clang' || 'gcc' }} |
🧰 Tools
🪛 actionlint (1.7.7)
84-84: property "use-clang" is not defined in object type {}
(expression)
🤖 Prompt for AI Agents
In .github/workflows/linux-build.yml around lines 80 to 84, the stash key uses
an undefined variable `inputs.use-clang`. Replace this with the environment
variable `USE_CLANG` to correctly reference the clang usage flag. Update the key
expression to use `${{ env.USE_CLANG && 'clang' || 'gcc' }}` instead of `${{
inputs.use-clang && 'clang' || 'gcc' }}` to fix the undefined reference and
ensure proper ccache restore functionality.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
.github/workflows/linux-build.yml (1)
16-29: 💡 Verification agent🧩 Analysis chain
Verify and refine trigger filters
The workflow is now only triggered on pushes and PRs againstpresto-0.293-clp-connector. Confirm this aligns with your CI requirements—if you intend to run CI onmainor other branches, you’ll need to extend the branch list. Also, sincelinux-build-base.ymlhas been deleted, remove its entry from thepathsfilter to avoid referencing a non-existent file.Example diff to remove the stale path reference:
-on: - push: - branches: [presto-0.293-clp-connector] - paths: - - ".github/workflows/linux-build-base.yml" +on: + push: + branches: [presto-0.293-clp-connector] + paths: + # …other patterns…
🏁 Script executed:
#!/bin/bash set -e # Check if the referenced workflow file exists echo "Checking .github/workflows/linux-build-base.yml existence:" if [ -e ".github/workflows/linux-build-base.yml" ]; then echo "✔ .github/workflows/linux-build-base.yml is present" else echo "✖ .github/workflows/linux-build-base.yml is missing" fi # Search the entire repo for any occurrences echo -e "\nSearching for any linux-build-base.yml matches in the repo:" fd linux-build-base.yml || echo "No matches found"Length of output: 461
Refine workflow triggers and remove stale path reference
Please confirm that your CI triggers cover all intended branches (e.g., addmainif required) and remove the deletedlinux-build-base.ymlentry from thepathsfilter since that file no longer exists.• .github/workflows/linux-build.yml: delete the
".github/workflows/linux-build-base.yml"line underpathson: push: branches: [presto-0.293-clp-connector] paths: - - ".github/workflows/linux-build-base.yml"
♻️ Duplicate comments (2)
.github/workflows/linux-build.yml (2)
52-54: 🧹 Nitpick (assertive)Simplify USE_CLANG assignment
No need for a string expression—use a boolean directly:-env: - USE_CLANG: "${{ 'false' }}" +env: + USE_CLANG: false
64-72: 🧹 Nitpick (assertive)Remove duplicate .NET cleanup and optimise Docker prune
Lines 65–66 both remove/usr/share/dotnet. Drop one to eliminate redundancy. Also consider limitingdocker pruneto dangling images (e.g.,docker image prune --force) or leveraging caching to speed up CI.Example diff:
- rm -rf /usr/share/dotnet - rm -rf /usr/share/dotnet + rm -rf /usr/share/dotnet
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/linux-build.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build.yml
51-51: label "yscope-gh-runner" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🪛 YAMLlint (1.37.1)
.github/workflows/linux-build.yml
[error] 93-93: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build with GCC
🔇 Additional comments (1)
.github/workflows/linux-build.yml (1)
45-47: Concurrency configuration looks solid
Using a unique group andcancel-in-progressto skip builds on the special branch is correct and the expression is unquoted (a native boolean).
.github/workflows/linux-build.yml
Outdated
| linux-gcc: | ||
| name: Build with GCC | ||
| uses: ./.github/workflows/linux-build-base.yml | ||
| runs-on: yscope-gh-runner |
There was a problem hiding this comment.
Validate custom runner label
yscope-gh-runner isn’t a standard GitHub-hosted label and may cause the job to never pick up. Ensure this self-hosted runner is correctly registered or switch to a supported label (e.g., self-hosted, linux).
🧰 Tools
🪛 actionlint (1.7.7)
51-51: label "yscope-gh-runner" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🤖 Prompt for AI Agents
In .github/workflows/linux-build.yml at line 51, the runner label
'yscope-gh-runner' is a custom label that might not be recognized by GitHub
Actions, potentially causing the job to not run. Verify that the self-hosted
runner with this label is properly registered and online; if not, replace
'yscope-gh-runner' with a standard GitHub-hosted runner label like
'ubuntu-latest' or 'self-hosted' to ensure the workflow runs correctly.
.github/workflows/linux-build.yml
Outdated
| - name: Build Docs | ||
| run: | | ||
| source .venv/bin/activate | ||
| cd velox/docs | ||
| make clean | ||
| make html |
There was a problem hiding this comment.
Fix working directory in Build Docs step
With defaults.working-directory: velox, cd velox/docs would end up in velox/velox/docs and fail. You can either remove the extra path component or override the step’s working-directory. For example:
- - name: Build Docs
- run: |
- source .venv/bin/activate
- cd velox/docs
+ - name: Build Docs
+ run: |
+ source .venv/bin/activate
+ cd docs📝 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.
| - name: Build Docs | |
| run: | | |
| source .venv/bin/activate | |
| cd velox/docs | |
| make clean | |
| make html | |
| - name: Build Docs | |
| run: | | |
| source .venv/bin/activate | |
| cd docs | |
| make clean | |
| make html |
🤖 Prompt for AI Agents
In .github/workflows/linux-build.yml around lines 94 to 99, the Build Docs step
incorrectly changes directory to velox/docs while the default working directory
is already set to velox, causing a path error. Fix this by removing the cd
velox/docs command or by overriding the step's working-directory to velox/docs
so the commands run in the correct directory.
There was a problem hiding this comment.
As mentioned in one of the comments, we should probably keep testing build with clang. Overall, I would suggest:
- Instead of changing
linux-build.ymlto do whatlinux-build-base.ymlwas doing, just modifylinux-build-base.ymland keep the clang input so that we can still call it fromscheduled.yml - Instead of inlining the docs steps into
linux-build-base.yml, keep doing them indocs.yml, but remove the unnecessary steps.
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
There was a problem hiding this comment.
Let's undo unnecessary changes like this one and others.
.github/workflows/linux-build.yml
Outdated
| push: | ||
| branches: | ||
| - "main" | ||
| branches: [presto-0.293-clp-connector] |
There was a problem hiding this comment.
| branches: [presto-0.293-clp-connector] | |
| branches: | |
| - "presto-0.293-clp-connector" |
To reduce the size of the diff.
.github/workflows/linux-build.yml
Outdated
| runs-on: yscope-gh-runner | ||
| env: | ||
| CCACHE_DIR: "${{ github.workspace }}/ccache" | ||
| USE_CLANG: "${{ 'false' }}" |
There was a problem hiding this comment.
Hm, it's probably important to still try and build with clang otherwise we might end up with a lot of compilation errors when we merge into the official Velox repo. In the existing code (before this PR), there's a scheduled workflow to build with clang (bottom of scheduled.yml. We should probably keep that.
.github/workflows/linux-build.yml
Outdated
| - name: Node Setup and Cleanup | ||
| working-directory: ${{ github.workspace }} | ||
| run: | | ||
| df -h | ||
| rm -rf /usr/local/lib/android | ||
| rm -rf /usr/share/dotnet | ||
| rm -rf /usr/share/dotnet | ||
| rm -rf /opt/ghc | ||
| rm -rf /opt/hostedtoolcache/CodeQL | ||
| rm -rf "/usr/local/share/boost" | ||
| rm -rf "$AGENT_TOOLSDIRECTORY" | ||
| docker image prune --all --force | ||
| docker builder prune -a |
There was a problem hiding this comment.
Where's this from and do we still need it now that we've reduced the size of the build?
.github/workflows/linux-build.yml
Outdated
| - name: Fetch the origin to get presto-0.293-clp-connector branch | ||
| run: | | ||
| git fetch |
There was a problem hiding this comment.
This shouldn't be necessary. What error do you get if you don't do this?
There was a problem hiding this comment.
When running the make header-fix and make format-fix on presto-0.293-clp-connector branch, it has to run git fetch first to get the branch's infomation.
But now since we removed the make header-fix and make format-fix, we can remove it.
Makefile
Outdated
| PIP ?= $(shell command -v ~/.local/bin/uv > /dev/null 2>&1 && echo "~/.local/bin/uv pip" || echo "python3 -m pip") | ||
| VENV ?= $(shell command -v ~/.local/bin/uv > /dev/null 2>&1 && echo "~/.local/bin/uv venv" || echo "python3 -m venv") |
There was a problem hiding this comment.
Isn't uv on the runner's path?
There was a problem hiding this comment.
Same as above. I tried but the PATH does not work even I print the content of .bashrc where it has been added by pipx.
Makefile
Outdated
| --minloglevel=0 | ||
|
|
||
| format-fix: #: Fix formatting issues in the main branch | ||
| format-fix: #: Fix formatting issues in the presto-0.293-clp-connector branch |
There was a problem hiding this comment.
| format-fix: #: Fix formatting issues in the presto-0.293-clp-connector branch | |
| format-fix: #: Fix formatting issues |
| format-fix: #: Fix formatting issues in the presto-0.293-clp-connector branch | ||
| ifneq ("$(wildcard ${PYTHON_VENV}/pyvenv.cfg)","") | ||
| source ${PYTHON_VENV}/bin/activate; scripts/check.py format main --fix | ||
| source ${PYTHON_VENV}/bin/activate; scripts/check.py format presto-0.293-clp-connector --fix |
There was a problem hiding this comment.
Instead of repeating the branch name everywhere, couldn't we use a variable instead? If we're going to remove this Makefile soon, then I guess you can ignore this suggestion.
There was a problem hiding this comment.
Yea, we will not use check.py anymore so we won't need to hardcode branch here.
There was a problem hiding this comment.
But to be clear, the Makefile is not removed, but just some tasks will be removed.
There was a problem hiding this comment.
Makefile
Outdated
| endif | ||
|
|
||
| format-check: #: Check for formatting issues on the main branch | ||
| format-check: #: Check for formatting issues on the presto-0.293-clp-connector branch |
There was a problem hiding this comment.
| format-check: #: Check for formatting issues on the presto-0.293-clp-connector branch | |
| format-check: #: Check for formatting issues |
| scripts/check.py format presto-0.293-clp-connector | ||
| endif | ||
|
|
||
| header-fix: #: Fix license header issues in the current branch |
There was a problem hiding this comment.
| header-fix: #: Fix license header issues in the current branch | |
| header-fix: #: Fix license header issues |
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
.github/workflows/linux-build.yml (1)
51-51:⚠️ Potential issueFix YAML expression spacing in concurrency
Add spaces inside the expression to satisfy YAML parsing:-cancel-in-progress: ${{github.ref != 'refs/heads/presto-0.293-clp-connector'}} +cancel-in-progress: ${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/docs.yml(2 hunks).github/workflows/linux-build-base.yml(2 hunks).github/workflows/linux-build.yml(2 hunks).github/workflows/scheduled.yml(3 hunks)Makefile(3 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/docs.yml
39-39: label "8-core-ubuntu" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
.github/workflows/linux-build-base.yml
52-52: could not parse as YAML: yaml: line 52: could not find expected ':'
(syntax-check)
🪛 YAMLlint (1.37.1)
.github/workflows/linux-build-base.yml
[error] 53-53: syntax error: could not find expected ':'
(syntax)
🔇 Additional comments (8)
.github/workflows/linux-build.yml (1)
19-19: Ensure correct branch filter for triggers
The push event now only fires onpresto-0.293-clp-connector. Confirm this aligns with your intended CI flow and that no other branches should be included..github/workflows/linux-build-base.yml (3)
29-29: Verify self-hosted runner label
runs-on: yscope-gh-runneris a custom label. Ensure this runner is registered and online, or switch to a standard label likeubuntu-latest.
38-41: Ensure checkout path matches workspace layout
You’re usingpath: velox—confirm all subsequent steps (and thedefaults.run.working-directory: velox) correctly reference this subdirectory.
48-49: Confirm install script exists
You runscripts/setup-ubuntu.sh && install_apt_deps. Double-check thatinstall_apt_depsis defined within that script and accessible at this path..github/workflows/docs.yml (3)
19-19: Restrict docs push trigger to feature branch
Limiting pushes topresto-0.293-clp-connectoris intentional; confirm it aligns with your publishing strategy.
30-30: Downgrade permissions to read-only
Changingcontentsfromwritetoreadmatches the removal of push and upload steps—LGTM.
39-39: Validate runner label
runs-on: 8-core-ubuntuisn’t a standard GitHub runner. Verify this custom label exists or use a supported one (e.g.,ubuntu-latest-8-cores).🧰 Tools
🪛 actionlint (1.7.7)
39-39: label "8-core-ubuntu" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
.github/workflows/scheduled.yml (1)
34-34: Confirm branch filter for triggers
Push and PR events now only fire onpresto-0.293-clp-connector; ensure this matches your CI requirements.
| format-fix: #: Fix formatting issues | ||
| ifneq ("$(wildcard ${PYTHON_VENV}/pyvenv.cfg)","") | ||
| source ${PYTHON_VENV}/bin/activate; scripts/check.py format main --fix | ||
| source ${PYTHON_VENV}/bin/activate; scripts/check.py format presto-0.293-clp-connector --fix | ||
| else | ||
| scripts/check.py format main --fix | ||
| scripts/check.py format presto-0.293-clp-connector --fix |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Parameterise branch argument for format-fix
Hardcoding presto-0.293-clp-connector in multiple targets is error-prone. Extract it into a Makefile variable (e.g. CHECK_BRANCH) to DRY up these invocations.
🤖 Prompt for AI Agents
In the Makefile around lines 188 to 192, the branch name
`presto-0.293-clp-connector` is hardcoded multiple times in the `format-fix`
target. Define a variable like `CHECK_BRANCH` at the top of the Makefile or near
these lines and replace all instances of the hardcoded branch name with this
variable to avoid repetition and make future updates easier.
| header-check: #: Check for license header issues on the presto-0.293-clp-connector branch | ||
| ifneq ("$(wildcard ${PYTHON_VENV}/pyvenv.cfg)","") | ||
| source ${PYTHON_VENV}/bin/activate; scripts/check.py header main | ||
| source ${PYTHON_VENV}/bin/activate; scripts/check.py header presto-0.293-clp-connector | ||
| else | ||
| scripts/check.py header main | ||
| scripts/check.py header presto-0.293-clp-connector |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Align header-check comment with behaviour
Same discrepancy here—consider generalising the comment or parameterising the branch name to avoid duplication.
🤖 Prompt for AI Agents
In the Makefile around lines 210 to 214, the comment for the `header-check`
target specifically mentions checking the license header on the
`presto-0.293-clp-connector` branch, which is too specific and does not reflect
the general behavior of the script. Update the comment to be more general or
modify the script to accept a parameter for the branch name, then update the
comment accordingly to avoid duplication and better describe the target's
functionality.
| format-check: #: Check for formatting issues | ||
| clang-format --version | ||
| ifneq ("$(wildcard ${PYTHON_VENV}/pyvenv.cfg)","") | ||
| source ${PYTHON_VENV}/bin/activate; scripts/check.py format main | ||
| source ${PYTHON_VENV}/bin/activate; scripts/check.py format presto-0.293-clp-connector | ||
| else | ||
| scripts/check.py format main | ||
| scripts/check.py format presto-0.293-clp-connector |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Parameterise branch argument for format-check
Apply the same DRY principle here; reference a shared CHECK_BRANCH variable instead of repeating the literal.
🤖 Prompt for AI Agents
In the Makefile around lines 195 to 200, the branch argument
"presto-0.293-clp-connector" is hardcoded in the format-check target. Define a
variable CHECK_BRANCH with this branch name at the top of the Makefile or near
this section, then replace all occurrences of the literal branch name in the
format-check target with the variable $(CHECK_BRANCH) to avoid repetition and
improve maintainability.
| header-fix: #: Fix license header issues in the current branch | ||
| ifneq ("$(wildcard ${PYTHON_VENV}/pyvenv.cfg)","") | ||
| source ${PYTHON_VENV}/bin/activate; scripts/check.py header main --fix | ||
| source ${PYTHON_VENV}/bin/activate; scripts/check.py header presto-0.293-clp-connector --fix | ||
| else | ||
| scripts/check.py header main --fix | ||
| scripts/check.py header presto-0.293-clp-connector --fix |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Align header-fix comment with behaviour
The comment mentions “current branch” but the script targets a specific branch. Either update the comment to reference presto-0.293-clp-connector or use a shared variable for consistency.
🤖 Prompt for AI Agents
In the Makefile around lines 203 to 207, the comment for the header-fix target
incorrectly states it fixes license header issues in the "current branch" while
the script explicitly targets the "presto-0.293-clp-connector" branch. Update
the comment to mention "presto-0.293-clp-connector" instead of "current branch"
or define a variable for the branch name and use it both in the comment and the
script command for consistency.
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.repository }}-${{ github.head_ref || github.sha }} | ||
| cancel-in-progress: true | ||
| cancel-in-progress: ${{github.ref != 'refs/heads/presto-0.293-clp-connector'}} |
There was a problem hiding this comment.
Fix YAML expression spacing in concurrency
Add spaces inside the expression to avoid parse errors:
-cancel-in-progress: ${{github.ref != 'refs/heads/presto-0.293-clp-connector'}}
+cancel-in-progress: ${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }}📝 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.
| cancel-in-progress: ${{github.ref != 'refs/heads/presto-0.293-clp-connector'}} | |
| cancel-in-progress: ${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }} |
🤖 Prompt for AI Agents
In .github/workflows/docs.yml at line 34, the YAML expression for
cancel-in-progress lacks spaces inside the curly braces, which can cause parse
errors. Add a space after the opening ${{ and before the closing }} to properly
format the expression and ensure it parses correctly.
.github/workflows/scheduled.yml
Outdated
| # schedule: | ||
| # - cron: '0 3 * * *' |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove commented schedule block
The cron block is commented out—consider deleting these lines to clean up the workflow.
🤖 Prompt for AI Agents
In .github/workflows/scheduled.yml at lines 47 to 48, remove the commented-out
schedule block containing the cron expression to clean up the workflow file and
avoid confusion.
| group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event_name == 'pull_request' && github.head_ref || github.sha }} | ||
| cancel-in-progress: true | ||
|
|
||
| env: | ||
| # Run for 15 minute on PRs | ||
| DURATION: "${{ inputs.duration || ( github.event_name != 'schedule' && 900 || 1800 )}}" | ||
| # minimize artifact duration for PRs, keep them a bit longer for nightly runs | ||
| RETENTION: "${{ github.event_name == 'pull_request' && 1 || 3 }}" | ||
| cancel-in-progress: ${{github.ref != 'refs/heads/presto-0.293-clp-connector'}} | ||
|
|
There was a problem hiding this comment.
Fix YAML expression spacing in concurrency
Add spaces within the expression to prevent parsing issues:
-cancel-in-progress: ${{github.ref != 'refs/heads/presto-0.293-clp-connector'}}
+cancel-in-progress: ${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }}📝 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.
| group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event_name == 'pull_request' && github.head_ref || github.sha }} | |
| cancel-in-progress: true | |
| env: | |
| # Run for 15 minute on PRs | |
| DURATION: "${{ inputs.duration || ( github.event_name != 'schedule' && 900 || 1800 )}}" | |
| # minimize artifact duration for PRs, keep them a bit longer for nightly runs | |
| RETENTION: "${{ github.event_name == 'pull_request' && 1 || 3 }}" | |
| cancel-in-progress: ${{github.ref != 'refs/heads/presto-0.293-clp-connector'}} | |
| group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event_name == 'pull_request' && github.head_ref || github.sha }} | |
| cancel-in-progress: ${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }} |
🤖 Prompt for AI Agents
In .github/workflows/scheduled.yml around lines 57 to 59, the concurrency
expression lacks proper spacing inside the curly braces, which can cause YAML
parsing errors. Add spaces inside the expression delimiters, for example, change
${{github.ref != 'refs/heads/presto-0.293-clp-connector'}} to ${{ github.ref !=
'refs/heads/presto-0.293-clp-connector' }}, ensuring consistent spacing around
operators and variables within the expressions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/linux-build-base.yml (1)
59-60:⚠️ Potential issueCorrect test working-directory path.
Tests are executed from
_build/releaserelative to the repository root, but the checkout path places the build output undervelox/_build/release. Update the step to:- working-directory: _build/release + working-directory: velox/_build/release
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/linux-build-base.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build-base.yml
29-29: label "yscope-gh-runner" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🪛 YAMLlint (1.37.1)
.github/workflows/linux-build-base.yml
[error] 54-54: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build with Clang / Ubuntu release with resolve_dependency
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (2)
.github/workflows/linux-build-base.yml (2)
27-37: Streamlined ubuntu-release job configuration is clear.The new job effectively consolidates dependency installation and build steps on a dedicated self-hosted runner, reducing complexity compared to the previous adapters job.
🧰 Tools
🪛 actionlint (1.7.7)
29-29: label "yscope-gh-runner" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
29-29: Validate custom runner label in actionlint.The
runs-on: yscope-gh-runnerlabel is unknown to actionlint by default. Ensure this custom self-hosted runner label is whitelisted in your actionlint configuration or update it to match a valid runner label.🧰 Tools
🪛 actionlint (1.7.7)
29-29: label "yscope-gh-runner" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
.github/workflows/linux-build.yml (1)
54-59: Fix invalid job definitionThe
linux-gccjob is syntactically invalid: it lacksruns-onand astepssection, andrunis not recognised at the job level. Either re-enable the reusable workflow:- # uses: ./.github/workflows/linux-build-base.yml + uses: ./.github/workflows/linux-build-base.yml with: use-clang: falseor define the job inline:
- run: | - echo "Test" + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Placeholder + run: echo "Test"
♻️ Duplicate comments (3)
.github/workflows/docs.yml (3)
56-56: Prefer$HOMEover~in paths
Explicit~may not expand consistently. Use$HOME/.local/bin/uvor adjustPATHinstead.
34-34: Fix YAML expression spacing
Missing spaces inside thecancel-in-progressexpression can cause parse errors. Add spaces after{{and before}}:-cancel-in-progress: ${{github.ref != 'refs/heads/presto-0.293-clp-connector'}} +cancel-in-progress: ${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }}
51-51: Use correct package manager for CentOS
This container is CentOS-based, soaptwill fail. Replace withsudo dnf install -y pipx(oryum):-sudo apt install -y pipx +sudo dnf install -y pipx
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/docs.yml(2 hunks).github/workflows/linux-build-base.yml(2 hunks).github/workflows/linux-build.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build.yml
54-54: "steps" section is missing in job "linux-gcc"
(syntax-check)
54-54: "runs-on" section is missing in job "linux-gcc"
(syntax-check)
57-57: unexpected key "run" for "job" section. expected one of "concurrency", "container", "continue-on-error", "defaults", "env", "environment", "if", "name", "needs", "outputs", "permissions", "runs-on", "secrets", "services", "steps", "strategy", "timeout-minutes", "uses", "with"
(syntax-check)
.github/workflows/docs.yml
39-39: label "8-core-ubuntu" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
.github/workflows/linux-build-base.yml
29-29: label "yscope-gh-runner" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🪛 YAMLlint (1.37.1)
.github/workflows/linux-build.yml
[error] 57-57: too many spaces after colon
(colons)
.github/workflows/linux-build-base.yml
[error] 54-54: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build with Clang / Ubuntu release with resolve_dependency
🔇 Additional comments (7)
.github/workflows/docs.yml (4)
15-15: Approve renamed workflow title
Changing the name from “Update Documentation” to “Build Documentation” accurately reflects the reduced scope.
19-19: Approve restricted push trigger
Limiting pushes to thepresto-0.293-clp-connectorbranch aligns with the PR’s branch-focused CI strategy.
30-30: Approve reduced permissions
Downgradingcontentsto read-only enforces least-privilege for this workflow.
38-38: Approve job renaming
Renaming the job to “Build” removes unnecessary push steps and matches its sole responsibility..github/workflows/linux-build-base.yml (1)
38-40: Checkout path and working directory alignmentUsing
actions/checkout@v4withpath: veloxand settingdefaults.working-directory: veloxcorrectly clones into and operates within the project directory..github/workflows/linux-build.yml (2)
19-19: Verify branch filter intentThe
pushtrigger is now restricted topresto-0.293-clp-connector. Ensure this branch-only filter is intentional and that builds on other branches (e.g.,main) are not required for your CI strategy.
51-51: Removed quotedcancel-in-progressThe
cancel-in-progressfield now correctly uses a boolean expression without surrounding quotes. This aligns with YAML expectations and ensures proper cancellation behaviour.
| export CC=/usr/bin/clang-15 | ||
| export CXX=/usr/bin/clang++-15 | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider explicit GCC fallback
Currently, the compiler is only set when USE_CLANG=true. For reproducible builds, you may add an else clause to set CC=/usr/bin/gcc and CXX=/usr/bin/g++ when USE_CLANG=false.
🤖 Prompt for AI Agents
In .github/workflows/linux-build-base.yml at lines 55 to 57, the compiler
environment variables CC and CXX are set only when USE_CLANG=true. To ensure
reproducible builds, add an else clause after the current if block to explicitly
set CC to /usr/bin/gcc and CXX to /usr/bin/g++ when USE_CLANG is false.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/docs.yml (2)
34-34: Add spacing in YAML expressions
YAML actions expressions benefit from spaces inside{{ }}for readability and to avoid parse issues.-cancel-in-progress: ${{github.ref != 'refs/heads/presto-0.293-clp-connector'}} +cancel-in-progress: ${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }}
55-55: Use$HOMEinstead of~for path expansion
Explicit$HOMEis more reliable in CI shells than~.- ~/.local/bin/uv pip install -r scripts/docs-requirements.txt + $HOME/.local/bin/uv pip install -r scripts/docs-requirements.txt
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/docs.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/docs.yml
39-39: label "yscope-gh-runner" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🔇 Additional comments (5)
.github/workflows/docs.yml (5)
15-15: Rename workflow to "Build Documentation"
The updated name succinctly describes the action being performed.
19-19: Restrict push triggers to connector branch
Limiting pushes topresto-0.293-clp-connectoris intentional for branch-centric CI; ensure this aligns with your release workflow.
30-30: Apply least-privilege permissions
Downgradingcontentstoreadfollows the principle of least privilege—good catch.
38-38: Approve job rename to "Build"
Renaming the job toBuildaccurately reflects its reduced scope.
50-51: Confirm package manager on self-hosted runner
Usingaptassumes the runner OS supports it. Please verify thatyscope-gh-runneris Debian/Ubuntu-based; otherwise switch to the appropriate package manager (e.g.,dnforyum).
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
velox/functions/lib/QuantileDigest.h (1)
536-543: Enforce threshold correctness at compile time
To ensurekMinDoubleAboveInt64Maxtruly exceedsstd::numeric_limits<int64_t>::max()and reliably prevents overflow, add astatic_assert.Proposed diff:
+#include <limits> // for std::numeric_limits + static_assert( kMinDoubleAboveInt64Max > static_cast<double>(std::numeric_limits<int64_t>::max()), "kMinDoubleAboveInt64Max must exceed int64_t max");
♻️ Duplicate comments (2)
.github/workflows/docs.yml (2)
57-57: Use$HOMEinstead of~for the pipx wrapper path.The
~may not expand correctly in CI; prefer$HOME/.local/bin/uv.- ~/.local/bin/uv pip install -r scripts/docs-requirements.txt + $HOME/.local/bin/uv pip install -r scripts/docs-requirements.txt
34-34: Fix YAML expression spacing to avoid parse errors.Add spaces inside
${{ }}for proper parsing.
Proposed diff:-cancel-in-progress: ${{github.ref != 'refs/heads/presto-0.293-clp-connector'}} +cancel-in-progress: ${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/docs.yml(2 hunks)velox/functions/lib/QuantileDigest.h(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/docs.yml
39-39: label "yscope-gh-runner" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build with Clang / Ubuntu release with resolve_dependency
🔇 Additional comments (9)
velox/functions/lib/QuantileDigest.h (1)
21-23: Add missing dependency for threshold constant
Including"velox/common/base/Doubles.h"is necessary to bring in the definition ofkMinDoubleAboveInt64Max..github/workflows/docs.yml (8)
15-15: Approve the workflow name update.This rename clarifies the intention of the job.
19-19: Restrict triggers to connector branch.Filtering push events to the
presto-0.293-clp-connectorbranch aligns with the PR objectives.
30-30: Minimal permissions principle applied.Limiting
contentsaccess toreadfollows least-privilege best practices.
38-38: Approve job name simplification.The concise
Buildname remains clear and consistent with other workflows.
42-45: Defaults block correctly sets shell and working directory.Specifying
bashandveloxas the working directory streamlines subsequent commands.
47-47: Checkout step with custom path.Using
path: veloxclones the repository intovelox/, matching theworking-directory.
49-49: Ensure checkout path aligns with commands.The
veloxpath is consistent with later steps that operate insidevelox/docs.
52-53: Confirm package manager compatibility.This uses
aptto installpipx, but the custom runner OS is unspecified.
Please verify the runner’s OS and switch to the appropriate package manager (e.g.dnforyumon CentOS).
.github/workflows/docs.yml
Outdated
|
|
||
| on: | ||
| push: | ||
| branches: [presto-0.293-clp-connector] |
There was a problem hiding this comment.
| branches: [presto-0.293-clp-connector] |
I think this workflow is fast enough that we can run it on every push.
There was a problem hiding this comment.
But I find that it will trigger two docs build workflow for a commit pushed to the branch has a PR:
https://github.com/y-scope/velox/actions/runs/15694580333
https://github.com/y-scope/velox/actions/runs/15694580024
The run IDs are different so they should different runs.
There was a problem hiding this comment.
Added a if statement to avoid this.
.github/workflows/docs.yml
Outdated
| runs-on: 8-core-ubuntu | ||
| container: ghcr.io/facebookincubator/velox-dev:centos9 | ||
| name: Build | ||
| runs-on: yscope-gh-runner |
There was a problem hiding this comment.
| runs-on: yscope-gh-runner | |
| runs-on: ubuntu-latest |
This workflow should be able to run on a GH runner.
.github/workflows/docs.yml
Outdated
| defaults: | ||
| run: | ||
| shell: bash | ||
| working-directory: velox |
| - name: Checkout | ||
| uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 |
There was a problem hiding this comment.
Let's continue using this. actions/checkout@v4 has some security risks if that repo gets compromised.
| fetch-depth: 0 | ||
| persist-credentials: true |
There was a problem hiding this comment.
Can we revert to this? Or do you have a good reason for the changes?
| dnf install -y --setopt=install_weak_deps=False pandoc | ||
| pip3 install pipx | ||
| sudo apt install -y pipx | ||
| pipx install --force uv cmake==3.31 |
There was a problem hiding this comment.
| pipx install --force uv cmake==3.31 | |
| pipx install --force uv cmake==3.31 | |
There was a problem hiding this comment.
Also added a new line after each job in linux-build-base.yml
| which uv | ||
| make python-venv | ||
| which uv |
| which uv | ||
| make python-venv | ||
| which uv | ||
| uv pip install -r scripts/docs-requirements.txt |
.github/workflows/docs.yml
Outdated
| path: "${{ env.CCACHE_DIR }}" | ||
| key: ccache-wheels-8-core-ubuntu | ||
|
|
||
| ~/.local/bin/uv pip install -r scripts/docs-requirements.txt |
There was a problem hiding this comment.
| ~/.local/bin/uv pip install -r scripts/docs-requirements.txt | |
| ~/.local/bin/uv pip install -r scripts/docs-requirements.txt | |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
.github/workflows/docs.yml (1)
33-33: Add spaces inside YAML expression braces
Improve readability and avoid potential parsing issues:-cancel-in-progress: ${{github.ref != 'refs/heads/presto-0.293-clp-connector'}} +cancel-in-progress: ${{ github.ref != 'refs/heads/presto-0.293-clp-connector' }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/docs.yml(2 hunks).github/workflows/linux-build-base.yml(2 hunks)Makefile(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/linux-build-base.yml
29-29: label "yscope-gh-runner" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🪛 YAMLlint (1.37.1)
.github/workflows/linux-build-base.yml
[error] 56-56: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build with GCC / Ubuntu release with resolve_dependency
🔇 Additional comments (9)
Makefile (1)
195-201: Generic update toformat-checkcomment looks good
The new comment accurately describes checking formatting issues without binding to a specific branch..github/workflows/docs.yml (4)
15-15: Rename workflow to “Build Documentation”
This label better reflects that the job only builds docs (no push or publish).
29-29: Least‐privilege permissions are correct
Downgradingcontentstoreadaligns with the fact that this workflow no longer writes back to the repo.
37-38: Simplify job naming and runner choice
Switching toubuntu-latestand renaming the job to “Build” streamlines the workflow.
48-49: System dependency installation is appropriate forubuntu-latest
Usingaptto installpipxon Ubuntu is correct and aligns with the updated runner..github/workflows/linux-build-base.yml (4)
27-28: Job key and name update is clear
Renaming the job toubuntu-releasewith a descriptive label improves readability.
40-40: Checkout intoveloxdirectory is valid
Usingpath: veloxkeeps the code in its own subdirectory and matches the subsequentworking-directoryusage.
50-54:MAKEFLAGSusage is acceptable
SettingMAKEFLAGSto passMAX_HIGH_MEM_JOBSandMAX_LINK_JOBSinto the build is consistent with custom Makefile conventions.
63-65: Working directory and test invocation are correct
Running tests invelox/_build/releasewithctest -j $(getconf _NPROCESSORS_ONLN)matches the expected build layout and uses all available CPUs.
| header-fix: #: Fix license header issues in the current branch | ||
| ifneq ("$(wildcard ${PYTHON_VENV}/pyvenv.cfg)","") | ||
| source ${PYTHON_VENV}/bin/activate; scripts/check.py header main --fix | ||
| source ${PYTHON_VENV}/bin/activate; scripts/check.py header presto-0.293-clp-connector --fix | ||
| else | ||
| scripts/check.py header main --fix | ||
| scripts/check.py header presto-0.293-clp-connector --fix | ||
| endif |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Align header-fix comment with actual scope
This target still refers to the “current branch” while invoking a script against a named branch. Either make the comment reflect the working tree (e.g. “Fix license header issues in the current working tree”) or reference the branch variable introduced above.
🤖 Prompt for AI Agents
In the Makefile around lines 203 to 208, the comment for the header-fix target
incorrectly states it fixes license header issues in the "current branch" while
the script is run against a specific named branch. Update the comment to
accurately reflect the scope by changing it to "Fix license header issues in the
current working tree" or modify it to reference the branch variable used in the
script invocation.
| format-fix: #: Fix formatting issues | ||
| ifneq ("$(wildcard ${PYTHON_VENV}/pyvenv.cfg)","") | ||
| source ${PYTHON_VENV}/bin/activate; scripts/check.py format main --fix | ||
| source ${PYTHON_VENV}/bin/activate; scripts/check.py format presto-0.293-clp-connector --fix | ||
| else | ||
| scripts/check.py format main --fix | ||
| scripts/check.py format presto-0.293-clp-connector --fix | ||
| endif |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Introduce a branch variable to DRY up Makefile
Hardcoding presto-0.293-clp-connector in these commands invites errors if the branch name ever changes. Define at the top, for example:
CHECK_BRANCH := presto-0.293-clp-connectorand replace all literal uses here with $(CHECK_BRANCH).
🤖 Prompt for AI Agents
In the Makefile around lines 188 to 193, the branch name
`presto-0.293-clp-connector` is hardcoded multiple times. To avoid errors if the
branch name changes, define a variable at the top of the Makefile like
`CHECK_BRANCH := presto-0.293-clp-connector` and replace all literal occurrences
of the branch name in this section with `$(CHECK_BRANCH)`.
| header-check: #: Check for license header issues on the presto-0.293-clp-connector branch | ||
| ifneq ("$(wildcard ${PYTHON_VENV}/pyvenv.cfg)","") | ||
| source ${PYTHON_VENV}/bin/activate; scripts/check.py header main | ||
| source ${PYTHON_VENV}/bin/activate; scripts/check.py header presto-0.293-clp-connector | ||
| else | ||
| scripts/check.py header main | ||
| scripts/check.py header presto-0.293-clp-connector | ||
| endif |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify header-check description
Similarly, consider genericising the comment to “Check license header issues in the current working tree” or include $(CHECK_BRANCH) in both the comment and command to stay consistent.
🤖 Prompt for AI Agents
In the Makefile around lines 210 to 215, the comment for the header-check target
is specific to the presto-0.293-clp-connector branch, which reduces clarity and
flexibility. Update the comment to a more generic description like "Check
license header issues in the current working tree" or modify both the comment
and the command to use the variable $(CHECK_BRANCH) instead of the hardcoded
branch name to maintain consistency and adaptability.
| with: | ||
| path: "${{ env.CCACHE_DIR }}" | ||
| key: ccache-wheels-8-core-ubuntu | ||
| uv pip install -r scripts/docs-requirements.txt |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure pipx shims are on the PATH before calling uv
pipx install drops binaries in ~/.local/bin, which might not be in the default PATH. Add before this step:
- name: Add pipx shims to PATH
run: echo "PATH=$HOME/.local/bin:$PATH" >> $GITHUB_ENV🤖 Prompt for AI Agents
In .github/workflows/docs.yml at line 56, before the step that runs `uv pip
install -r scripts/docs-requirements.txt`, add a step to ensure pipx shims are
on the PATH by appending `PATH=$HOME/.local/bin:$PATH` to the GITHUB_ENV file.
This guarantees that binaries installed by pipx in `~/.local/bin` are accessible
during the workflow execution.
| if [[ "${USE_CLANG}" = "true" ]]; then | ||
| export CC=/usr/bin/clang-15 | ||
| export CXX=/usr/bin/clang++-15 | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clean up shell conditional and consider fallback
- Remove the trailing space after
thento avoid YAML lint warnings. - Optionally add an
elseblock to explicitly setCC=/usr/bin/gccandCXX=/usr/bin/g++when not using Clang for more reproducible builds.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 56-56: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In .github/workflows/linux-build-base.yml at lines 56 to 59, remove the trailing
space after `then` in the shell conditional to prevent YAML lint warnings.
Additionally, add an `else` block to explicitly set `CC=/usr/bin/gcc` and
`CXX=/usr/bin/g++` when `USE_CLANG` is not true, ensuring consistent compiler
environment for reproducible builds.
| name: "Ubuntu debug with resolve_dependency" | ||
| ubuntu-release: | ||
| name: "Ubuntu release with resolve_dependency" | ||
| runs-on: yscope-gh-runner |
There was a problem hiding this comment.
Confirm runner label validity
The yscope-gh-runner label isn’t a standard GitHub-hosted runner. Ensure you have a self-hosted runner registered with this label or switch to a supported label (e.g., ubuntu-22.04 or self-hosted).
🧰 Tools
🪛 actionlint (1.7.7)
29-29: label "yscope-gh-runner" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🤖 Prompt for AI Agents
In .github/workflows/linux-build-base.yml at line 29, the runner label
'yscope-gh-runner' is not a standard GitHub-hosted runner label. Verify that you
have a self-hosted runner registered with this exact label; if not, replace
'yscope-gh-runner' with a valid runner label such as 'ubuntu-22.04' for a
GitHub-hosted runner or 'self-hosted' if you intend to use any self-hosted
runner.
…ger-overflow (facebookincubator#13831) Summary: Pull Request resolved: facebookincubator#13831 This avoids the following errors: ``` fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/std_abs.h:56:41: runtime error: negation of -9223372036854775808 cannot be represented in type 'long'; cast to an unsigned type to negate this value to itself #0 0x000000346ce5 in std::abs(long) fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/std_abs.h:56 #1 0x000000345879 in std::shared_ptr<facebook::velox::BiasVector<facebook::velox::test::EvalTypeHelper<long>::Type>> facebook::velox::test::VectorMaker::biasVector<long>(std::vector<std::optional<long>, std::allocator<std::optional<long>>> const&) fbcode/velox/vector/tests/utils/VectorMaker-inl.h:58 #2 0x000000344d34 in facebook::velox::test::BiasVectorErrorTest::errorTest(std::vector<std::optional<long>, std::allocator<std::optional<long>>>) fbcode/velox/vector/tests/BiasVectorTest.cpp:39 #3 0x00000033ec99 in facebook::velox::test::BiasVectorErrorTest_checkRangeTooLargeError_Test::TestBody() fbcode/velox/vector/tests/BiasVectorTest.cpp:44 #4 0x7fe0a2342c46 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) fbsource/src/gtest.cc:2727 #5 0x7fe0a234275d in testing::Test::Run() fbsource/src/gtest.cc:2744 #6 0x7fe0a2345fb3 in testing::TestInfo::Run() fbsource/src/gtest.cc:2890 #7 0x7fe0a234c8eb in testing::TestSuite::Run() fbsource/src/gtest.cc:3068 #8 0x7fe0a237b52b in testing::internal::UnitTestImpl::RunAllTests() fbsource/src/gtest.cc:6059 #9 0x7fe0a237a0a2 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) fbsource/src/gtest.cc:2727 #10 0x7fe0a23797f5 in testing::UnitTest::Run() fbsource/src/gtest.cc:5599 #11 0x7fe0a2239800 in RUN_ALL_TESTS() fbsource/gtest/gtest.h:2334 #12 0x7fe0a223952c in main fbcode/common/gtest/LightMain.cpp:20 #13 0x7fe09ec2c656 in __libc_start_call_main /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/csu/../sysdeps/nptl/libc_start_call_main.h:58:16 #14 0x7fe09ec2c717 in __libc_start_main@GLIBC_2.2.5 /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/csu/../csu/libc-start.c:409:3 #15 0x00000033d8b0 in _start /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/csu/../sysdeps/x86_64/start.S:116 UndefinedBehaviorSanitizer: signed-integer-overflow fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/std_abs.h:56:41 ``` Avoid overflow by using the expression (static_cast<uint64_t>(1) + ~static_cast<uint64_t>(min)) to calculate the absolute value of min without using std::abs Reviewed By: dmm-fb, peterenescu Differential Revision: D76901449 fbshipit-source-id: 7eb3bd0f83e42f44cdf34ea1759f3aa9e1042dae
Description
Simiar to this one: y-scope/presto#7
Removed some unnecessary workflows for now.
For header and format check, we will apply this commit, which also uses a new way to implement ccache, and we will try that in the next PR.
For clang-15 build, there is an error cause the CI failure. By
gcc --help=warnings | grep conversionthere is no-Wimplicit-const-int-float-conversion, but in clang, there is such a flag (added in commit). So the root cause is the way that velox configures the flags is first using-Wallthen manually disabling some specific flags, so this-Wimplicit-const-int-float-conversionis missed forclangdue to the difference of sets of warning flags. This is why it can be built with gcc but not with clang. Since we don't modify the code, this issue should also exist in main repo's 0.293, will try to submit a fix PR to the OSS as well.A few more things about the error mentioned above. After applying the fix similar to this commit, there is another place triggers the error (in this CI failure).
Checklist
breaking change.
Validation performed
Summary by CodeRabbit