cloud-fan commented on code in PR #56499:
URL: https://github.com/apache/spark/pull/56499#discussion_r3454224176
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/ConstraintPropagationSuite.scala:
##########
@@ -427,4 +428,115 @@ class ConstraintPropagationSuite extends PlanTest {
assert(aliasedRelation.analyze.constraints.isEmpty)
}
}
+
+ test("infer range constraints by substituting attr=literal bindings") {
+ // When a.pt = '20260610' (modeled as a.k = 5) and b.v >= a.k are both
present,
+ // inferAdditionalConstraints should emit b.v >= 5.
+ val tr = LocalRelation($"k".int, $"v".int)
+ val a = resolveColumn(tr, "k")
+ val b = resolveColumn(tr, "v")
+
+ val constraints = ExpressionSet(Seq(
+ EqualTo(a, Literal(5)),
+ GreaterThanOrEqual(b, a),
+ LessThanOrEqual(b, Add(a, Literal(10)))))
+
+ val helper = new ConstraintHelper {}
+ val inferred = helper.inferConstraintsFromLiteralBindings(constraints)
+
+ // b >= 5 must be inferred
+ assert(inferred.exists {
+ case GreaterThanOrEqual(attr: Attribute, Literal(v, IntegerType)) =>
+ attr.semanticEquals(b) && v == 5
+ case _ => false
+ }, s"Expected b >= 5 in inferred constraints; got: $inferred")
+
+ // b <= 15 must be inferred (a + 10 with a=5, i.e. Add(Literal(5),
Literal(10)))
+ assert(inferred.exists {
+ case LessThanOrEqual(attr: Attribute, rhs) if attr.semanticEquals(b) =>
+ // rhs is either Literal(15) or Add(Literal(5), Literal(10)) - both
are foldable
+ rhs.foldable && rhs.eval(null) == 15
+ case _ => false
+ }, s"Expected b <= 15 in inferred constraints; got: $inferred")
+ }
+
+ test("infer range constraints: literal on left side of EqualTo") {
+ val tr = LocalRelation($"k".int, $"v".int)
+ val a = resolveColumn(tr, "k")
+ val b = resolveColumn(tr, "v")
+
+ // Literal appears on the left: 5 = a.k
+ val constraints = ExpressionSet(Seq(
+ EqualTo(Literal(5), a),
+ GreaterThanOrEqual(b, a)))
+
+ val helper = new ConstraintHelper {}
+ val inferred = helper.inferConstraintsFromLiteralBindings(constraints)
+
+ assert(inferred.exists {
+ case GreaterThanOrEqual(attr: Attribute, Literal(v, IntegerType)) =>
+ attr.semanticEquals(b) && v == 5
+ case _ => false
+ }, s"Expected b >= 5 in inferred constraints; got: $inferred")
+ }
+
+ test("infer constraints by substituting attr=literal bindings into EqualTo
expressions") {
+ val tr = LocalRelation($"k".int, $"v".int)
+ val a = resolveColumn(tr, "k")
+ val b = resolveColumn(tr, "v")
+
+ val constraints = ExpressionSet(Seq(
+ EqualTo(a, Literal(5)),
+ EqualTo(b, Add(a, Literal(1)))))
+
+ val helper = new ConstraintHelper {}
+ val inferred = helper.inferConstraintsFromLiteralBindings(constraints)
+
+ // b = a + 1 with a = 5 should infer b = 5 + 1 (foldable to 6)
+ assert(inferred.exists {
+ case EqualTo(attr: Attribute, rhs) if attr.semanticEquals(b) =>
+ rhs.foldable && rhs.eval(null) == 6
+ case _ => false
+ }, s"Expected b = 6 in inferred constraints; got: $inferred")
+ }
+
+ test("non-deterministic constraints inferred from literal substitution are
filtered out") {
+ val tr = LocalRelation($"k".int, $"v".int)
+ val a = resolveColumn(tr, "k")
+ val b = resolveColumn(tr, "v")
+
+ // b >= a.k + Rand(); after substituting a.k=5 the result is
non-deterministic.
+ // inferAdditionalConstraints may emit it, but the plan-level constraints
filter must drop it.
+ val randExpr = Add(a, Cast(Rand(0L), IntegerType))
+ val condition = EqualTo(a, Literal(5)) && GreaterThanOrEqual(b, randExpr)
+ val plan = Filter(condition, tr.analyze)
+
+ assert(!plan.analyze.constraints.exists {
+ case GreaterThanOrEqual(attr: Attribute, _) => attr.semanticEquals(b)
+ case _ => false
+ }, s"Should not keep non-deterministic constraint in plan constraints; " +
+ s"got: ${plan.analyze.constraints}")
+ }
+
+ test("do not infer constraints by substituting non-binary-stable collated
attributes") {
+ // a is UTF8_LCASE (case-insensitive). a = 'hello' only tells us a is
case-insensitively
+ // equal to 'hello'; it could still be 'HELLO'. Substituting a with
'hello' into a
+ // UTF8_BINARY comparison like b >= a would infer b >= 'hello', which is
wrong.
+ val a = AttributeReference("a",
StringType(CollationFactory.UTF8_LCASE_COLLATION_ID))()
+ val b = AttributeReference("b", StringType)()
+ val hello = Literal("hello")
+
+ val constraints = ExpressionSet(Seq(
+ EqualTo(a, hello),
+ GreaterThanOrEqual(b, a)))
+
+ val helper = new ConstraintHelper {}
+ val inferred = helper.inferAdditionalConstraints(constraints)
Review Comment:
This collation regression test is now vacuous. Literal substitution moved
out of `inferAdditionalConstraints` into `inferConstraintsFromLiteralBindings`,
so `inferAdditionalConstraints` no longer has any `EqualTo(attr, Literal)` case
— for `{EqualTo(a, 'hello'), GreaterThanOrEqual(b, a)}` it matches nothing and
returns empty. The assertion `!inferred.exists { b >= 'hello' }` therefore
passes trivially, and would still pass even if the `isBinaryStable` guard were
deleted from `inferConstraintsFromLiteralBindings` — so it no longer protects
the collation fix it's named for.
The three sibling tests (the range / literal-on-left / EqualTo-expression
cases) correctly call `inferConstraintsFromLiteralBindings`; this one looks
like it was missed in the rename. The production guard itself is correct, so
this is a test-only fix:
```suggestion
val inferred = helper.inferConstraintsFromLiteralBindings(constraints)
```
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/ConstraintPropagationSuite.scala:
##########
@@ -427,4 +428,115 @@ class ConstraintPropagationSuite extends PlanTest {
assert(aliasedRelation.analyze.constraints.isEmpty)
}
}
+
+ test("infer range constraints by substituting attr=literal bindings") {
+ // When a.pt = '20260610' (modeled as a.k = 5) and b.v >= a.k are both
present,
+ // inferAdditionalConstraints should emit b.v >= 5.
Review Comment:
Stale method name in the comment — this test (and the one at line 509)
describe `inferAdditionalConstraints`, but the substitution they exercise is
now done by `inferConstraintsFromLiteralBindings` (this test body even calls
it). Worth updating both comments to name the new method.
--
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]
