Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(rust/drivers): adbc driver for datafusion #2267

Merged
merged 13 commits into from
Oct 27, 2024
Merged

feat(rust/drivers): adbc driver for datafusion #2267

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5bf89d7
feat(rust/driver/datafusion): adbc driver for datafusion
tokoko Oct 21, 2024
73d9c09
feat(rust/driver/datafusion): run pre-commit
tokoko Oct 21, 2024
c48dcee
feat(rust/driver/datafusion): remove unnecessary dependency on arrow
tokoko Oct 22, 2024
14a5cfc
feat(rust/driver/datafusion): install protoc, fix clippy errors
tokoko Oct 23, 2024
e01283c
feat(rust/driver/datafusion): remove arrow-cast dependency
tokoko Oct 23, 2024
52b14af
feat(rust/driver/datafusion): remove token from setup-protoc action
tokoko Oct 25, 2024
5a49b17
feat(rust/driver/datafusion): install system-provided protoc
tokoko Oct 25, 2024
aec5a96
feat(rust/driver/datafusion): install system-provided protoc
tokoko Oct 25, 2024
ef27afc
feat(rust/driver/datafusion): install protoc with curl
tokoko Oct 25, 2024
3609834
feat(rust/driver/datafusion): install protoc with curl
tokoko Oct 25, 2024
9e660a1
Apply suggestions from code review
tokoko Oct 27, 2024
3184f7c
feat(rust/driver/datafusion): remove O option from curl
tokoko Oct 27, 2024
e72420a
feat(rust/driver/datafusion): switch to universal_binary for osx
tokoko Oct 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/rust.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ jobs:
run: |
rustup toolchain install stable --no-self-update
rustup default stable
- name: Install Protoc
uses: arduino/setup-protoc@v3
with:
repo-token: ${{ secrets.GITHUB_TOKEN }}
Copy link
Member

Choose a reason for hiding this comment

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

...Why does installing protoc need the repo token?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, supposedly to query Github for the latest version...

Is there some other source for protoc that doesn't rely on a third party action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • We can just do curl+unzip but it will require some logic to choose the right binary in order to work cross-platform.
  • Another option is to install grpcio-tools from pypi and then alias protoc to python -m grpc_tools.protoc, but that obviously requires python to be installed.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose system protoc would be too old? (And Conda too heavy?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pixi should be fine, I think. I can add pixi.toml in rust directory, we can even install rust from there instead of using rustup iirc. Not sure if just protoc justifies pixi adoption, but it would be the simplest alternative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(1) substrait crate depends on protoc
(2) I guess, but if we still need to write some logic to install the system protoc (check whether to use apt or brew dependending on the os), we might as well download the binaries instead.

Copy link
Member

Choose a reason for hiding this comment

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

Or we can use the action minus the token? (Even if it's from Arduino, I'm a bit hesitant about passing even a read-only token to an external action.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, that works as well.. I will drop the token then, if we see that the step becomes flaky as a result, we can always make the changes afterwards.

Copy link
Contributor Author

@tokoko tokoko Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like this is a no-go 😆. judging from the action code, even if I pin an exact version, the action still tries to fetch all versions, presumably to validate.. seems like there's no way to make it work w/o a token.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lidavidm Finally got it working with pre-compiled binaries

- uses: actions/download-artifact@v4
with:
name: driver-manager-${{ matrix.os }}
Expand Down
Loading
Loading