Skip to content

Conversation

@anlowee
Copy link

@anlowee anlowee commented Oct 9, 2025

Description

This PR upgraded the velox submodule to https://github.com/y-scope/velox/tree/cf895960f83ece5621ba3dd40f68f322a4bab570 to apply the fixes in the following PRs:

  1. fix: support ZSTD-compressed IR and fix filteredLogEvents_ lifecycle. velox#36
  2. fix: Fix bugs in populating float values and in performing sanity check before loading the vector. velox#38

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

Passed the CI.

Summary by CodeRabbit

  • Chores
    • Updated an internal upstream dependency reference to the latest version. No user-facing functionality changed, and existing features and workflows remain unaffected. Performance, behaviour, and interfaces are unchanged. This aligns the project with upstream and prepares for future improvements. No action or downtime required for users.

@coderabbitai
Copy link

coderabbitai bot commented Oct 9, 2025

Walkthrough

Updated the presto-native-execution/velox submodule pointer from 1604a4d to cf89596. No in-repo source or configuration files were modified.

Changes

Cohort / File(s) Summary of Changes
Submodule pointer update
presto-native-execution/velox
Advanced submodule reference from 1604a4deb95924f2f5ddabf8bffbcd32a990f7d3 to cf895960f83ece5621ba3dd40f68f322a4bab570

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Possibly related PRs

Suggested reviewers

  • kirkrodrigues

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description deviates from the repository template by using incorrect heading levels and missing several required sections; it provides a Description section but omits Motivation and Context, Impact, and Test Plan headings, and does not include Release Notes. The contributor checklist is labelled under a different heading and the placeholder markdown comments remain at the top. Because critical template sections are missing or misformatted, the description does not meet the expected structure. Please update the pull request description to use the repository’s required “##” section headings for Description, Motivation and Context, Impact, Test Plan, Contributor checklist, and Release Notes, filling in each section with the relevant details about the change, testing, and impact. Include a release notes entry or use “== NO RELEASE NOTE ==” if none is required, and remove any leftover placeholder comments at the top of the description.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly describes the primary change by stating that the Velox submodule is being upgraded to a specific commit and uses the Conventional Commits “feat:” prefix in imperative form, making it concise and easily understood by teammates.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 733f4a2 and 25c34a4.

📒 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-main)
  • GitHub Check: test (17.0.13, :presto-tests -P ci-only-aggregation-queries)
  • GitHub Check: test (17.0.13, :presto-main-base)
  • GitHub Check: test (17.0.13, :presto-tests -P ci-only-resource-manager)
  • GitHub Check: test (17.0.13, :presto-tests -P presto-tests-general)
  • GitHub Check: test (17.0.13, :presto-tests -P ci-only-tpch-distributed-queries)
  • GitHub Check: test (17.0.13, :presto-tests -P ci-only-local-queries)
  • GitHub Check: test (17.0.13, :presto-tests -P ci-only-distributed-non-hash-gen)
  • GitHub Check: test (8.0.442, :presto-main)
  • 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-non-hash-gen)
  • 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 presto-tests-general)
  • GitHub Check: maven-checks (8.0.442)
  • GitHub Check: maven-checks (17.0.13)
  • GitHub Check: prestocpp-linux-build-for-test
  • GitHub Check: prestissimo-worker-images-build

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@anlowee anlowee requested a review from wraymo October 10, 2025 14:04
@anlowee anlowee merged commit bfa18ff into y-scope:release-0.293-clp-connector Oct 10, 2025
38 checks passed
@anlowee anlowee deleted the xwei/bump-velox-to-cf89596 branch October 10, 2025 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants