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

Tracking issue for sorted collection ranges #27787

Closed
Gankro opened this Issue Aug 13, 2015 · 92 comments

Comments

Projects
None yet
@Gankro
Contributor

Gankro commented Aug 13, 2015

This covers btree_range and collections_bound. Both try to address the combinatorics over inclusive/exclusive/none bounds on ordered queries. I am significantly unsatisfied with the current solution, which is btree.range(Bound(&K), Bound(&K)). This pushes all the combinatorics into a nasty calling convention. Instead, I believe we would be better served by a build pattern:

for x in btree.range().ge(&min).lt(&max) { .. }

This allows you to avoid importing and dispatching on enum. It also has the advantage of making simpler queries simpler. Compare:

// today (assuming you've totally imported the bound variants)
for x in btree.range(Unbounded, Inclusive(&max)) {

// tomorrow?
for x in btree.range().le(&max) { .. }

This also could potentially address rust-lang/rfcs#1195

let pred = btree.range().le(&max).get();

And can be extended to support drain or remove similarly;

for x in btree.range_mut().gt(&min).drain() { .. }
ler pred = btree.range_mut().le(&max).remove();

Which if desirable can be sugarred to direct methods on btree in the future.

@Gankro Gankro added the B-unstable label Aug 13, 2015

@Gankro Gankro changed the title from sorted collection ranges to Tracking issue for sorted collection ranges Aug 13, 2015

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro
Contributor

Gankro commented Aug 13, 2015

@apasel422

This comment has been minimized.

Show comment
Hide comment
@apasel422

apasel422 Aug 13, 2015

Member

Will this still allow for efficient implementations of things like just the
predecessor (without iteration)?

On Wednesday, August 12, 2015, Alexis Beingessner notifications@github.com
wrote:

CC @bluss https://github.com/bluss @apasel422
https://github.com/apasel422


Reply to this email directly or view it on GitHub
#27787 (comment).

Member

apasel422 commented Aug 13, 2015

Will this still allow for efficient implementations of things like just the
predecessor (without iteration)?

On Wednesday, August 12, 2015, Alexis Beingessner notifications@github.com
wrote:

CC @bluss https://github.com/bluss @apasel422
https://github.com/apasel422


Reply to this email directly or view it on GitHub
#27787 (comment).

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Aug 13, 2015

Contributor

@apasel422 Yes the builder is completely lazy -- get would just extract the one set bound and do lt/le/gt/whatever as appropriate.

Contributor

Gankro commented Aug 13, 2015

@apasel422 Yes the builder is completely lazy -- get would just extract the one set bound and do lt/le/gt/whatever as appropriate.

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Aug 13, 2015

Contributor

I should note the builder API is considerably more hairy internally. Basically requires converting all the static types into dynamic types and then dispatching to the existing impls (for IntoIter) or those proposed in rust-lang/rfcs#1195

Contributor

Gankro commented Aug 13, 2015

I should note the builder API is considerably more hairy internally. Basically requires converting all the static types into dynamic types and then dispatching to the existing impls (for IntoIter) or those proposed in rust-lang/rfcs#1195

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Aug 13, 2015

Contributor

Shoutouts to @zentner-kyle for proposing this API. Big improvement.

Contributor

Gankro commented Aug 13, 2015

Shoutouts to @zentner-kyle for proposing this API. Big improvement.

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Aug 13, 2015

Contributor

In practice, with the parent pointer design I anticipate the guts of BTreeMap having the query API just do raw pointers and be an internal impl detail: get_lt(&BTreeMap, &Q) -> Handle<*mut Node>

A range knows how to compute handles from bounds by dispatching to the query API, and then get or into_iter are just munging up handles into a (K, V) pair or Iter(Handle, Handle).

Contributor

Gankro commented Aug 13, 2015

In practice, with the parent pointer design I anticipate the guts of BTreeMap having the query API just do raw pointers and be an internal impl detail: get_lt(&BTreeMap, &Q) -> Handle<*mut Node>

A range knows how to compute handles from bounds by dispatching to the query API, and then get or into_iter are just munging up handles into a (K, V) pair or Iter(Handle, Handle).

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Aug 13, 2015

Contributor

@apasel422 does this sound good to you?

Contributor

Gankro commented Aug 13, 2015

@apasel422 does this sound good to you?

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro
Contributor

Gankro commented Aug 13, 2015

@alexcrichton alexcrichton added the T-libs label Aug 13, 2015

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Aug 14, 2015

Contributor

I have made an RFC to this affect: rust-lang/rfcs#1254

Contributor

Gankro commented Aug 14, 2015

I have made an RFC to this affect: rust-lang/rfcs#1254

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 27, 2016

Member

Removing the B-unstable tag as I believe these haven't been implemented.

Member

alexcrichton commented Jan 27, 2016

Removing the B-unstable tag as I believe these haven't been implemented.

@alexcrichton alexcrichton added A-libs and removed B-unstable T-libs labels Jan 27, 2016

@Gankro Gankro added the B-unstable label Jan 28, 2016

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Jan 28, 2016

Contributor

@alexcrichton range and range_mut have been around since 1.0, I think. But they won't be stabilized as-is.

Contributor

Gankro commented Jan 28, 2016

@alexcrichton range and range_mut have been around since 1.0, I think. But they won't be stabilized as-is.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jan 28, 2016

Member

Oh oops there are indeed some unstable APIs tagged with this.

Member

alexcrichton commented Jan 28, 2016

Oh oops there are indeed some unstable APIs tagged with this.

@alexcrichton alexcrichton added T-libs and removed A-libs labels Jan 28, 2016

@naktinis

This comment has been minimized.

Show comment
Hide comment
@naktinis

naktinis Feb 29, 2016

What's the state of this? I'm using btree ranges in production code and would be happy to help as much as I can to stabilize this feature.

naktinis commented Feb 29, 2016

What's the state of this? I'm using btree ranges in production code and would be happy to help as much as I can to stabilize this feature.

@calebmer

This comment has been minimized.

Show comment
Hide comment
@calebmer

calebmer Mar 11, 2016

I also would like to know the status as I'm building a program which uses ranges and if there is an idiomatic rust solution I'd love to use that over my own implementation.

calebmer commented Mar 11, 2016

I also would like to know the status as I'm building a program which uses ranges and if there is an idiomatic rust solution I'd love to use that over my own implementation.

@apasel422

This comment has been minimized.

Show comment
Hide comment
@apasel422

apasel422 Apr 12, 2016

Member

My understanding is that we're waiting on a new RFC in lieu of rust-lang/rfcs#1254.

Member

apasel422 commented Apr 12, 2016

My understanding is that we're waiting on a new RFC in lieu of rust-lang/rfcs#1254.

@naktinis

This comment has been minimized.

Show comment
Hide comment
@naktinis

naktinis Apr 13, 2016

How can we contribute to the discussion or at least keep track of the remaining issues?

naktinis commented Apr 13, 2016

How can we contribute to the discussion or at least keep track of the remaining issues?

@apasel422

This comment has been minimized.

Show comment
Hide comment
@apasel422

apasel422 Apr 13, 2016

Member

@naktinis A pre-RFC or general discussion thread on https://internals.rust-lang.org/ would probably work.

Member

apasel422 commented Apr 13, 2016

@naktinis A pre-RFC or general discussion thread on https://internals.rust-lang.org/ would probably work.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 25, 2016

Member

cc #33197, a case where the APIs currently segfault apparently

Member

alexcrichton commented Apr 25, 2016

cc #33197, a case where the APIs currently segfault apparently

@ticki

This comment has been minimized.

Show comment
Hide comment
@ticki

ticki May 9, 2016

Contributor

It seems to me that the extension Bound provides over Range is very small. I wonder why .range() doesn't take Range instead.

Contributor

ticki commented May 9, 2016

It seems to me that the extension Bound provides over Range is very small. I wonder why .range() doesn't take Range instead.

@apasel422

This comment has been minimized.

Show comment
Hide comment
@apasel422

apasel422 May 9, 2016

Member

@ticki Range is [start, end), but Bound provides inclusive/exclusive/unbounded on both ends.

Member

apasel422 commented May 9, 2016

@ticki Range is [start, end), but Bound provides inclusive/exclusive/unbounded on both ends.

bors added a commit that referenced this issue Jan 14, 2017

Auto merge of #38610 - djzin:master, r=sfackler
Implementation of plan in issue #27787 for btree_range

Still some ergonomics to be worked on, the ::<str,_> is particularly unsightly

bors added a commit that referenced this issue Jan 15, 2017

Auto merge of #38610 - djzin:master, r=sfackler
Implementation of plan in issue #27787 for btree_range

Still some ergonomics to be worked on, the ::<str,_> is particularly unsightly
@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Feb 3, 2017

Member

Now that the changes to the surface API have landed, I think we're ready to consider stabilization.

@rfcbot rfc merge

Member

aturon commented Feb 3, 2017

Now that the changes to the surface API have landed, I think we're ready to consider stabilization.

@rfcbot rfc merge

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Feb 3, 2017

Member

Grr:

@rfcbot fcp merge

Member

aturon commented Feb 3, 2017

Grr:

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Feb 3, 2017

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Feb 3, 2017

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Feb 3, 2017

Member

@aturon, to clarify, what APIs are you thinking of stabilizing as part of this? Just BTreeMap::range{,_mut} or also the RangeArgument trait and associated types?

Member

alexcrichton commented Feb 3, 2017

@aturon, to clarify, what APIs are you thinking of stabilizing as part of this? Just BTreeMap::range{,_mut} or also the RangeArgument trait and associated types?

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Feb 3, 2017

Member

@alexcrichton I think we should start with just the method for now, and let the trait bake a while longer.

Member

aturon commented Feb 3, 2017

@alexcrichton I think we should start with just the method for now, and let the trait bake a while longer.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Feb 3, 2017

Member

In that case you have my checkmark!

Member

alexcrichton commented Feb 3, 2017

In that case you have my checkmark!

@djzin

This comment has been minimized.

Show comment
Hide comment
@djzin

djzin Feb 4, 2017

Contributor

There is a small ergonomic issue I came across while writing tests for this relating to taking a range by borrowed key; say you have a map with strings as keys, then it would be nice to just use range syntax to specify the range. Unfortunately this doesn't work, because you would need a Range<str> which is unsized. Changing the bounds required type annotations in the common case. Instead what I did is implement RangeArgument<T> for (Bound<&T>, Bound<&T>), allowing you to do this, but it still requires type annotations.

#![feature(btree_range)]
#![feature(collections_bound)]

use std::collections::btree_map::BTreeMap;
use std::collections::Bound::*;

fn main() {
    let mut map = BTreeMap::new();
    map.insert("key".to_string(), "value".to_string());
    map.insert("lee".to_string(), "walue".to_string());
    map.insert("me".to_string(), "xalue".to_string());
    // let range = map.range("key".."me"); // doesn't work
    // let range = map.range((Included("key"), Excluded("me"))); // type annotations required
    let range = map.range::<str, _>((Included("key"), Excluded("me"))); // finally
    for (key, value) in range {
        println!("({}, {})", key, value);
    }
}

I suppose this is covered by not stabilizing RangeArgument, though?

Contributor

djzin commented Feb 4, 2017

There is a small ergonomic issue I came across while writing tests for this relating to taking a range by borrowed key; say you have a map with strings as keys, then it would be nice to just use range syntax to specify the range. Unfortunately this doesn't work, because you would need a Range<str> which is unsized. Changing the bounds required type annotations in the common case. Instead what I did is implement RangeArgument<T> for (Bound<&T>, Bound<&T>), allowing you to do this, but it still requires type annotations.

#![feature(btree_range)]
#![feature(collections_bound)]

use std::collections::btree_map::BTreeMap;
use std::collections::Bound::*;

fn main() {
    let mut map = BTreeMap::new();
    map.insert("key".to_string(), "value".to_string());
    map.insert("lee".to_string(), "walue".to_string());
    map.insert("me".to_string(), "xalue".to_string());
    // let range = map.range("key".."me"); // doesn't work
    // let range = map.range((Included("key"), Excluded("me"))); // type annotations required
    let range = map.range::<str, _>((Included("key"), Excluded("me"))); // finally
    for (key, value) in range {
        println!("({}, {})", key, value);
    }
}

I suppose this is covered by not stabilizing RangeArgument, though?

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Feb 6, 2017

Member

@djzin We have to be a bit careful: once the range method is stabilized, we're committing to it working for at least all of the arguments allowed by today's definition of RangeArgument. That is, we can change the definition and impls for RangeArgument but only if all uses of range will continue to work.

Member

aturon commented Feb 6, 2017

@djzin We have to be a bit careful: once the range method is stabilized, we're committing to it working for at least all of the arguments allowed by today's definition of RangeArgument. That is, we can change the definition and impls for RangeArgument but only if all uses of range will continue to work.

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Feb 7, 2017

Contributor

I propose that #33197 should be fixed before stabilization (unfortunately), but it's being fixed.

Contributor

bluss commented Feb 7, 2017

I propose that #33197 should be fixed before stabilization (unfortunately), but it's being fixed.

@sgrif sgrif referenced this issue Feb 11, 2017

Closed

Support PG ranges #676

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Feb 13, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Feb 13, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Feb 13, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Feb 13, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Feb 13, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Feb 13, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@djzin

This comment has been minimized.

Show comment
Hide comment
@djzin

djzin Feb 18, 2017

Contributor

The first issue I listed above (not being able to use range syntax) may be fixed at a later time by adding impls such as

impl<'a, T: ?Sized> RangeArgument<T> for Range<&'a T> {
    fn start(&self) -> Bound<&T> { Included(self.start) }
    fn end(&self) -> Bound<&T> { Excluded(self.end) }
}

Then one may use map.range::<str, _>("a".."z").
A similar impl could be provided for &'a mut T if that were a thing.

For some reason, even though there is only one type that can fit here (as far as I can tell), rust still requires type annotations for the str. This seems to me like improvements in rust's type inference down the line may fix this ergonomic issue.

A related problem occurs with RangeFull aka .. - rust cannot derive the type for a String key. In this case there are multiple options, all with the same result. I feel as though this is a non-issue as selecting the whole range is a no-op.

None of these issues affect BTreeMaps with an i64 key (or any key that has no nontrivial implementation of Borrow), and the main ones are solvable in future backwards-compatibly. So I think, as this feature has been in unstable limbo for so long, it's due its chance to shine in stable rust-land :)

Contributor

djzin commented Feb 18, 2017

The first issue I listed above (not being able to use range syntax) may be fixed at a later time by adding impls such as

impl<'a, T: ?Sized> RangeArgument<T> for Range<&'a T> {
    fn start(&self) -> Bound<&T> { Included(self.start) }
    fn end(&self) -> Bound<&T> { Excluded(self.end) }
}

Then one may use map.range::<str, _>("a".."z").
A similar impl could be provided for &'a mut T if that were a thing.

For some reason, even though there is only one type that can fit here (as far as I can tell), rust still requires type annotations for the str. This seems to me like improvements in rust's type inference down the line may fix this ergonomic issue.

A related problem occurs with RangeFull aka .. - rust cannot derive the type for a String key. In this case there are multiple options, all with the same result. I feel as though this is a non-issue as selecting the whole range is a no-op.

None of these issues affect BTreeMaps with an i64 key (or any key that has no nontrivial implementation of Borrow), and the main ones are solvable in future backwards-compatibly. So I think, as this feature has been in unstable limbo for so long, it's due its chance to shine in stable rust-land :)

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Feb 23, 2017

The final comment period is now complete.

rfcbot commented Feb 23, 2017

The final comment period is now complete.

frewsxcv pushed a commit to frewsxcv/rust that referenced this issue Mar 17, 2017

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 17, 2017

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 17, 2017

@bors bors closed this in 37b38a2 Mar 19, 2017

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Mar 29, 2017

Contributor

!?!? REALLY?

Contributor

Gankro commented Mar 29, 2017

!?!? REALLY?

@solson

This comment has been minimized.

Show comment
Hide comment
@solson

solson Mar 29, 2017

Member

@Gankro I was shocked too: solson/miri#155 :)

this feature has been stable since ____. Attribute no longer needed is the best warning in rustc.

Member

solson commented Mar 29, 2017

@Gankro I was shocked too: solson/miri#155 :)

this feature has been stable since ____. Attribute no longer needed is the best warning in rustc.

@cgaebel

This comment has been minimized.

Show comment
Hide comment
@cgaebel

cgaebel Mar 29, 2017

Contributor
Contributor

cgaebel commented Mar 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment