dissallow pushdown of volatile PhysicalExprs

[adriangb]

Closes #16545

[adriangb]

@theirix could you take a look? Are there any other expressions we should dissallow?

@alamb are there any existing APIs to get "volatility" from a PhysicalExpr? If not should we add some?

[theirix]

Thank you! I'll check my cases.

[theirix]

Thank you, @adriangb ! I can confirm that it works great with the table sampling, since I use random function (matched by name):

query TT
EXPLAIN SELECT COUNT(*) from t TABLESAMPLE 42 WHERE a < 10;
----
logical_plan
01)Projection: count(Int64(1)) AS count(*)
02)--Aggregate: groupBy=[[]], aggr=[[count(Int64(1))]]
03)----Projection:
04)------Filter: t.a < Int32(10) AND random() < Float64(0.42)
05)--------TableScan: t projection=[a]
physical_plan
01)ProjectionExec: expr=[count(Int64(1))@0 as count(*)]
02)--AggregateExec: mode=Final, gby=[], aggr=[count(Int64(1))]
03)----CoalescePartitionsExec
04)------AggregateExec: mode=Partial, gby=[], aggr=[count(Int64(1))]
05)--------RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=1
06)----------ProjectionExec: expr=[]
07)------------CoalesceBatchesExec: target_batch_size=8192
08)--------------FilterExec: a@0 < 10 AND random() < 0.42
09)----------------DataSourceExec: partitions=1, partition_sizes=[1]

The volatile filter is not pushed to the datasource. Without this patch, it looked like predicate=random() < 0.1.

I agree it'd be more scalable to have an abstract way to specify UDF volatility.

[alamb]

Thank you @adriangb and @theirix -- this looks good to me. I think we need to change the volatility check but otherwise this is good to go

It is probably reasonable to add a pub fn volatility() method to PhysicalExpr as well so that user defined expressions that are volatile / shouldn't be pushed down can be excluded as well

[alamb]

I think you can find volatility using

https://docs.rs/datafusion/latest/datafusion/logical_expr/trait.ScalarUDFImpl.html#tymethod.signature

Which then has a volatility field: https://docs.rs/datafusion/latest/datafusion/logical_expr/struct.Signature.html#structfield.volatility

[alamb]

All the book keeping for volatile functions got complicated (though I can't really see a better way to do it).

Maybe we could encapsulate the book keeping into some structure that would make it easier to track or something 🤔

[adriangb]

Yes agreed. I'll see what I can come up with...

[adriangb]

Claude and I came up with pydantic#34. It's not exactly simple or pretty, but it does put the indexing logic in one place and make it testable in isolation. Curious what you folks think.