Add datafusion cli for iceberg #1143
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has checked 285 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 241 | 5 | 39 | 0 |
Click to see the invalid file list
- crates/integrations/cli/Cargo.toml
- crates/integrations/cli/README.md
- crates/integrations/cli/src/catalog.rs
- crates/integrations/cli/src/lib.rs
- crates/integrations/cli/src/main.rs
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
There was a problem hiding this comment.
license-eye has checked 285 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 241 | 5 | 39 | 0 |
Click to see the invalid file list
- crates/integrations/cli/Cargo.toml
- crates/integrations/cli/README.md
- crates/integrations/cli/src/catalog.rs
- crates/integrations/cli/src/lib.rs
- crates/integrations/cli/src/main.rs
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
rust-toolchain.toml
Outdated
There was a problem hiding this comment.
Maybe we can exclude crates/integrations from workspace, and have separate rust-toolchain.toml for them
There was a problem hiding this comment.
What's the benefit of this?
There was a problem hiding this comment.
Because currently we already use different MSRV for the datafusion integration.
And in previous discussion:
We currently only have
msrvforiceberg. I'm not sure if our goal is to maintainmsrvfor all our integration crates, such asdatafusion.
Originally posted by @Xuanwo in #849 (comment)
You can see e.g., this PR is blocked by this. Actually we can upgrade the toolchain version used by the datafusion integration, without upgrading the project MSRV.
And e.g., for the cli project, since it depends on the datafusion integration, it needs to use max(project_msrv, datafusion_integration_msrv). That's why I proposed to just have a separate toolchain for all the integrations.
There was a problem hiding this comment.
But it seems our new MSRV policy (3 month) can cover datafusion's 4 month policy, so maybe there's no need to have separate MSRV any more.
However, if we only upgrade MSRV "on release", that may not be enough. (imagine datafusion releases more frequently, and we can't bump MSRV until next release.) Maybe we could just bump MSRV "on demand" in this case? We just need to guarantee at any time, the 3 month time range is covered?
There was a problem hiding this comment.
(imagine datafusion releases more frequently, and we can't bump MSRV until next release.)
Do we need to catch up with every latest datafusion release? In this case I would prefer to release iceberg-rust more often.
There was a problem hiding this comment.
I don't have strong opinions on this. It would be nice to have more frequent iceberg-rs releases.
But it seems currently we want to wait for some internal milestones to be finished before release. So if some user/developer wants to upgrade datafusion, they have to wait for it. This slowness seems a little unnecessary to me. 🤔
To summarize a bit,
- approach 1: use separate toolchain/msrv for integrations. (currently used)
- approach 2: use the same toolchain/msrv
- to do this, we might either need to bump msrv on demand (not only on release ), or release more frequently.
Maybe we could have a tracking issue and create subtasks for feature ideas? |
There was a problem hiding this comment.
license-eye has checked 291 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 245 | 2 | 44 | 0 |
Click to see the invalid file list
- crates/integrations/cli/Cargo.toml
- crates/integrations/cli/README.md
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
There was a problem hiding this comment.
license-eye has checked 291 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 245 | 2 | 44 | 0 |
Click to see the invalid file list
- crates/integrations/cli/Cargo.toml
- crates/integrations/cli/README.md
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
There was a problem hiding this comment.
license-eye has checked 291 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 245 | 2 | 44 | 0 |
Click to see the invalid file list
- crates/integrations/cli/Cargo.toml
- crates/integrations/cli/README.md
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
There was a problem hiding this comment.
license-eye has checked 291 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 245 | 2 | 44 | 0 |
Click to see the invalid file list
- crates/integrations/cli/Cargo.toml
- crates/integrations/cli/README.md
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
Do you mean to have a tracking issue for all feature ideas? |
There was a problem hiding this comment.
license-eye has checked 291 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 245 | 2 | 44 | 0 |
Click to see the invalid file list
- crates/integrations/cli/Cargo.toml
- crates/integrations/cli/README.md
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
There was a problem hiding this comment.
license-eye has checked 291 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 245 | 2 | 44 | 0 |
Click to see the invalid file list
- crates/integrations/cli/Cargo.toml
- crates/integrations/cli/README.md
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
|
Seems we have can't merge this since datafusion-cli requires newer version of rustc. We should postpone it after we release 0.5.0. |
Yes
I replied some thoughts on #1143 (comment) |
There was a problem hiding this comment.
license-eye has checked 292 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 246 | 2 | 44 | 0 |
Click to see the invalid file list
- crates/integrations/cli/Cargo.toml
- crates/integrations/cli/README.md
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
There was a problem hiding this comment.
license-eye has checked 292 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 246 | 2 | 44 | 0 |
Click to see the invalid file list
- crates/integrations/cli/Cargo.toml
- crates/integrations/cli/README.md
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
There was a problem hiding this comment.
license-eye has checked 292 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 246 | 2 | 44 | 0 |
Click to see the invalid file list
- crates/integrations/cli/Cargo.toml
- crates/integrations/cli/README.md
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
Signed-off-by: Ray Liu <[email protected]>
|
In favor #1193 |
Which issue does this PR close?
What changes are included in this PR?
Initial check in iceberg cli.
Are these changes tested?
Yes, ut.