Skip to content

Conversation

@wenym1
Copy link
Contributor

@wenym1 wenym1 commented Oct 16, 2025

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Add an experimental rustc flag "-Zhigher-ranked-assumptions" to bypass the false positive borrow checker error discussed in rust-lang/rust#100013. The rustc flag is recently enabled after rust-lang/rust#143545 gets merged to the rust toolchain.

After enabling this rustc flag, for the on_key_value and nearest state store trait method, which pass a closure to observe the slice value, the closure does not have to be static, and in the method caller, we can avoid unnecessary clone to make the closure static.

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • I have checked the Release Timeline and Currently Supported Versions to determine which release branches I need to cherry-pick this PR into.

Documentation

  • My PR needs documentation updates.
Release note

Copy link
Contributor Author

wenym1 commented Oct 16, 2025

@wenym1 wenym1 changed the title temp save refactor(storage): use new higher rank lifetime assumption Oct 16, 2025
@github-actions github-actions bot added type/refactor Type: Refactoring. and removed Invalid PR Title labels Oct 16, 2025
@wenym1 wenym1 changed the title refactor(storage): use new higher rank lifetime assumption refactor(storage): use new higher rank lifetime assumption to avoid unnecessary clone Oct 16, 2025
@wenym1 wenym1 marked this pull request as ready for review October 16, 2025 09:58
@wenym1 wenym1 changed the base branch from yiming/bump-toolchain-20251010 to graphite-base/23490 October 16, 2025 10:16
@wenym1 wenym1 force-pushed the graphite-base/23490 branch from a08f8ef to 64c1eb3 Compare October 16, 2025 10:19
@wenym1 wenym1 force-pushed the yiming/storage-pass-higher-ranked-lifetime branch from 3693fe9 to 5d0b2a0 Compare October 16, 2025 10:19
@wenym1 wenym1 changed the base branch from graphite-base/23490 to yiming/bump-toolchain-20251010 October 16, 2025 10:19
Base automatically changed from yiming/bump-toolchain-20251010 to main October 16, 2025 16:57
@buildkite-limited-access buildkite-limited-access bot requested a review from a team as a code owner October 16, 2025 16:57
@buildkite-limited-access buildkite-limited-access bot requested review from xiangjinwu and removed request for a team October 16, 2025 16:57
@graphite-app
Copy link

graphite-app bot commented Oct 16, 2025

Looks like this PR extends new SQL syntax or updates existing ones. Make sure that:

  • Test cases about the new/updated syntax are added in src/sqlparser/tests/testdata. Especially, double check the formatted_sql is still a valid SQL #20713
  • The meaning of each enum variant is documented in PR description. Additionally, document what it means when each optional clause is omitted.

@wenym1 wenym1 force-pushed the yiming/storage-pass-higher-ranked-lifetime branch from 5d0b2a0 to aaf3dd0 Compare October 17, 2025 05:36
@wenym1 wenym1 force-pushed the yiming/storage-pass-higher-ranked-lifetime branch from cc04230 to c6bac47 Compare October 20, 2025 03:38
Copy link
Member

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

LGTM!


echo "--- Build documentation"
RUSTDOCFLAGS="-Dwarnings" cargo doc --document-private-items --no-deps
RUSTDOCFLAGS="-Dwarnings -Zhigher-ranked-assumptions" cargo doc --document-private-items --no-deps
Copy link
Member

Choose a reason for hiding this comment

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

Shall we by default to also set -Dwarnings in config.toml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is a good idea. -Dwarnings is used here because we want to quickly detect the warning and return with error in CI, but not a general requirement when we build the doc.

@wenym1 wenym1 added this pull request to the merge queue Oct 21, 2025
Merged via the queue into main with commit 0e2472a Oct 21, 2025
39 of 40 checks passed
@wenym1 wenym1 deleted the yiming/storage-pass-higher-ranked-lifetime branch October 21, 2025 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants