Skip to content

build: Switch from y-scope/presto to prestodb/presto@6e1942 which has equivalent functionality. - #14

Merged
jackluo923 merged 2 commits into
mainfrom
build/adapt-plugin-to-upstream-pin
Jun 23, 2026
Merged

jackluo923 merged 2 commits into
mainfrom
build/adapt-plugin-to-upstream-pin

Conversation

@jackluo923

@jackluo923 jackluo923 commented Jun 22, 2026 •

Copy link
Copy Markdown
Member

Description

Repoint the velox-connector's CMake FetchContent of presto_native_execution from the y-scope/presto fork to prestodb/presto upstream, and lift the presto SHA into a single set(PRESTO_GIT_TAG ...) variable so a presto bump is a one-line change.

Also adds serialize/deserialize stubs for ConnectorDistributedProcedureHandle in ClpConnectorProtocol.h — the newer upstream ConnectorProtocol base class declares them as pure virtuals, so without these stubs the plugin won't compile against the new pin. Both stubs throw VELOX_NYI, consistent with the existing to_json/from_json stubs above them.

Breaking changes

None

Validation performed

  • Built the plugin against the new prestodb/presto pin and verified it compiles successfully.
  • Confirmed that PRESTO_GIT_TAG is referenced in a single place and that the FetchContent_Declare uses the variable.

Checklist

  • The PR satisfies the contribution guidelines.
  • Necessary docs have been updated, OR no docs need to be updated.
  • The description has been filled out.
  • The breaking changes section has been filled out.
  • The validation performed section has been filled out.
  • The PR title:
    • follows the Conventional Commits specification.
    • uses one of the commit types from here.
    • is in imperative form (e.g., "Add new node types.").
    • references the GitHub issues that the PR resolves (if any) using the (fixes #N) or (resolves #N) syntax in the title.

Summary by CodeRabbit

  • Chores
    • Updated the Presto connector build to pin headers using a configurable tag and switched the header source repository to the newer upstream.
    • Updated the build output to report the pinned tag value.
  • Refactor
    • Added inline placeholder behavior for distributed procedure handle serialization/deserialization that intentionally reports the feature as not implemented.

@jackluo923
jackluo923 requested a review from a team as a code owner June 22, 2026 22:14
@coderabbitai

coderabbitai Bot commented Jun 22, 2026 •

Copy link
Copy Markdown

Caution

Review failed

An error occurred during the review process. Please try again later.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch build/adapt-plugin-to-upstream-pin

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.

@jackluo923
jackluo923 force-pushed the build/adapt-plugin-to-upstream-pin branch from 71fcdea to 78fc40c Compare June 22, 2026 22:21
@kirkrodrigues

Copy link
Copy Markdown
Member

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 23, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Comment thread velox-connector/CMakeLists.txt Outdated
20001020ycx
20001020ycx previously approved these changes Jun 23, 2026

@20001020ycx 20001020ycx left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment for nit, the changes LGTM as I also have the same patch applied to the private repo.

…stream pin.

Repoints velox-connector's CMake FetchContent of presto_native_execution from the y-scope/presto fork to prestodb/presto and lifts the presto SHA into a single `set(PRESTO_GIT_TAG ...)` line so a presto bump is a one-line change. That same line is read by tools/build-packages/dependency-image/lib.sh to derive the velox SHA for the build-env image.

Adds serialize/deserialize stubs for ConnectorDistributedProcedureHandle in ClpConnectorProtocol.h — the newer upstream ConnectorProtocol base class declares them as pure virtuals, so without these the plugin wouldn't compile against the new pin. Both stubs throw VELOX_NYI consistent with the existing to_json/from_json stubs above them.
@jackluo923
jackluo923 force-pushed the build/adapt-plugin-to-upstream-pin branch from 78fc40c to 537c76b Compare June 23, 2026 13:51
Comment thread velox-connector/CMakeLists.txt Outdated

@kirkrodrigues kirkrodrigues left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the PR title, how about:

build: Switch from `y-scope/presto` to prestodb/presto@6e1942 which has equivalent functionality.

@jackluo923 jackluo923 changed the title build: Adapt plugin sources to compile against the prestodb/presto upstream pin. build: Switch from y-scope/presto to prestodb/presto@6e1942 which has equivalent functionality. Jun 23, 2026
@jackluo923
jackluo923 merged commit 1d3e89d into main Jun 23, 2026
5 checks passed
@jackluo923
jackluo923 deleted the build/adapt-plugin-to-upstream-pin branch June 23, 2026 14:49
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.

3 participants