[SPARK-50241][SQL] Replace NullIntolerant Mixin with Expression.nullIntolerant method #48772

yaooqinn · 2024-11-06T08:30:31Z

What changes were proposed in this pull request?

Replace NullIntolerant Mixin with Expression.nullIntolerant method

Why are the changes needed?

This is not the first time that we are restricted by the trait-based tagging system. Extending a trait is static as it happens at compile but we really want to be more dynamic at runtime. For example, we added Expression#stateful and removed the trait StatefulExpression a while ago. I think the same thing happens to NullIntelerant now. Shall we also add Expression#nullIntolerant?

Does this PR introduce any user-facing change?

no

How was this patch tested?

Modified ExpressionInfoSuite,

Check whether SQL expressions should extend NullIntolerant

Was this patch authored or co-authored using generative AI tooling?

No

…ntolerant method

yaooqinn · 2024-11-06T11:40:47Z

cc @cloud-fan, thank you in advance

dongjoon-hyun

+1, LGTM. Thank you, @yaooqinn .

cloud-fan · 2024-11-06T15:29:54Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala

    isDeterministic: Boolean = true,
-    scalarFunction: Option[ScalarFunction[_]] = None) extends InvokeLike {
+    scalarFunction: Option[ScalarFunction[_]] = None,
+    override val nullIntolerant: Boolean = false) extends InvokeLike {


This flag is the same as propagateNull?

BTW I kind of like the name propagateNull more as it's more straightforward. But I'm not a native English speaker and I could be wrong. cc @dtenedor @srielau

I guess they are the same too. If you don't mind, I'd like too keep the PR in AS-IS state to minimize the potential influence of this refactoring work. Since nullIntolerant defaults to false，propagateNull defaults to true，mapping them blindly might result in many unsure plan changes，I‘d like to revist and revise them in followups, in the meanwhile I'm fixing the RuntimeReplaceables which use StaticInvoke and Invoke heavily？ WDYT？ @cloud-fan

OK, but at lease we shouldn't add a new parameter here. We can just add

override def nullIntolerant = propagateNull

nullIntolerant and propagateNull currently have opposite defaults in where they are defined and called, I guess we shall leave them AS-IS and change them more carefully

OK, but at lease we shouldn't add a new parameter here. We can just add

override def nullIntolerant = propagateNull

I have tested this change in a separate push, the actions did show some test failures to me. I suggest we do that change separately.

Does this mean propagateNull and nullIntolerant are not exactly the same? I'd like to figure this out now, instead of adding a new parameter to StaticInvoke (not binary compatible) and then remove it later.

I didn't mean that, I mean propagateNull lacks of some optimizations applied to the NullIntolerant trait

So it's not even related to Invoke?

cloud-fan · 2024-11-12T11:17:20Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala

+    case s: StaticInvoke
+        if s.staticObject == classOf[ByteArrayMethods] &&
+          Set("contains", "startsWith", "endsWith").contains(s.functionName) &&
+          s.arguments.length == 2 =>


why is this change?

cloud-fan · 2024-11-12T11:18:36Z

sql/core/src/test/scala/org/apache/spark/sql/expressions/ExpressionInfoSuite.scala

        val isEvalOverrode = clazz.getMethod("eval", classOf[InternalRow]) !=
          superClass.getMethod("eval", classOf[InternalRow])
-        val isNullIntolerantMixedIn = classOf[NullIntolerant].isAssignableFrom(clazz)
+        val isNullIntolerantMixedIn = clazz.getMethod("nullIntolerant") !=


does it mean overrideNullIntolerant?

cloud-fan · 2024-11-12T11:20:51Z

sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala

    checkAnswer(df5, Seq(Row("amy"), Row("cathy"), Row("alex"), Row("david"), Row("jen")))

    val df6 = sql("SELECT name FROM h2.test.employee WHERE " +
      "aes_decrypt(cast(null as binary), name) is null")


can we change the query to not use null input? We can use X'...' to write binary literal.

I leave it as-is because 1) we already have a similar one above and 2) the second parameter name get a chance to be validated while it's clearly not a valid input

Well, the second point makes me consider a potential breaking change: null intolerance takes higher precedence for error-raising in certain cases.

Does this mean aes_decrypt should not set propagateNull to true?

I haven't done a full check, but I believe some parameter checks may occur in the original expressions, while others might be checked in the replacement, which could result in different behaviors.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala

This reverts commit 6369f26.

yaooqinn · 2024-11-13T02:27:11Z

Merged to master. Thank you for the review @dongjoon-hyun @cloud-fan

[SPARK-50241][SQL] Replace NullIntolerant Mixin with Expression.nullI…

5536b4b

…ntolerant method

github-actions bot added the SQL label Nov 6, 2024

dongjoon-hyun approved these changes Nov 6, 2024

View reviewed changes

cloud-fan reviewed Nov 6, 2024

View reviewed changes

yaooqinn added 3 commits November 12, 2024 15:57

test propogate null and null intolerant

32b89ff

test propogate null and null intolerant

3410ced

fix tests

9d86ad1

cloud-fan reviewed Nov 12, 2024

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala Outdated Show resolved Hide resolved

cloud-fan approved these changes Nov 12, 2024

View reviewed changes

yaooqinn added 2 commits November 12, 2024 23:08

addr comments

556f521

timeout 240

6369f26

github-actions bot added the INFRA label Nov 12, 2024

Revert "timeout 240"

208cce3

This reverts commit 6369f26.

github-actions bot removed the INFRA label Nov 13, 2024

yaooqinn closed this in a84ca5e Nov 13, 2024

yaooqinn deleted the SPARK-50241 branch November 13, 2024 02:27

[SPARK-50241][SQL] Replace NullIntolerant Mixin with Expression.nullIntolerant method #48772

[SPARK-50241][SQL] Replace NullIntolerant Mixin with Expression.nullIntolerant method #48772

Uh oh!

Conversation

yaooqinn commented Nov 6, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

yaooqinn commented Nov 6, 2024

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaooqinn Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaooqinn Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yaooqinn commented Nov 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yaooqinn Nov 6, 2024 •

edited

Loading

yaooqinn Nov 7, 2024 •

edited

Loading