jiwen624 commented on PR #56860: URL: https://github.com/apache/spark/pull/56860#issuecomment-4862910970
> ### **LGTM with non-blocking suggestions** > Clean and well-scoped, and the right approach over #56836 - it keeps the safe same-collation inference that #56836 drops while blocking the cross-collation substitution. > > One question: `sameCollationOperand` checks only that both operands' dataTypes equal the source's, which is looser than the `ConstantPropagation` guard it cites (that one also requires the operands be `AttributeReference`/`CollationKey`-wrapped). Looks sufficient for safety, so not a bug - deliberate, or worth aligning with the stricter form? > > Two non-blocking test suggestions: > > * The `EqualTo(Cast(attr), attr)` branches in `inferAdditionalConstraints` aren't covered under a collation - the added tests only hit `Attribute = Attribute`. > * A direct assertion in `ConstraintPropagationSuite` that the unsafe constraint is _not_ inferred would lock the negative case at the layer being changed, rather than via end-to-end row counts. > > What I verified: > > * The binary-stable branch is byte-identical to the original - no regression for non-collated columns. > * The motivating case is fixed - the `COLLATE UTF8_BINARY` operand is a `Cast` whose dataType is not the source's, so it isn't substituted. > * Over-conservative passthroughs are dropped by `inferredConstraints -- constraints`. Thanks for the review! For the question of guard different from ConstantPropagation - yes it's intentional. If you adopted the stricter attribute-only shape, you'd reject cases like `source_lcase = 'hello' COLLATE UTF8_LCASE`, which is a legit inference. -- 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]
