[SPARK-46331][SQL] Removing CodegenFallback from subset of DateTime expressions and version() expression by dbatomic · Pull Request #44261 · apache/spark

dbatomic · 2023-12-08T12:18:59Z

What changes were proposed in this pull request?

This PR moves us a bit closer to removing CodegenFallback class and instead of it relying on RuntimeReplaceable with StaticInvoke.

In this PR there are following changes:

Doing StaticInvoke + RuntimeReplaceable against spark version expression.
Adding Unevaluable trait for DateTime expressions. These expressions need to be replaced during analysis anyhow so we explicitly forbid eval from being called.

Why are the changes needed?

Direction is to get away from CodegenFallback. This PR moves us closer to that destination.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Running existing tests.

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

No

MaxGekk · 2023-12-08T15:11:02Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala

@@ -79,30 +79,22 @@ class DateExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
  }

  test("datetime function current_date") {


This test and below are supposed to be positive tests. Do we have positive tests for the functions current_date(), current_timestamp(), localtimestamp() in Spark code base?

Actually these tests were not useful at all previously, as they are testing dead code. We should check ComputeCurrentTimeSuite and make sure it has good test coverage for the execution part.

dbatomic · 2023-12-12T11:56:53Z

After looking at test failures, there seem to be 'minor' correctness bug for time aware expressions that rely on ReplaceCurrentLike rule. For following expression:

select b from values (
 'one', current_timestamp),
 'two', current_timestamp),
 'three', current_timestamp as data(a, b)

current_timestamp will not be replaced. Instead, it will be evaluated every time which means that these three rows will have different values (tested locally). This happens because ResolveInlineTables rule happens first and it tries to eval all the expressions. IMO, ReplaceCurrentLike should have higher precedence.
I will try to fix this as part of this PR.

@cloud-fan and @MaxGekk as FYI.

MaxGekk · 2023-12-12T12:33:49Z

I will try to fix this as part of this PR.

@dbatomic Could you open a separate PR with a bug fix. In this way, we can backport it without your changes in this PR.

dbatomic · 2023-12-12T13:57:45Z

I will try to fix this as part of this PR.

@dbatomic Could you open a separate PR with a bug fix. In this way, we can backport it without your changes in this PR.

I just created this PR:
#44316

dtenedor

In general this approach looks good, I just have a couple comments.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveInlineTables.scala

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveInlineTables.scala

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

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/EliminateSortsSuite.scala

...talyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/FoldablePropagationSuite.scala

cloud-fan · 2023-12-28T12:59:19Z

LGTM except for #44261 (comment) . Can we verify the test coverage for current datetime functions in ComputeCurrentTimeSuite?

dbatomic · 2023-12-29T13:03:42Z

LGTM except for #44261 (comment) . Can we verify the test coverage for current datetime functions in ComputeCurrentTimeSuite?

Hopefully everything looks fine now.

cloud-fan · 2023-12-29T13:34:26Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala

            val t2 = UnixTimestamp(
-              CurrentTimestamp(), Literal("yyyy-MM-dd HH:mm:ss")).eval().asInstanceOf[Long]
+              CurrentTimestamp, Literal("yyyy-MM-dd HH:mm:ss")).eval().asInstanceOf[Long]
            assert(t2 - t1 <= 1)


This doesn't test anything now as we are testing the same CurrentTimestamp instance. Shall we move this to ComputeCurrentTimeSuite?

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala

beliefer

LGTM except one comment and @cloud-fan 's comment.

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala

cloud-fan · 2024-01-08T09:14:24Z

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/ComputeCurrentTimeSuite.scala

+    val t1 = DateTimeUtils.microsToMillis(
+      literals[Long](planT1)(0))
+
+    val inT2 = Project(Alias(CurrentTimestamp(), "t1")() :: Nil, LocalRelation())


do we need inT2 as it's exactly the same as inT1? Seems we can just do

