feat: allow for various arrow versions#1
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughIntroduces multi-version Arrow library support (versions 56, 57, 58) through conditional Cargo features and crate aliasing. Adds a new CI workflow for matrix testing across Arrow versions, restructures dependencies with version-specific aliases, and implements compile-time crate name aliasing to support multiple Arrow versions simultaneously. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Cargo.toml (1)
12-14:⚠️ Potential issue | 🟠 Majordocs.rs build will fail:
all-features = trueenables conflicting arrow versions.With the new feature split,
all-features = trueturns onarrow-56,arrow-57, andarrow-58simultaneously, causingsrc/arrow_aliases.rsto emit threeextern crate … as arrow_schema;items → E0259 at compile time. Pin docs.rs to a single backend instead.🐛 Proposed fix
[package.metadata.docs.rs] -all-features = true +features = ["convert", "arrow-58"] rustdoc-args = ["--cfg", "docsrs"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 12 - 14, The docs.rs config enabling all-features causes multiple conflicting arrow feature flags (arrow-56/57/58) and duplicate externs in src/arrow_aliases.rs; change the [package.metadata.docs.rs] stanza to avoid all-features=true and instead pin docs.rs to a single arrow backend (for example replace all-features = true with features = ["arrow-58"] or an equivalent single-arrow feature), keeping rustdoc-args as-is so docs build with a single arrow version.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
1-11: Optional: add aconcurrencygroup to cancel superseded runs.PR pushes can stack three matrix legs each; cancelling in-progress runs on a new push saves CI time without behavioural change.
♻️ Suggested addition
on: push: branches: [main] pull_request: + +concurrency: + group: ci-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 1 - 11, CI runs can pile up for matrix jobs like the check job (name: Check (arrow-${{ matrix.arrow }})), so add a top-level concurrency stanza to the workflow to cancel superseded runs; update the YAML to include concurrency with a stable group key that incorporates the workflow and matrix axis (e.g. use github.workflow and matrix.arrow) and set cancel-in-progress: true so new pushes cancel older in-progress matrix legs for the check job.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 60-66: The convert feature in Cargo.toml must declare a dependency
on the default arrow backend so building with --features convert brings in
arrow_schema/arrow_array/parquet; update the features block to set convert =
["arrow-58"] (or whatever the default arrow-XX feature is) while keeping the
empty arrow = [] marker intact; this fixes unresolved crates referenced
unconditionally in src/convert/arrow.rs, src/convert/mod.rs and
src/cli/generate.rs without adding new compile guards in src/arrow_aliases.rs.
In `@src/arrow_aliases.rs`:
- Around line 1-20: Add a compile-time guard in arrow_aliases.rs that emits
compile_error! when an invalid feature combination is detected: check that
exactly one of the features "arrow-56", "arrow-57", or "arrow-58" is enabled and
emit a clear compile_error! message if none or more than one are set; this
prevents duplicate extern crate aliases (arrow_schema, arrow_array, parquet) and
avoids unresolved symbols in modules that depend on them (e.g.,
src/convert/arrow.rs and src/cli/generate.rs expecting
arrow_schema/arrow_array/parquet); implement the guard before the current extern
crate blocks and refer to the feature flags ("arrow-56","arrow-57","arrow-58")
and the symbols arrow_schema, arrow_array, parquet in the error text.
---
Outside diff comments:
In `@Cargo.toml`:
- Around line 12-14: The docs.rs config enabling all-features causes multiple
conflicting arrow feature flags (arrow-56/57/58) and duplicate externs in
src/arrow_aliases.rs; change the [package.metadata.docs.rs] stanza to avoid
all-features=true and instead pin docs.rs to a single arrow backend (for example
replace all-features = true with features = ["arrow-58"] or an equivalent
single-arrow feature), keeping rustdoc-args as-is so docs build with a single
arrow version.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 1-11: CI runs can pile up for matrix jobs like the check job
(name: Check (arrow-${{ matrix.arrow }})), so add a top-level concurrency stanza
to the workflow to cancel superseded runs; update the YAML to include
concurrency with a stable group key that incorporates the workflow and matrix
axis (e.g. use github.workflow and matrix.arrow) and set cancel-in-progress:
true so new pushes cancel older in-progress matrix legs for the check job.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94a53812-d75f-4728-b5ed-9a615d414bfd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.github/workflows/ci.yml.github/workflows/main.ymlCargo.tomlsrc/arrow_aliases.rssrc/lib.rssrc/main.rs
| #[cfg(feature = "arrow-56")] | ||
| extern crate arrow_schema_56 as arrow_schema; | ||
| #[cfg(feature = "arrow-56")] | ||
| extern crate arrow_array_56 as arrow_array; | ||
| #[cfg(feature = "arrow-56")] | ||
| extern crate parquet_56 as parquet; | ||
|
|
||
| #[cfg(feature = "arrow-57")] | ||
| extern crate arrow_schema_57 as arrow_schema; | ||
| #[cfg(feature = "arrow-57")] | ||
| extern crate arrow_array_57 as arrow_array; | ||
| #[cfg(feature = "arrow-57")] | ||
| extern crate parquet_57 as parquet; | ||
|
|
||
| #[cfg(feature = "arrow-58")] | ||
| extern crate arrow_schema_58 as arrow_schema; | ||
| #[cfg(feature = "arrow-58")] | ||
| extern crate arrow_array_58 as arrow_array; | ||
| #[cfg(feature = "arrow-58")] | ||
| extern crate parquet_58 as parquet; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm `convert` module truly requires arrow_schema/parquet (otherwise the second guard is unnecessary).
rg -nP --type=rust '\b(arrow_schema|arrow_array|parquet)\b' src/convert src/cli
# Confirm there is no existing static guard already in the tree.
rg -nP 'compile_error!'Repository: aqora-io/rustcroissant
Length of output: 606
🏁 Script executed:
# Check Cargo.toml for feature definitions
grep -A 20 "\[features\]" Cargo.toml
# Check if convert module is conditionally compiled
rg -n "convert" Cargo.toml
# Verify the current structure of src/arrow_aliases.rs
head -30 src/arrow_aliases.rs
# Check if there's an "arrow" feature or if the second guard needs adjustment
rg -n 'feature = "arrow' Cargo.tomlRepository: aqora-io/rustcroissant
Length of output: 1121
Add a compile_error! guard to surface invalid feature combinations clearly.
Two failure modes currently produce cryptic errors:
- Enabling multiple
arrow-56/arrow-57/arrow-58produces E0259 duplicateextern crate … as arrow_schema;definitions. - Enabling
convertwithout anyarrow-XXleavesarrow_schema/arrow_array/parquetunresolved insrc/convert/arrow.rsandsrc/cli/generate.rs.
A static guard makes the contract explicit and the error actionable.
♻️ Suggested guard
+#[cfg(any(
+ all(feature = "arrow-56", feature = "arrow-57"),
+ all(feature = "arrow-56", feature = "arrow-58"),
+ all(feature = "arrow-57", feature = "arrow-58"),
+))]
+compile_error!(
+ "features `arrow-56`, `arrow-57`, and `arrow-58` are mutually exclusive; enable exactly one"
+);
+
+#[cfg(all(feature = "convert", not(feature = "arrow")))]
+compile_error!(
+ "feature `convert` requires one of `arrow-56`, `arrow-57`, or `arrow-58`"
+);
+
#[cfg(feature = "arrow-56")]
extern crate arrow_schema_56 as arrow_schema;📝 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.
| #[cfg(feature = "arrow-56")] | |
| extern crate arrow_schema_56 as arrow_schema; | |
| #[cfg(feature = "arrow-56")] | |
| extern crate arrow_array_56 as arrow_array; | |
| #[cfg(feature = "arrow-56")] | |
| extern crate parquet_56 as parquet; | |
| #[cfg(feature = "arrow-57")] | |
| extern crate arrow_schema_57 as arrow_schema; | |
| #[cfg(feature = "arrow-57")] | |
| extern crate arrow_array_57 as arrow_array; | |
| #[cfg(feature = "arrow-57")] | |
| extern crate parquet_57 as parquet; | |
| #[cfg(feature = "arrow-58")] | |
| extern crate arrow_schema_58 as arrow_schema; | |
| #[cfg(feature = "arrow-58")] | |
| extern crate arrow_array_58 as arrow_array; | |
| #[cfg(feature = "arrow-58")] | |
| extern crate parquet_58 as parquet; | |
| #[cfg(any( | |
| all(feature = "arrow-56", feature = "arrow-57"), | |
| all(feature = "arrow-56", feature = "arrow-58"), | |
| all(feature = "arrow-57", feature = "arrow-58"), | |
| ))] | |
| compile_error!( | |
| "features `arrow-56`, `arrow-57`, and `arrow-58` are mutually exclusive; enable exactly one" | |
| ); | |
| #[cfg(all(feature = "convert", not(feature = "arrow")))] | |
| compile_error!( | |
| "feature `convert` requires one of `arrow-56`, `arrow-57`, or `arrow-58`" | |
| ); | |
| #[cfg(feature = "arrow-56")] | |
| extern crate arrow_schema_56 as arrow_schema; | |
| #[cfg(feature = "arrow-56")] | |
| extern crate arrow_array_56 as arrow_array; | |
| #[cfg(feature = "arrow-56")] | |
| extern crate parquet_56 as parquet; | |
| #[cfg(feature = "arrow-57")] | |
| extern crate arrow_schema_57 as arrow_schema; | |
| #[cfg(feature = "arrow-57")] | |
| extern crate arrow_array_57 as arrow_array; | |
| #[cfg(feature = "arrow-57")] | |
| extern crate parquet_57 as parquet; | |
| #[cfg(feature = "arrow-58")] | |
| extern crate arrow_schema_58 as arrow_schema; | |
| #[cfg(feature = "arrow-58")] | |
| extern crate arrow_array_58 as arrow_array; | |
| #[cfg(feature = "arrow-58")] | |
| extern crate parquet_58 as parquet; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/arrow_aliases.rs` around lines 1 - 20, Add a compile-time guard in
arrow_aliases.rs that emits compile_error! when an invalid feature combination
is detected: check that exactly one of the features "arrow-56", "arrow-57", or
"arrow-58" is enabled and emit a clear compile_error! message if none or more
than one are set; this prevents duplicate extern crate aliases (arrow_schema,
arrow_array, parquet) and avoids unresolved symbols in modules that depend on
them (e.g., src/convert/arrow.rs and src/cli/generate.rs expecting
arrow_schema/arrow_array/parquet); implement the guard before the current extern
crate blocks and refer to the feature flags ("arrow-56","arrow-57","arrow-58")
and the symbols arrow_schema, arrow_array, parquet in the error text.
36e4c96 to
741a6d2
Compare
1 participant
Summary by CodeRabbit
Release Notes