Skip to content

[FEA]: Avoid stream synchronization in ConditinalJoin.Predicate.__init__ #20372

@TomAugspurger

Description

@TomAugspurger

Is your feature request related to a problem? Please describe.

During #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.

Metadata

Metadata

Assignees

Labels

cudf-polarsIssues specific to cudf-polarsfeature requestNew feature or request

Type

No type

Projects

Status

In Progress

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions