Initial attempt at efficient range queries across multiple Set objects. #10

davidblewett · 2018-01-10T15:05:05Z

This is not compiling yet (using rustc 1.24.0-nightly (0a3761e63 2018-01-03)):

(dblewett)Wed Jan 10, 10:03 | /home/dblewett/src/python-rust-fst/fstwrapper
rusty% cargo build          
 Compiling fst-wrapper v0.3.0 (file:///home/dblewett/src/python-rust-fst/fstwrapper)
error[E0277]: the trait bound `for<'a> &fst::set::Stream<'_>: fst::Streamer<'a>` is not satisfied
 --> src/set.rs:152:36
  |
152 |     let ob = set::OpBuilder::new().add(stream);
  |                                    ^^^ the trait `for<'a> fst::Streamer<'a>` is not implemented for `&fst::set::Stream<'_>`
  |
  = help: the following implementations were found:
            <fst::set::Stream<'s, A> as fst::Streamer<'a>>

error[E0277]: the trait bound `for<'a> &fst::set::Stream<'_>: fst::Streamer<'a>` is not satisfied
 --> src/set.rs:168:8
  |
168 |     ob.push(stream);
  |        ^^^^ the trait `for<'a> fst::Streamer<'a>` is not implemented for `&fst::set::Stream<'_>`
  |
  = help: the following implementations were found:
            <fst::set::Stream<'s, A> as fst::Streamer<'a>>

error: aborting due to 2 previous errors

error: Could not compile `fst-wrapper`.

BurntSushi · 2018-01-18T00:12:04Z

fstwrapper/src/set.rs

@@ -146,13 +146,28 @@ pub extern "C" fn fst_set_make_opbuilder(ptr: *mut Set) -> *mut set::OpBuilder<'
 }
 make_free_fn!(fst_set_opbuilder_free, *mut set::OpBuilder);

+#[no_mangle]
+pub extern "C" fn fst_set_make_opstreambuilder(ptr: *mut set::Stream) -> *mut set::OpBuilder<'static> {
+    let stream = ref_from_ptr!(ptr);


@davidblewett I think you want stream to be a set::Stream here, but it looks like it's a &set::Stream. In particular, this requires passing ownership of the stream into this function, so I imagine you want something like let stream = unsafe { *Box::from_raw(ptr) };. Similarly for the function below.

I haven't audited this code thoroughly, but I might be concerned about memory leaks here. e.g., What's responsible for freeing the memory of an opbuilder?

https://github.com/jbaiter/python-rust-fst/blob/master/rust_fst/set.py#L60-L61 :

# NOTE: No need for `ffi.gc`, since the struct will be free'd # once we call union/intersection/difference

@jbaiter how do I wire up fst_set_opstreambuilder_free ?

Ah, right! Nice catch.

@BurntSushi I tried updating to use *mut &set::Stream, but the result didn't change:

Compiling fst-wrapper v0.3.0 (file:///home/dblewett/src/python-rust-fst/fstwrapper) error[E0277]: the trait bound `for<'a> &fst::set::Stream<'_>: fst::Streamer<'a>` is not satisfied --> src/set.rs:152:36 | 152 | let ob = set::OpBuilder::new().add(stream); | ^^^ the trait `for<'a> fst::Streamer<'a>` is not implemented for `&fst::set::Stream<'_>` | = help: the following implementations were found: <fst::set::Stream<'s, A> as fst::Streamer<'a>> error[E0277]: the trait bound `for<'a> &fst::set::Stream<'_>: fst::Streamer<'a>` is not satisfied --> src/set.rs:168:8 | 168 | ob.push(stream); | ^^^^ the trait `for<'a> fst::Streamer<'a>` is not implemented for `&fst::set::Stream<'_>` | = help: the following implementations were found: <fst::set::Stream<'s, A> as fst::Streamer<'a>> error: aborting due to 2 previous errors error: Could not compile `fst-wrapper`.

BurntSushi · 2018-01-18T01:53:01Z

fstwrapper/src/set.rs

 #[no_mangle]
 pub extern "C" fn fst_set_opbuilder_push(ptr: *mut set::OpBuilder, set_ptr: *mut Set) {
    let set = ref_from_ptr!(set_ptr);
    let ob = mutref_from_ptr!(ptr);
    ob.push(set);
 }

+#[no_mangle]
+pub extern "C" fn fst_set_opbuilder_push_stream(ptr: *mut set::OpBuilder, stream_ptr: *mut &set::Stream) {


This should be *mut set::Stream, not *mut &set::Stream.

davidblewett · 2018-01-19T03:11:44Z

@BurntSushi fstwrapper is now compiling; thanks!

@jbaiter However, now I'm getting segfaults in trying to test the functionality:

Python 3.6.3 (default, Oct  3 2017, 21:45:48)           
Type 'copyright', 'credits' or 'license' for more information                                                   
IPython 6.2.1 -- An enhanced Interactive Python. Type '?' for help.                                             

In [1]: import rust_fst                                 

In [2]: fst = rust_fst.Set('/home/dblewett/Desktop/bgzf-test/activity.fst')                                     

In [3]: fst.range_union(slice('1000~000003e8-0000-0054-01d3-8bdfe91ec2b7','1000~000003e8-0000-0054-01d3-8bdfe91ec2b7~~'), rust_fst.Set('/home/dblewett/Desktop/bgzf-test/index/nyt/2018/1/2/0/activity.fst'))                   
*** Error in `/home/dblewett/.virtualenvs/fst/bin/python3.6': free(): invalid pointer: 0x00007f88dd4f5cd8 ***   
zsh: abort (core dumped)  ipython

The API signature is a little awkward and should probably get some more thought, but I was just trying to test the plumbing without luck.

jbaiter · 2018-01-21T19:31:28Z

Can you add a unit test for the range_union method? Then I can take a shot at debugging it :-)
To fix the merge conflict, just move the new function signatures from _build_ffy.py to rust/rust_fst.h.

* Supports range queries across multiple sets: a = Set.from_iter(["bar", "foo"]) b = Set.from_iter(["baz", "foo"]) list(UnionSet(a, b)['ba':'bb']) ['bar', 'baz'] * Add StreamBuilder, to correspond with the Rust library's abstraction * Update OpBuilder to support constructing operations against multiple underlying types (Set and StreamBuilder for now)

* difference, intersection, symmetric_difference, union

davidblewett · 2018-09-12T18:45:54Z

@jbaiter @BurntSushi Sorry this got delayed so long. I have this working now.

davidblewett · 2018-09-12T19:47:37Z

The tests pass for me (cPython 3.7, Ubuntu 18.04.1):

(fst-test) 
(dblewett)Wed Sep 12, 15:46 | /home/dblewett/src
rusty% py.test python-rust-fst/tests
==================================================================================================================== test session starts ====================================================================================================================
platform linux -- Python 3.7.0b3, pytest-3.8.0, py-1.6.0, pluggy-0.7.1
rootdir: /home/dblewett/src/python-rust-fst, inifile:
collected 47 items                                                                                                                                                                                                                                          

python-rust-fst/tests/test_map.py ..................                                                                                                                                                                                                  [ 38%]
python-rust-fst/tests/test_set.py .............................                                                                                                                                                                                       [100%]

================================================================================================================= 47 passed in 0.18 seconds =================================================================================================================```

davidblewett · 2018-09-19T14:16:59Z

@jbaiter, @BurntSushi what do you think of this? I wasn't sure about the UnionSet name.

BurntSushi

From a quick skim, this all looks reasonable to me! Nice work!

davidblewett · 2018-10-31T19:08:51Z

@jbaiter is there interest in including this in this package? If not the entire PR, maybe the supporting C functions? it would be cumbersome to do completely outside this package.

davidblewett mentioned this pull request Jan 10, 2018

Efficient range operations over multiple Set objects #8

Open

BurntSushi reviewed Jan 18, 2018

View reviewed changes

davidblewett added 5 commits September 12, 2018 14:18

Add support for set operations with UnionSet:

358d5a8

* difference, intersection, symmetric_difference, union

Add support for Levenshtein fuzzy search to UnionSet.

c82a480

Add support for regex search to UnionSet.

e745f25

Update README with examples.

5b1a20e

davidblewett force-pushed the range-union branch from 4fe60f7 to 5b1a20e Compare September 12, 2018 18:44

Remove extraneous checks.

3488ff7

davidblewett force-pushed the range-union branch from 33e7553 to 3488ff7 Compare October 3, 2018 19:27

BurntSushi approved these changes Oct 6, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial attempt at efficient range queries across multiple Set objects. #10

Initial attempt at efficient range queries across multiple Set objects. #10

davidblewett commented Jan 10, 2018

BurntSushi Jan 18, 2018

davidblewett Jan 18, 2018 •

edited

Loading

BurntSushi Jan 18, 2018

davidblewett Jan 18, 2018

BurntSushi Jan 18, 2018

davidblewett commented Jan 19, 2018

jbaiter commented Jan 21, 2018

davidblewett commented Sep 12, 2018

davidblewett commented Sep 12, 2018

davidblewett commented Sep 19, 2018

BurntSushi left a comment

davidblewett commented Oct 31, 2018

Initial attempt at efficient range queries across multiple Set objects. #10

Are you sure you want to change the base?

Initial attempt at efficient range queries across multiple Set objects. #10

Conversation

davidblewett commented Jan 10, 2018

BurntSushi Jan 18, 2018

Choose a reason for hiding this comment

davidblewett Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

BurntSushi Jan 18, 2018

Choose a reason for hiding this comment

davidblewett Jan 18, 2018

Choose a reason for hiding this comment

BurntSushi Jan 18, 2018

Choose a reason for hiding this comment

davidblewett commented Jan 19, 2018

jbaiter commented Jan 21, 2018

davidblewett commented Sep 12, 2018

davidblewett commented Sep 12, 2018

davidblewett commented Sep 19, 2018

BurntSushi left a comment

Choose a reason for hiding this comment

davidblewett commented Oct 31, 2018

davidblewett Jan 18, 2018 •

edited

Loading