Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1409613002:
Use the sort.Search api correctly. (Closed)
DescriptionUse the sort.Search api correctly.
I was using it again and I realized that I used it wrong here :(. Includes
regression test.
R=dnj@chromium.org, estaab@chromium.org, maruel@chromium.org, tandrii@chromium.org, vadimsh@chromium.org
BUG=
Committed: https://github.com/luci/gae/commit/43f0ab3a4229c46c418273d0334ade6b46f45a38
Patch Set 1 #
Total comments: 1
Messages
Total messages: 14 (1 generated)
lgtm
lgtm w/ thought. https://chromiumcodereview.appspot.com/1409613002/diff/1/service/datastore/qu... File service/datastore/query.go (right): https://chromiumcodereview.appspot.com/1409613002/diff/1/service/datastore/qu... service/datastore/query.go:278: copy(s[idx+1:], s[idx:]) nit: WDYT about making eqfilts a binheap?
On 2015/10/15 at 02:23:46, dnj wrote: > lgtm w/ thought. > > https://chromiumcodereview.appspot.com/1409613002/diff/1/service/datastore/qu... > File service/datastore/query.go (right): > > https://chromiumcodereview.appspot.com/1409613002/diff/1/service/datastore/qu... > service/datastore/query.go:278: copy(s[idx+1:], s[idx:]) > nit: WDYT about making eqfilts a binheap? Is there a stdlib package for binheap that I'm not seeing?
On 2015/10/15 05:18:08, iannucci wrote: > On 2015/10/15 at 02:23:46, dnj wrote: > > lgtm w/ thought. > > > > > https://chromiumcodereview.appspot.com/1409613002/diff/1/service/datastore/qu... > > File service/datastore/query.go (right): > > > > > https://chromiumcodereview.appspot.com/1409613002/diff/1/service/datastore/qu... > > service/datastore/query.go:278: copy(s[idx+1:], s[idx:]) > > nit: WDYT about making eqfilts a binheap? > > Is there a stdlib package for binheap that I'm not seeing? Spluh! https://golang.org/pkg/container/heap/
On 2015/10/15 at 08:18:54, dnj wrote: > On 2015/10/15 05:18:08, iannucci wrote: > > On 2015/10/15 at 02:23:46, dnj wrote: > > > lgtm w/ thought. > > > > > > > > https://chromiumcodereview.appspot.com/1409613002/diff/1/service/datastore/qu... > > > File service/datastore/query.go (right): > > > > > > > > https://chromiumcodereview.appspot.com/1409613002/diff/1/service/datastore/qu... > > > service/datastore/query.go:278: copy(s[idx+1:], s[idx:]) > > > nit: WDYT about making eqfilts a binheap? > > > > Is there a stdlib package for binheap that I'm not seeing? > > Spluh! https://golang.org/pkg/container/heap/ Hm. Heap doesn't do deduplication, and because it's a heap, you can't use sort.Search to determine if the heap contains a certain value. I'll keep it in mind for later, but I don't think that it's the right choice here.
The CQ bit was checked by iannucci@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1409613002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1409613002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://github.com/luci/gae/commit/43f0ab3a4229c46c418273d0334ade6b46f45a38
Message was sent while issue was closed.
On 2015/10/15 20:37:34, iannucci wrote: > On 2015/10/15 at 08:18:54, dnj wrote: > > On 2015/10/15 05:18:08, iannucci wrote: > > > On 2015/10/15 at 02:23:46, dnj wrote: > > > > lgtm w/ thought. > > > > > > > > > > > > https://chromiumcodereview.appspot.com/1409613002/diff/1/service/datastore/qu... > > > > File service/datastore/query.go (right): > > > > > > > > > > > > https://chromiumcodereview.appspot.com/1409613002/diff/1/service/datastore/qu... > > > > service/datastore/query.go:278: copy(s[idx+1:], s[idx:]) > > > > nit: WDYT about making eqfilts a binheap? > > > > > > Is there a stdlib package for binheap that I'm not seeing? > > > > Spluh! https://golang.org/pkg/container/heap/ > > Hm. Heap doesn't do deduplication, and because it's a heap, you can't use > sort.Search to determine if the heap contains a certain value. I'll keep it in > mind for later, but I don't think that it's the right choice here. I think Uniq() from https://github.com/xtgo/set implements Uniq() is the better choice.
Message was sent while issue was closed.
On 2015/10/15 20:47:07, M-A Ruel wrote: > On 2015/10/15 20:37:34, iannucci wrote: > > On 2015/10/15 at 08:18:54, dnj wrote: > > > On 2015/10/15 05:18:08, iannucci wrote: > > > > On 2015/10/15 at 02:23:46, dnj wrote: > > > > > lgtm w/ thought. > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/1409613002/diff/1/service/datastore/qu... > > > > > File service/datastore/query.go (right): > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/1409613002/diff/1/service/datastore/qu... > > > > > service/datastore/query.go:278: copy(s[idx+1:], s[idx:]) > > > > > nit: WDYT about making eqfilts a binheap? > > > > > > > > Is there a stdlib package for binheap that I'm not seeing? > > > > > > Spluh! https://golang.org/pkg/container/heap/ > > > > Hm. Heap doesn't do deduplication, and because it's a heap, you can't use > > sort.Search to determine if the heap contains a certain value. I'll keep it in > > mind for later, but I don't think that it's the right choice here. > > I think Uniq() from https://github.com/xtgo/set implements Uniq() is the better > choice. Wow, that's an ugly interface... Are the pivot arguments really necessary? I mean... sort.Search is a super ugly, but I think this one takes the cake... /me just wants std::set<T> :(
Message was sent while issue was closed.
On 2015/10/15 21:00:04, iannucci1 wrote: > On 2015/10/15 20:47:07, M-A Ruel wrote: > > I think Uniq() from https://github.com/xtgo/set implements Uniq() is the > better > > choice. > > Wow, that's an ugly interface... Are the pivot arguments really necessary? I > mean... sort.Search is a super ugly, but I think this one takes the cake... I disagree, there's no interface, just a few specialized functions. Why do you dislike it? e.g. https://godoc.org/github.com/xtgo/set#Uniq See the example, you can't go any simpler.
Message was sent while issue was closed.
On 2015/10/15 21:00:04, iannucci1 wrote: > On 2015/10/15 20:47:07, M-A Ruel wrote: > > On 2015/10/15 20:37:34, iannucci wrote: > > > On 2015/10/15 at 08:18:54, dnj wrote: > > > > On 2015/10/15 05:18:08, iannucci wrote: > > > > > On 2015/10/15 at 02:23:46, dnj wrote: > > > > > > lgtm w/ thought. > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/1409613002/diff/1/service/datastore/qu... > > > > > > File service/datastore/query.go (right): > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/1409613002/diff/1/service/datastore/qu... > > > > > > service/datastore/query.go:278: copy(s[idx+1:], s[idx:]) > > > > > > nit: WDYT about making eqfilts a binheap? > > > > > > > > > > Is there a stdlib package for binheap that I'm not seeing? > > > > > > > > Spluh! https://golang.org/pkg/container/heap/ > > > > > > Hm. Heap doesn't do deduplication, and because it's a heap, you can't use > > > sort.Search to determine if the heap contains a certain value. I'll keep it > in > > > mind for later, but I don't think that it's the right choice here. > > > > I think Uniq() from https://github.com/xtgo/set implements Uniq() is the > better > > choice. > > Wow, that's an ugly interface... Are the pivot arguments really necessary? I > mean... sort.Search is a super ugly, but I think this one takes the cake... > > /me just wants std::set<T> :( What are you discussing? How to optimize a search in an array of size of <=5?.. (Unless I misunderstand something, the array in question holds filters in the query. In any realistic situation it isn't going to be large). Stupidest array is fastest in such situation.
Message was sent while issue was closed.
On 2015/10/15 at 21:05:03, vadimsh wrote: > On 2015/10/15 21:00:04, iannucci1 wrote: > > On 2015/10/15 20:47:07, M-A Ruel wrote: > > > On 2015/10/15 20:37:34, iannucci wrote: > > > > On 2015/10/15 at 08:18:54, dnj wrote: > > > > > On 2015/10/15 05:18:08, iannucci wrote: > > > > > > On 2015/10/15 at 02:23:46, dnj wrote: > > > > > > > lgtm w/ thought. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/1409613002/diff/1/service/datastore/qu... > > > > > > > File service/datastore/query.go (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/1409613002/diff/1/service/datastore/qu... > > > > > > > service/datastore/query.go:278: copy(s[idx+1:], s[idx:]) > > > > > > > nit: WDYT about making eqfilts a binheap? > > > > > > > > > > > > Is there a stdlib package for binheap that I'm not seeing? > > > > > > > > > > Spluh! https://golang.org/pkg/container/heap/ > > > > > > > > Hm. Heap doesn't do deduplication, and because it's a heap, you can't use > > > > sort.Search to determine if the heap contains a certain value. I'll keep it > > in > > > > mind for later, but I don't think that it's the right choice here. > > > > > > I think Uniq() from https://github.com/xtgo/set implements Uniq() is the > > better > > > choice. > > > > Wow, that's an ugly interface... Are the pivot arguments really necessary? I > > mean... sort.Search is a super ugly, but I think this one takes the cake... > > > > /me just wants std::set<T> :( > > What are you discussing? How to optimize a search in an array of size of <=5?.. (Unless I misunderstand something, the array in question holds filters in the query. In any realistic situation it isn't going to be large). Stupidest array is fastest in such situation. Yes, there's no performance critical code here :) If we keep on talking about it, I'll replace it with a linear search + full copy :P |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
