[FEA]: Avoid stream synchronization in `ConditinalJoin.Predicate.__init__`
- Dominant language
- C++
- Stars
- 9.8k
- Forks
- 1.1k
- Avg merge
- 3d 6m
- Merged PRs (30d)
- 278
Description
**Is your feature request related to a problem? Please describe.**
During https://github.com/rapidsai/cudf/pull/20291, we ran into issues with `to_ast`. This does two things:
1. Validates whether expression is supported, in the sense that we're able to translate the expression (e.g. there are some binary ops that aren't supported by parquet filter). When we encounter an unsupported expression we raise a `NotImplementedError` and (optionally) fall back. This *must* happen at IR translation time.
2. Does the actual translation, i.e. makes the `pylibcudf.Expression`. *Some* of the translations require (or can require) a CUDA stream because they make a `plc.Scalar`.
So we have a conflict. Validation needs to occur at translation time, when we don't have a scalar at hand. Our workaround in that PR is to just create a new stream, do the operation, and synchronize it, all in `Predicate.__init__`.
A natural question to ask is "can we split the validation from the translation". I attempted that, but failed. I think it's still work exploring, so I'll outline my attempt here:
1. Define a new `validate_to_ast` transformer, similar to `to_ast`
2. Move the pieces of the `_to_ast` implementations that `raise NotImplementedError` to this new `validate_to_ast`
3. Call `validate_to_ast` in `Predict.__init__`
4. Store `predicate` in `Predict.__init__` instead of calling `to_ast`
5. Make a new `Predict.to_ast` method that calls `to_ast` to do the actual translation
6. Call `Predict.to_ast` inside of `ConditionalJoin.do_evaluate`, where we have a stream ready to go.
Some of the tests in `tests/test_parquet_filters.py::test_scan_by_hand` were failing with this setup. I think it's worth working through these to avoid that `stream.synchronize()`.
**Describe the solution you'd like**
Similar functionality, with no stream synchronize.
**Describe alternatives you've considered**
I also wondered whether we could define something like an s-expression, where you delay creating the plc.Scalar` until a time when you have a stream. Might work, but it gets a bit messy.
**Additional context**
Add any other context, code examples, or references to existing implementations about the feature request here.
Contributor guide
Assessment
This issue has not been assessed yet.