Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rust modules: Revise BTreeIndexBounds #1815

Merged
merged 6 commits into from
Oct 22, 2024
Merged

Conversation

gefjon
Copy link
Contributor

@gefjon gefjon commented Oct 9, 2024

Description of Changes

  • Make BTree filter operators reject owned values of non-Copy types, so that diagnostics prefer to suggest &x instead of x.clone().
  • Restrict BTree filter args to a small set of types defined by our previous "Consistent filtering rules" proposal, per @RReverser 's request.
  • Also make it so that String columns can be queried by &str arguments and ranges thereof.
  • Add some comments to the macros that implement BTreeIndexBounds, because it's a doozy.

For reasons that escape me, this breaks inference on integer literals: they get defaulted too early to i32, even when the indexed column is of a different type like u32 or i64. We could potentially work around this by making i32 a FilterableValue for any integer column type, but that opens the door for some nasty truncation-related bugs. Failing that, we'll have to decide whether allowing &str for String, and owned values for Copy types, is more valuable than correctly inferring the types of integer literals (which would in that case be &0 instead of 0i64).

API and ABI breaking changes

BTreeIndexBounds is now less permissive than it was in the 0.12 release. Also the integer literal thing.

Expected complexity level and risk

3 - Complex macros and traits, but relatively contained. The only undesirable interactions I can forsee are other cases like the integer literal thing where type inference gets less reliable because we have more complex traits involved.

Testing

Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!

  • Wrote a new test case in rust-wasm-test which demonstrates many combinations of valid args.
  • Added some comments in the same place with some invalid combinations, and tested uncommenting them and seeing that they do not compile.

…rable types

And add some comments to the macros which implement it.

For reasons that escape me, this breaks inference on integer literals:
they get defaulted too early to `i32`,
even when the indexed column is of a different type like `u32` or `i64`.
We could potentially work around this
by making `i32` a `FilterableValue` for any integer column type,
but that opens the door for some nasty truncation-related bugs.
gefjon added 2 commits October 9, 2024 16:46
iTeRatORs aRe LAzY ANd Do nothiNG UnLEsS consUmed
@gefjon gefjon requested a review from cloutiertyler October 10, 2024 18:09
Copy link
Collaborator

@coolreader18 coolreader18 left a comment

Choose a reason for hiding this comment

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

I guess if that's the direction we wanna go, these changes look good to me.

gefjon added a commit to clockworklabs/spacetime-docs that referenced this pull request Oct 22, 2024
Companion to clockworklabs/SpacetimeDB#1815

Also fix surrounding example code and text: you filter on indices, not columns.
@gefjon gefjon added this pull request to the merge queue Oct 22, 2024
gefjon added a commit to clockworklabs/spacetime-docs that referenced this pull request Oct 22, 2024
Companion to clockworklabs/SpacetimeDB#1815

Also fix surrounding example code and text: you filter on indices, not columns.
Merged via the queue into master with commit 437eed7 Oct 22, 2024
9 checks passed
bfops added a commit to clockworklabs/spacetime-docs that referenced this pull request Nov 6, 2024
* Whitespace (#98)

* Add note about integer literal type inference (#100)

Companion to clockworklabs/SpacetimeDB#1815

Also fix surrounding example code and text: you filter on indices, not columns.

---------

Co-authored-by: Tyler Cloutier <[email protected]>
Co-authored-by: Phoebe Goldman <[email protected]>
@bfops bfops added the api-break A PR that makes an API breaking change label Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break A PR that makes an API breaking change release-rc1-nice-to-have
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants