Skip to content

Conversation

@wenym1
Copy link
Contributor

@wenym1 wenym1 commented Oct 15, 2025

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

What's changed and what's your intention?

Source of code changes:

  1. Our forked lru-rs can no longer compiler in the latest toolchain, since it uses Box::into_raw, but in the latest toolchain the method has been disabled for Box with customized Allocator other than Global. Though it's easy to make it compile again, but it seems that the forked crate is only used in bench and rpc_client as a container for client, and therefore we can directly use the official lru-rs.
  2. hashbrown 0.14.5 can not cleanly compiled under nightly toolchain without enabling the nightly feature. Previous the feature is enabled via the forked lru-rs. As we are removing the dependency on the forked crate, we should enable the feature in our workspace explicitly.
  3. Our previous enabled string_to_string has been deprecated, and the compiler indicates that we should instead enable the superset of it implicit_clone. Most code changes are caused by enabling this lint rule.
  4. Removed some explicitly enabled feature that has newly become stable.
  5. supress newly generated warning.

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 15, 2025

@wenym1 wenym1 changed the title temp save chore: bump toolchain to nightly-20251010 Oct 15, 2025
@wenym1 wenym1 marked this pull request as ready for review October 15, 2025 08:46
@wenym1 wenym1 requested a review from a team as a code owner October 15, 2025 08:46
@wenym1 wenym1 requested review from chenzl25 and removed request for a team October 15, 2025 08:46
@graphite-app
Copy link

graphite-app bot commented Oct 15, 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.

@graphite-app graphite-app bot requested a review from a team October 15, 2025 10:24
@wenym1 wenym1 force-pushed the yiming/bump-toolchain-20251010 branch from de80ef0 to 87abbb7 Compare October 15, 2025 11:30
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!

futures = { version = "0.3", default-features = false, features = ["alloc"] }
governor = { workspace = true }
hashbrown = { workspace = true }
hashbrown0_14 = { workspace = true }
Copy link
Member

Choose a reason for hiding this comment

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

What about specifying this in the workspace-config crate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't look like a good idea.

The hashbrown 0.14.5 can only compile in nightly toolchain with its nightly feature is enabled. When doing feature unification to resolve the features enabled in each crate, cargo only include the crate usage when a crate include it in the dependency lists and explicitly use it. Though we have added the hashbrown 0.14.5 in the workspace-config crate, and even have used it with use hashbrown0_14 as _; in the lib.rs, the nightly feature will only be enabled when the crate we are building has included workspace-config in the dependency list. However, the workspace-config is only depended by risingwave_cmd, which means if we build solely on other crates, such as running a test on risingwave_connector, the nightly feature of hashbrown won't be enabled, and then we cannot pass the compile.

To resolve this, we will have to directly or indirectly include workspace-config in all crates that want to be built independently, and the crate risingwave_common is a good common crate that is directly or indirectly depended by most crates. If so, it's better to directly include hashbrown 0.14.5 in risingwave_common, like what we do currently in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Make sense. This somehow reminds me of the workspace-hack crate. I also encountered this in #23453.

Cargo.toml Outdated
Comment on lines 181 to 184
lru = { git = "https://github.com/risingwavelabs/lru-rs.git", rev = "2682b85" }
lru = "0.12"
Copy link
Member

Choose a reason for hiding this comment

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

It seems we've stopped using the forked crate and have instead vendorred it directly into our repository since version #16087.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we have stopped using it in executor. But as mentioned in the PR description, our rpc_client still use it to hold client, and there is logic of iterating over the lru cache, which is not supported in the one implemented in our repo. This code change is for rpc_client to use the official lru-rs crate.

@wenym1 wenym1 force-pushed the yiming/bump-toolchain-20251010 branch from 87abbb7 to a08f8ef Compare October 16, 2025 08:29
@wenym1 wenym1 added this pull request to the merge queue Oct 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Oct 16, 2025
@wenym1 wenym1 force-pushed the yiming/bump-toolchain-20251010 branch from a08f8ef to 64c1eb3 Compare October 16, 2025 10:16
@wenym1 wenym1 added this pull request to the merge queue Oct 16, 2025
Merged via the queue into main with commit c6daacc Oct 16, 2025
39 of 40 checks passed
@wenym1 wenym1 deleted the yiming/bump-toolchain-20251010 branch October 16, 2025 16:57
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