[GLUTEN-9095][UT] Remove Vanilla Spark InternalRow based checkEvaluation by ArnavBalyan · Pull Request #9096 · apache/gluten

ArnavBalyan · 2025-03-21T14:22:19Z

What changes were proposed in this pull request?

GlutenCheckExpression currently falls back to internal row for certain complex/unsupported types.
As discussed in the PR, this is running Vanilla Spark and does not mean anything for Gluten.
Removing the same

How was this patch tested?

Unit Tests

github-actions · 2025-03-21T14:22:36Z

#9095

github-actions · 2025-03-21T14:22:51Z

Run Gluten Clickhouse CI on x86

ArnavBalyan · 2025-03-21T14:23:00Z

cc @jackylee-ch could you PTAL thanks

ArnavBalyan · 2025-03-24T05:08:06Z

cc @jackylee-ch @jinchengchenghh gentle reminder if you could please review thanks!

jackylee-ch · 2025-03-24T06:28:37Z

gluten-ut/common/src/test/scala/org/apache/spark/sql/GlutenTestsTrait.scala

-        checkEvaluationWithUnsafeProjection(expr, catalystValue, inputRow)
-      }
-      checkEvaluationWithOptimization(expr, catalystValue, inputRow)
+      checkEvaluationWithFallback(expression, expected, inputRow)


Why do we need this extra functions? They looks same as before.

The checkEvaluationWithFallback only calls checkEvaluationWithoutCodegen and checkEvaluationWithMutableProjection etc. Avoiding the glutenCheckExpression. Moved this logic to the new function to prevent code duplication.
With this change, a test can directly call checkEvaluationWithFallback and avoid the glutenCheckExpression when not supported. (Have done for the UT added)

Can you please review if all looks good thanks!

ArnavBalyan · 2025-03-25T06:46:17Z

cc @jackylee-ch, gentle reminder sorry for the ping thanks

jackylee-ch · 2025-03-25T08:20:48Z

gluten-ut/common/src/test/scala/org/apache/spark/sql/GlutenTestsTrait.scala

+    val resolver = ResolveTimeZone
+    val expr = resolver.resolveTimeZones(expression)
+    val catalystValue = CatalystTypeConverters.convertToCatalyst(expected)
+    checkEvaluationWithoutCodegen(expr, catalystValue, inputRow)


It seems these function are testing vaniila Cast, not related to gluten. Why we need it here?

This is the default flow for checkEvaluation in Gluten. For Array/Map other unsupported types where we can't convert to DF, currently all tests fallback to vanilla Spark's row based evaluation.
This refactor makes it directly accessable to the test, for cases when it can't be incorporated into canConvertToDataFrame

gentle reminder thanks!

I'm trying to reproduce the problem with checkExecution, but the local compilation environment has some problems. The tests added to gluten should be related to testing gluten trasformer. If a case is just for testing vanilla, there is really no need to add it.
cc @philo-he

Yes it seems this case runs the test on Vanilla Spark and the test may not mean anything for Gluten. I can remove it altogether, just wanted to confirm if there is a good reason behind adding this? @jackylee-ch @philo-he

For now, It seems useless. We may consider adding it later once we have a better reason.

github-actions · 2025-03-31T11:59:24Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-03-31T12:28:57Z

Run Gluten Clickhouse CI on x86

ArnavBalyan · 2025-04-01T06:40:34Z

cc @jackylee-ch removed the check as we discussed, could you please take a look. thanks!

ArnavBalyan · 2025-04-14T04:35:39Z

cc @jackylee-ch gentle reminder thanks!

jackylee-ch

Thanks.

jackylee-ch · 2025-04-15T01:57:43Z

