Python: Add visitor to DNF expr into Dask/PyArrow format #6566
Conversation
Fokko
force-pushed
the
fd-format-into-dash-format
branch
from
0b0ba26 to
ff5fc81
Compare
Fokko
force-pushed
the
fd-format-into-dash-format
branch
from
ff5fc81 to
816cbe5
Compare
| return [(term.ref().field.name, "<=", literal.value)] | ||
|
|
||
| def visit_true(self) -> List[Tuple[str, str, Any]]: | ||
| return [] # Not supported |
There was a problem hiding this comment.
I think this is okay. Zero filters basically do the same thing as true. The problem is converting false into the same thing. I think for that, we should throw an exception because it cannot be safely handled.
There was a problem hiding this comment.
I'm okay with raising an error. Keep in mind that for Dask (and also PyArrow) this is only used to skip Parquet pages, we have to do a final pass to filter on a row level anyway:
(from the abovementioned blog)
There was a problem hiding this comment.
This also allows us to rewrite an expression to read PyArrow tables with filters: https://arrow.apache.org/docs/python/generated/pyarrow.parquet.read_table.html
Fokko
changed the title
Fokko
force-pushed
the
fd-format-into-dash-format
branch
from
cb97baa to
c7bbe18
Compare
| Formatter filter compatible with Dask and PyArrow | ||
| """ | ||
| # In the form of expr1 ∨ expr2 ∨ ... ∨ exprN | ||
| return [visit(expression, ExpressionToPlainFormat()) for expression in expressions] |
There was a problem hiding this comment.
Should this reuse the ExpressionToPlainFormat instance?
There was a problem hiding this comment.
Yes, good call! I've pulled this out of the loop
| return [(term.ref().field.name, "==", float("nan"))] | ||
|
|
||
| def visit_not_nan(self, term: BoundTerm[L]) -> List[Tuple[str, str, Any]]: | ||
| return [(term.ref().field.name, "!=", float("nan"))] |
There was a problem hiding this comment.
NaN is always not equal to itself, at least in Java. Are we sure that this works?
| return [(term.ref().field.name, "==", None)] | ||
|
|
||
| def visit_not_null(self, term: BoundTerm[L]) -> List[Tuple[str, str, Any]]: | ||
| return [(term.ref().field.name, "!=", None)] |
There was a problem hiding this comment.
I don't see anything in the docs that indicate this is the right way to pass this, so we should make sure there are tests for it.
There was a problem hiding this comment.
I agree, and I have #6398 lined up to exactly test this. I'll revive the PR tomorrow.
rdblue
left a comment
rdblue
left a comment
There was a problem hiding this comment.
Looks good, but is under tested until we get the follow up PR in.
2 participants
After reading https://www.coiled.io/blog/parquet-file-column-pruning-predicate-pushdown I noticed that we still need to filter everything.