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

Adding facet filter and search #21

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

bloodbare
Copy link
Contributor

No description provided.

src/index.rs Outdated
@@ -305,6 +305,7 @@ impl Index {
&self,
query: &str,
default_field_names: Option<Vec<String>>,
filters: Option<&PyDict>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be part of the QueryParser?
It feels like the query a query with (+yourfillter +<parsed_query>) could be built outside of this function.

I agree an helper would be nice, but this can be in external to the queyr parser I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree that may not be needed, its easier as API and I had problems parsing fields that are not facets with a facets schema:

running this test:

    def test_and_query_parser_default_fields_undefined(self, ram_index):
        query = ram_index.parse_query("winter")

gives this tb:

tests/tantivy_test.py thread '<unnamed>' panicked at 'assertion failed: path.starts_with('/')', /Users/ramon/.cargo/registry/src/github.com-1ecc6299db9ec823/tantivy-0.12.0/src/schema/facet.rs:147:9
stack backtrace:
   0: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
   1: core::fmt::write
   2: std::io::Write::write_fmt
   3: std::panicking::default_hook::{{closure}}
   4: std::panicking::default_hook
   5: std::panicking::rust_panic_with_hook
   6: std::panicking::begin_panic
   7: <tantivy::schema::facet::Facet as core::convert::From<&T>>::from
   8: tantivy::schema::facet::Facet::from_text
   9: tantivy::query::query_parser::query_parser::QueryParser::compute_terms_for_string
  10: tantivy::query::query_parser::query_parser::QueryParser::compute_logical_ast_for_leaf
  11: tantivy::query::query_parser::query_parser::QueryParser::compute_logical_ast_from_leaf
  12: tantivy::query::query_parser::query_parser::QueryParser::compute_logical_ast_with_occur
  13: tantivy::query::query_parser::query_parser::QueryParser::compute_logical_ast
  14: tantivy::query::query_parser::query_parser::QueryParser::parse_query_to_logical_ast
  15: tantivy::query::query_parser::query_parser::QueryParser::parse_query
  16: tantivy::index::Index::parse_query
  17: tantivy::index::__init12805926066545533911::__init12805926066545533911::__wrap::{{closure}}
  18: tantivy::index::__init12805926066545533911::__init12805926066545533911::__wrap
  19: _PyMethodDef_RawFastCallKeywords
  20: _PyMethodDescr_FastCallKeywords
  21: call_function
  22: _PyEval_EvalFrameDefault
  23: _PyEval_EvalCodeWithName
  24: _PyFunction_FastCallDict
  25: _PyObject_Call_Prepend
  26: PyObject_Call
  27: _PyEval_EvalFrameDefault
  28: _PyEval_EvalCodeWithName
  29: _PyFunction_FastCallDict
  30: _PyEval_EvalFrameDefault
  31: _PyEval_EvalCodeWithName
  32: _PyFunction_FastCallKeywords
  33: call_function
  34: _PyEval_EvalFrameDefault
  35: _PyEval_EvalCodeWithName
  36: _PyFunction_FastCallKeywords
  37: call_function
  38: _PyEval_EvalFrameDefault
  39: function_code_fastcall
  40: call_function
  41: _PyEval_EvalFrameDefault
  42: _PyEval_EvalCodeWithName
  43: _PyFunction_FastCallDict
  44: _PyObject_Call_Prepend
  45: slot_tp_call
  46: _PyObject_FastCallKeywords
  47: call_function
  48: _PyEval_EvalFrameDefault
  49: function_code_fastcall
  50: call_function
  51: _PyEval_EvalFrameDefault
  52: function_code_fastcall
  53: _PyEval_EvalFrameDefault
  54: _PyEval_EvalCodeWithName
  55: _PyFunction_FastCallKeywords
  56: call_function
  57: _PyEval_EvalFrameDefault
  58: _PyEval_EvalCodeWithName
  59: _PyFunction_FastCallKeywords
  60: call_function
  61: _PyEval_EvalFrameDefault
  62: function_code_fastcall
  63: call_function
  64: _PyEval_EvalFrameDefault
  65: _PyEval_EvalCodeWithName
  66: _PyFunction_FastCallDict
  67: _PyObject_Call_Prepend
  68: slot_tp_call
  69: PyObject_Call
  70: _PyEval_EvalFrameDefault
  71: _PyEval_EvalCodeWithName
  72: _PyFunction_FastCallKeywords
  73: call_function
  74: _PyEval_EvalFrameDefault
  75: _PyEval_EvalCodeWithName
  76: _PyFunction_FastCallKeywords
  77: call_function
  78: _PyEval_EvalFrameDefault
  79: _PyEval_EvalCodeWithName
  80: _PyFunction_FastCallDict
  81: _PyEval_EvalFrameDefault
  82: _PyEval_EvalCodeWithName
  83: _PyFunction_FastCallKeywords
  84: call_function
  85: _PyEval_EvalFrameDefault
  86: _PyEval_EvalCodeWithName
  87: _PyFunction_FastCallKeywords
  88: call_function
  89: _PyEval_EvalFrameDefault
  90: function_code_fastcall
  91: _PyEval_EvalFrameDefault
  92: _PyEval_EvalCodeWithName
  93: _PyFunction_FastCallKeywords
  94: call_function
  95: _PyEval_EvalFrameDefault
  96: _PyEval_EvalCodeWithName
  97: _PyFunction_FastCallKeywords
  98: call_function
  99: _PyEval_EvalFrameDefault
 100: function_code_fastcall
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
fatal runtime error: failed to initiate panic, error 5
Fatal Python error: Aborted

Current thread 0x000000011193ddc0 (most recent call first):
  File "/Users/ramon/floss/tantivy-py/tests/tantivy_test.py", line 121 in test_and_query_parser_default_fields_undefined
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/_pytest/python.py", line 166 in pytest_pyfunc_call
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/callers.py", line 187 in _multicall
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/manager.py", line 87 in <lambda>
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/manager.py", line 93 in _hookexec
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/hooks.py", line 286 in __call__
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/_pytest/python.py", line 1435 in runtest
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/_pytest/runner.py", line 131 in pytest_runtest_call
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/callers.py", line 187 in _multicall
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/manager.py", line 87 in <lambda>
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/manager.py", line 93 in _hookexec
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/hooks.py", line 286 in __call__
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/_pytest/runner.py", line 207 in <lambda>
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/_pytest/runner.py", line 234 in from_call
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/_pytest/runner.py", line 207 in call_runtest_hook
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/_pytest/runner.py", line 182 in call_and_report
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/_pytest/runner.py", line 96 in runtestprotocol
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/_pytest/runner.py", line 81 in pytest_runtest_protocol
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/callers.py", line 187 in _multicall
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/manager.py", line 87 in <lambda>
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/manager.py", line 93 in _hookexec
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/hooks.py", line 286 in __call__
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/_pytest/main.py", line 270 in pytest_runtestloop
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/callers.py", line 187 in _multicall
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/manager.py", line 87 in <lambda>
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/manager.py", line 93 in _hookexec
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/hooks.py", line 286 in __call__
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/_pytest/main.py", line 246 in _main
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/_pytest/main.py", line 196 in wrap_session
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/_pytest/main.py", line 239 in pytest_cmdline_main
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/callers.py", line 187 in _multicall
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/manager.py", line 87 in <lambda>
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/manager.py", line 93 in _hookexec
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pluggy/hooks.py", line 286 in __call__
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/_pytest/config/__init__.py", line 92 in main
  File "/Users/ramon/.pyenv/versions/tantivy/lib/python3.7/site-packages/pytest/__main__.py", line 7 in <module>
  File "/Users/ramon/.pyenv/versions/3.7.7/lib/python3.7/runpy.py", line 85 in _run_code
  File "/Users/ramon/.pyenv/versions/3.7.7/lib/python3.7/runpy.py", line 193 in _run_module_as_main
[1]    27689 abort      RUST_BACKTRACE=1 pipenv run python -m pytest -s -k

Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

This looks mostly good, but if it allows people to crash the Python interpreter because of a panic in Tantivy it will probably have to wait for the panic to be gone from Tantivy before it can be merged.

src/searcher.rs Outdated Show resolved Hide resolved
tests/tantivy_test.py Outdated Show resolved Hide resolved
tests/tantivy_test.py Outdated Show resolved Hide resolved
src/searcher.rs Show resolved Hide resolved
@bloodbare
Copy link
Contributor Author

@poljar @fulmicoton Sorry I could not finish this PR, I was bootstraping getflaps.com and was super busy. Now I have some time and would like to get back to it as we are using with super successful result on our startup!

@poljar fixed all style errors

@fulmicoton About removing facets from search function and move it the query itself I don't understand how we can add a collector to count filters on the query parsed query

@@ -1,6 +1,6 @@
[package]
name = "tantivy"
version = "0.13.2"
version = "0.14.0"
Copy link
Contributor

@poljar poljar Apr 10, 2021

Choose a reason for hiding this comment

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

I think the tantivy upgrade should be a separate PR, or is there something this PR needs from 0.14?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using this branch on prod, I can do a new PR for the 0.14 if its better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please, separate PR for the 0.14 bump would be nice.

@@ -277,7 +277,7 @@ impl Index {
#[staticmethod]
fn exists(path: &str) -> PyResult<bool> {
let directory = MmapDirectory::open(path).map_err(to_pyerr)?;
Ok(tv::Index::exists(&directory))
Ok(tv::Index::exists(&directory).unwrap())
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should panic for IO errors.

@ZeroCool2u
Copy link

Any chance this PR might get merged soon? I was just trying to use the tantivy-py package and realized it's quite a bit behind the core Tantivy package, which I was using as a reference for documentation. I'm currently trying to do some of the work to bump the version to v0.15.3, but I'm very new to Rust, so it's quite a slog fo me. It seems like such a shame to just let all the good work in this PR go to waste :)

@poljar
Copy link
Contributor

poljar commented Jul 20, 2021

Any chance this PR might get merged soon? I was just trying to use the tantivy-py package and realized it's quite a bit behind the core Tantivy package, which I was using as a reference for documentation. I'm currently trying to do some of the work to bump the version to v0.15.3, but I'm very new to Rust, so it's quite a slog fo me. It seems like such a shame to just let all the good work in this PR go to waste :)

Sure, but the panic absolutely needs to disappear, I guess we can leave the version bump.

Sidhant29 pushed a commit to Sidhant29/tantivy-py that referenced this pull request Jan 16, 2023
…ions/alexellis/upload-assets-0.4.0

build(deps): bump alexellis/upload-assets from 0.2.2 to 0.4.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants