Flyangz commented on PR #2370: URL: https://github.com/apache/auron/pull/2370#issuecomment-4873124798
> Nice fix! One small test coverage thought: maybe consider adding a similar regression test with `SCOrExpr(id = 1, id = age)`. The implementation updates both `BinaryExpr::Or` and `SCOrExpr`, while the new mixed convertible/unconvertible OR tests only cover the `BinaryExpr::new(..., Operator::Or, ...)` path. > > While reviewing this, I also noticed a related existing case around `NOT`: `NOT(id = 5 AND unsupported_expr)` seems like it could become `NOT(id = 5)` if the inner `AND` is partiallyconverted. That would be narrower than the original filter and could prune matching rows. This seems to be an existing issue, but it may be worth a follow-up issue/PR. Thanks for the review! I have added the corresponding unit test for SCOrExpr as you suggested. Regarding your second point about the NOT issue, I agree that it should be tracked and fixed in a follow-up issue/PR. That being said, in a typical production environment, the `BooleanSimplification` rule usually optimizes this away, so the practical impact is likely limited. -- 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]