gluten-ut/common/src/test/scala/org/apache/spark/sql/GlutenTestsTrait.scala


    if (canConvertToDataFrame(inputRow)) {
      glutenCheckExpression(expr, expected, inputRow)
-    } else {


Could you add a comment to "checkEvaluation" to indicate that the function only verifies the native result and skips the vanilla check.

Maybe, it would be better to add a warning log to indicate the evaluation is skipped.

Added the warn

philo-he · 2025-04-16T07:13:55Z

gluten-ut/common/src/test/scala/org/apache/spark/sql/GlutenTestsTrait.scala

@@ -136,15 +135,6 @@ trait GlutenTestsTrait extends GlutenTestsCommonTrait {

    if (canConvertToDataFrame(inputRow)) {


Can we move the above expression resolving code into the if branch?

philo-he · 2025-04-16T07:18:33Z

This patch may also have an impact on CH backend test. @zzcclp, could you also take a look?

github-actions · 2025-04-16T11:55:11Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-04-16T12:22:04Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-04-17T07:25:03Z

Run Gluten Clickhouse CI on x86

philo-he · 2025-04-17T07:44:47Z

gluten-ut/common/src/test/scala/org/apache/spark/sql/GlutenTestsTrait.scala

-      }
-      checkEvaluationWithOptimization(expr, catalystValue, inputRow)
+      logWarning(
+        "Skipping evaluation - Expression could not be converted " +


Suggestion:

Skipping evaluation - Nonempty inputRow cannot be converted to DataFrame due to **

github-actions · 2025-04-17T13:08:31Z

Run Gluten Clickhouse CI on x86

philo-he

Thanks!

github-actions bot added the CORE works for Gluten Core label Mar 21, 2025

jackylee-ch reviewed Mar 24, 2025

View reviewed changes

jackylee-ch reviewed Mar 25, 2025

View reviewed changes

ArnavBalyan force-pushed the arnavb/fix-testing-cast1 branch from 2cbf133 to 5674dc0 Compare March 31, 2025 11:58

ArnavBalyan changed the title ~~[GLUTEN-9095][VL] Allow CheckEvaluation tests use InternalRow based checkEvaluation~~ [GLUTEN-9095][VL] Remove Vanilla Spark InternalRow based checkEvaluation Mar 31, 2025

ArnavBalyan force-pushed the arnavb/fix-testing-cast1 branch from 5674dc0 to 0c130cd Compare March 31, 2025 12:28

jackylee-ch approved these changes Apr 14, 2025

View reviewed changes

jackylee-ch reviewed Apr 15, 2025

View reviewed changes

philo-he reviewed Apr 16, 2025

View reviewed changes

philo-he changed the title ~~[GLUTEN-9095][VL] Remove Vanilla Spark InternalRow based checkEvaluation~~ [GLUTEN-9095] Remove Vanilla Spark InternalRow based checkEvaluation Apr 16, 2025

philo-he changed the title ~~[GLUTEN-9095] Remove Vanilla Spark InternalRow based checkEvaluation~~ [GLUTEN-9095][UT] Remove Vanilla Spark InternalRow based checkEvaluation Apr 16, 2025

ArnavBalyan force-pushed the arnavb/fix-testing-cast1 branch from 0c130cd to 48f420f Compare April 16, 2025 11:54

ArnavBalyan force-pushed the arnavb/fix-testing-cast1 branch from 48f420f to 19f90d1 Compare April 16, 2025 12:20

ArnavBalyan force-pushed the arnavb/fix-testing-cast1 branch from 19f90d1 to 4ae8847 Compare April 17, 2025 07:24

philo-he reviewed Apr 17, 2025

View reviewed changes

update

171165f

ArnavBalyan force-pushed the arnavb/fix-testing-cast1 branch from 4ae8847 to 171165f Compare April 17, 2025 13:07

philo-he approved these changes Apr 17, 2025

View reviewed changes

philo-he merged commit f0bdaec into apache:main Apr 18, 2025
47 checks passed

		@@ -136,15 +135,6 @@ trait GlutenTestsTrait extends GlutenTestsCommonTrait {

		if (canConvertToDataFrame(inputRow)) {

Conversation

ArnavBalyan commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

github-actions bot commented Mar 21, 2025

Uh oh!

github-actions bot commented Mar 21, 2025

Uh oh!

ArnavBalyan commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArnavBalyan commented Mar 24, 2025

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!

ArnavBalyan commented Mar 25, 2025

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!

ArnavBalyan Mar 26, 2025 • 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!

github-actions bot commented Mar 31, 2025

Uh oh!

github-actions bot commented Mar 31, 2025

Uh oh!

ArnavBalyan commented Apr 1, 2025

Uh oh!

ArnavBalyan commented Apr 14, 2025

Uh oh!

jackylee-ch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philo-he Apr 16, 2025 • 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!

philo-he commented Apr 16, 2025

Uh oh!

github-actions bot commented Apr 16, 2025

Uh oh!

github-actions bot commented Apr 16, 2025

Uh oh!

github-actions bot commented Apr 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 17, 2025

Uh oh!

philo-he left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

ArnavBalyan commented Mar 21, 2025 •

edited

Loading

ArnavBalyan commented Mar 21, 2025 •

edited

Loading

ArnavBalyan Mar 26, 2025 •

edited

Loading

philo-he Apr 16, 2025 •

edited

Loading