Check in Cargo.lock

[alamb]

Which issue does this PR close?

N/A

Rationale for this change

Checking in Cargo.lock gives CI reproducible builds and makes dependency updates visible in review, per the current Cargo guidance which now recommends committing the lockfile for libraries as well.

What changes are included in this PR?

• Remove Cargo.lock from .gitignore
• Check in Cargo.lock generated with cargo update (all 46 packages locked to the latest semver-compatible versions; no Cargo.toml changes needed)

Are these changes tested?

cargo test --all-features passes (22/22 suites, 0 failures).

Are there any user-facing changes?

No. This only affects local development and CI of this repository; downstream users of the published crate are unaffected.

[LucaCappelletti94]

To make dependency updates reliably visible in review, maybe we should use --locked in the CI? The sqlparser_bench job uses an independent manifest, so it likely needs its own committed lockfile, or possibly better a workspace integration.

[LucaCappelletti94]

At this time https://github.com/apache/datafusion-sqlparser-rs/blob/main/.tool-versions still specifies Rust 1.75, and version = 4 is only supported from 1.78. I suspect that the current CI misses this because it tests only stable, beta, and nightly, and not specifically 1.75.

I am not sure whether here we would want to either bump .tool-versions or downgrade the version of the Cargo.lock.

[alamb]

thanks @LucaCappelletti94 -- I defer to your judgement for such changes. Maybe we should just punt on thsi PR until we have some issue due to dependencies 🤷

[LucaCappelletti94]

thanks @LucaCappelletti94 -- I defer to your judgement for such changes. Maybe we should just punt on thsi PR until we have some issue due to dependencies 🤷

I had to Google what punt meant lol, I hadn't heard it yet. Anyway sure I do not believe it is biting at this time, we can extend this PR with the updated CI later. I currently cannot yet push commits here to adjust the aforementioned things, but I suppose I should be able to next week.

[alamb]

Sounds good -- thank you

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.96%. Comparing base (667e81d) to head (e9b3ef6).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2482   +/-   ##
=======================================
  Coverage   80.96%   80.96%           
=======================================
  Files          42       42           
  Lines       33373    33373           
  Branches    33373    33373           
=======================================
  Hits        27022    27022           
  Misses       2789     2789           
  Partials     3562     3562           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[LucaCappelletti94]

@alamb I pushed the follow-ups from my review to your branch. sqlparser_bench is now a workspace member, so one lockfile covers everything except fuzz/, which keeps its own workspace and ignores its lockfile as before. Every cargo command in CI now runs with --locked, so a dependency change that is not committed to Cargo.lock fails the build, and the lockfile diff is where reviewers see it.

.tool-versions is bumped to 1.88.0 and kept the lockfile at version 4. Making sqlparser_derive a workspace member also brings it under clippy, which flagged one sort_by that I changed to sort_by_key.

One consequence worth knowing is that the daily cargo audit now checks the committed lockfile, so a new advisory will fail it until someone runs cargo update.

LMK whether all looks good to you.

[alamb]

Thanks @LucaCappelletti94 -- other than the change to tool version this makes sense to me

[alamb]

Does this change the "msrv" of the crate (the minimum supported rust version) or just the versions that run the tests?

[LucaCappelletti94]

The .tool-versions bump leaves the crate's MSRV untouched. The crate declares no rust-version in Cargo.toml (maybe we should), so cargo enforces no MSRV for downstream users, and downstream builds resolve their own dependency graph without reading our Cargo.lock. CI does not read .tool-versions either.

The file only selects the toolchain that asdf and mise install for local development in this checkout.

[alamb]

The file only selects the toolchain that asdf and mise install for local development in this checkou

I thought it changes the version of rust used for clippy, etc

So that by changing this we will implicitly start testing with a newer version of rust 🤔

[LucaCappelletti94]

No, those tools go by the version specified in CI, which currently is:

datafusion-sqlparser-rs/.github/workflows/rust.yml

Line 95 in 0fd4fa8

rust: [stable, beta, nightly]

[alamb]

ok, I think i was confused with rust-toolchain.toml (e.g. https://github.com/apache/arrow-rs/blob/main/rust-toolchain.toml). Sorry about that

comments addressed

[alamb]

Looks ready to me @LucaCappelletti94 -- thank you