andygrove commented on PR #5471: URL: https://github.com/apache/datafusion-comet/pull/5471#issuecomment-5607717903
Rebase checked. Against `95c744c4c`, the head I approved, the only differences are two import lines that moved with `main`, so the change itself is unaltered and my approval stands. On the question I left open last time, about `CometScalaUDF.emitJvmCodegenDispatch` calling `withCodegenDispatchExpr` without the `!isStructuralExpr` guard that `exprToProtoInternal` applies to `withNativeExpr`: rereading it with the new comments, that condition is not load-bearing any more, and this PR is what makes it not load-bearing. `collectExprTagValues` filters on the *node* through `isNeverTagged`, so a coverage tag that reached a `Literal`, `Attribute` or `BoundReference` by any route, including a direct write at the dispatch site, is dropped on read rather than depending on nobody writing it. That is a better answer than the comment I asked for, and the two comments you added say the right thing about which set has to track which. Worth noting explicitly that `FALLBACK_REASONS` is excluded from the invariant in both places. One coordination note, since it is my own PR and therefore my problem rather than yours. #5610 also touches expression coverage: it records the names of a dispatched subtree's descendants on the dispatched root's `CODEGEN_DISPATCH_EXPRS` tag. I checked the interaction and it is clean in both merge orders. Your filter operates on nodes and mine writes names onto a non-structural root, and mine already excludes attribute references and literals from the names it records, so neither one can undo the other. I will rebase #5610 behind this rather than the other way round. -- 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]
