feat: make it possible to use rowid and rowaddr in filters #2973
Conversation
| batch_size: Optional[int] = None, | ||
| batch_readahead: Optional[int] = None, | ||
| fragment_readahead: Optional[int] = None, | ||
| scan_in_order: bool = True, | ||
| scan_in_order: bool = None, | ||
| fragments: Optional[Iterable[LanceFragment]] = None, | ||
| full_text_query: Optional[Union[str, dict]] = None, | ||
| *, | ||
| prefilter: bool = False, | ||
| with_row_id: bool = False, | ||
| with_row_address: bool = False, | ||
| use_stats: bool = True, | ||
| fast_search: bool = False, | ||
| prefilter: bool = None, | ||
| with_row_id: bool = None, | ||
| with_row_address: bool = None, | ||
| use_stats: bool = None, | ||
| fast_search: bool = None, |
There was a problem hiding this comment.
These changes in defaults should not be breaking changes since the defaults in ScannerBuilder match the defaults that used to be here.
By using None we can easily tell if the user is specifying a non-default value, in which case we will override whatever is in default_scan_options.
| /// the dataset schema (`dataset_schema`). This means that Substrait will | ||
| /// not be able to access columns that are not in the dataset schema (e.g. | ||
| /// _rowid, _rowaddr, etc.) | ||
| #[allow(unused)] |
There was a problem hiding this comment.
This was needed because dataset_schema is unused if the substrait feature is not specified.
| /// | ||
| /// The schema for this conversion should be the full schema available to | ||
| /// the filter (`full_schema`). However, due to a limitation in the way | ||
| /// we do Substrait conversion today we can only do Substrait conversion with |
There was a problem hiding this comment.
This lilmitation has been filed as a follow-up in #2972
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2973 +/- ##
=======================================
Coverage 78.77% 78.77%
=======================================
Files 237 237
Lines 74099 74143 +44
Branches 74099 74143 +44
=======================================
+ Hits 58374 58409 +35
- Misses 12703 12711 +8
- Partials 3022 3023 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| RT.block_on(async { scanner.filter_substrait(substrait).await })?; | ||
| RT.block_on(async { scanner.filter_substrait(substrait) })?; |
There was a problem hiding this comment.
🤔 Why shouldn't we await that future?
There was a problem hiding this comment.
The method is no longer async. Now we don't actually compile the filters until scan time (since the input schema may depend on other calls to the scanner builder). The scanner.filter and scanner.filter_substrait methods just record whatever the user passes in.
There was a problem hiding this comment.
If it's no longer async, you could also consider removing the wrapping RT.block_on. I think that would be a lot clearer.
| ds = lance.write_dataset(tab, str(tmp_path)) | ||
| ds = lance.dataset(str(tmp_path), default_scan_options={"with_row_id": True}) |
There was a problem hiding this comment.
Unimportant, but these should take paths as-is:
| ds = lance.write_dataset(tab, str(tmp_path)) | |
| ds = lance.dataset(str(tmp_path), default_scan_options={"with_row_id": True}) | |
| ds = lance.write_dataset(tab, tmp_path) | |
| ds = lance.dataset(tmp_path, default_scan_options={"with_row_id": True}) |
westonpace
force-pushed
the
feat/meta-cols-in-filter
branch
from
d231010
to
1a142da
Compare
This is particularly useful for operations like "delete by id"
I ran into a fair bit of difficulty with this PR and punted a few things to follow-ups (#2971 #2972).