tlfua commented on code in PR #7456:
URL: https://github.com/apache/datafusion-comet/pull/7456#discussion_r4497373377
##########
native/spark-expr/src/conditional_funcs/lazy_logical.rs:
##########
@@ -191,7 +193,11 @@ fn is_deterministic(expr: &Arc<dyn PhysicalExpr>) -> bool {
|| expr.is::<IfExpr>()
|| expr
.downcast_ref::<ScalarFunctionExpr>()
- .is_some_and(|f| f.fun().signature().volatility ==
Volatility::Immutable);
+ .is_some_and(|f| f.fun().signature().volatility ==
Volatility::Immutable)
+ // Its children are those of the function it wraps
+ || expr
+ .downcast_ref::<NullShortCircuit>()
+ .is_some_and(|n| is_deterministic(n.inner()));
Review Comment:
Good catch, thanks. `is_deterministic` now handles a `NullShortCircuit`
first and returns `is_deterministic(inner)`, without visiting the children
again, so the check is linear in the size of the tree.
`deeply_nested_short_circuits_are_checked_in_linear_time` in `lazy_logical.rs`
nests 64 wrappers, which would take 2^64 steps with the old code. Fixed in
9b6c7be04.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