val planT2 = Optimize.execute(inT1.analyze).asInstanceOf[Project]

cloud-fan · 2024-01-08T09:14:58Z

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/ComputeCurrentTimeSuite.scala

    checkLiterals({ zoneId: String => CurrentDate(Some(zoneId)) }, numTimezones)
  }

+  test("datetime function CurrentDate and LocalTime are Unevaluable") {


This does not test the optimizer rule and we should keep it in DateExpressionsSuite

cloud-fan · 2024-01-09T11:19:20Z

thanks, merging to master!

…ime functions ### What changes were proposed in this pull request? This is a followup of #44261 to fix one regression: we missed one exception that time travel needs to evaluate current datetime functions before the analysis phase completes. This PR fixes it by explicitly compute current datetime values in the time travel handling. ### Why are the changes needed? re-support current datetime functions as time travel timestamp ### Does this PR introduce _any_ user-facing change? no, the regression has not been released yet. ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? no Closes #45476 from cloud-fan/time-travel. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

#### Which Delta project/connector is this regarding?  - [x] Spark - [ ] Standalone - [ ] Flink - [ ] Kernel - [ ] Other (fill in here) ## Description  Heading towards the removal of codegenFallback from Date/Time expressions (apache/spark#44261), delta constraints need to resolve current_datetime expressions during the analysis of the invariants. The proposed changes work for both Spark 3.5 and Spark master. ## How was this patch tested?  Existing tests. In particular, `CheckConstaintsSuite` covers constraints with `current_timestamp()` expressions. Added an extra test to cover `current_date()`. ## Does this PR introduce _any_ user-facing changes?  No.

### What changes were proposed in this pull request? This PR is a followed of [this](#44261) PR that made "compute current time" family of expressions `Unevaluable`. Implicitly, all `Unevaluable` expressions are also not foldable. Hence, we have a regression comparing to state prior to 44261 where "compute current time" expressions could have been used in places where constant folding is required. Proposed change is to keep these expression `Unevaluable` in a sense that `eval`/`codeGen` can't be called but to allow folding. This is a special case given that these expressions are supposed to be replaced by QO with literals (that are foldable) so proposal is to create new interface for this family of expressions. ### Why are the changes needed? Explained above. ### Does this PR introduce _any_ user-facing change? Yes. ### How was this patch tested? Additional test is added that ensures that `CurrentDate` can be used in places that require folding. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46120 from dbatomic/constantfoldingforcurrenttime. Lead-authored-by: Aleksandar Tomic <aleksandar.tomic@databricks.com> Co-authored-by: Aleksandar Tomic <150942779+dbatomic@users.noreply.github.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

Fix GlutenDateExpressionsSuite in Spark-4.0 apache/spark#44261 make CurrentTimestamp as Unevaluable.

dbatomic added 6 commits December 4, 2023 18:13

initial prototype for SparkVersion

7d70567

Working Current time replacement and current timestamp

1c05987

Making date time expressions unevaluable

11d1094

Cleanup of spark version change

2550884

removing UnresolvedReplacement

caf803f

cleanup

795f9f6

github-actions bot added the SQL label Dec 8, 2023

dbatomic added 2 commits December 8, 2023 14:12

removing bad formatting

2539d53

merging from master

b5b2216

MaxGekk reviewed Dec 8, 2023

View reviewed changes

Merge branch 'apache:master' into codegenfallback_removal

d4f3c7f

Making sure that time resolution is done prior to inline table eval

fcefc63

dtenedor reviewed Dec 14, 2023

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveInlineTables.scala Outdated Show resolved Hide resolved

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala Outdated Show resolved Hide resolved

dbatomic added 7 commits December 21, 2023 18:55

merge

9a65253

Removing function that is already covered

7267583

Minor update in validateInputEvaluable.

6fda2aa

Fixing a subset of failing tests.

7cc513c

Fixing ResolveTableSpec failing test.

8b86c97

updating method name.

298adc3

updating method name.

673e2fc