Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-51420][SQL][TESTS][FOLLOWUP] Re-generate sql-expression-schema.md #50361

Closed
wants to merge 1 commit into from
Closed

[SPARK-51420][SQL][TESTS][FOLLOWUP] Re-generate sql-expression-schema.md #50361

wants to merge 1 commit into from

Conversation

MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Mar 24, 2025

What changes were proposed in this pull request?

In the PR, I propose to regenerate sql-expression-schema.md via:

$ SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "test:testOnly *ExpressionsSchemaSuite"

Why are the changes needed?

To have sql-expression-schema.md up-to-date, and avoid unrelated changes in other PRs.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

By running the related test:

$ build/sbt "test:testOnly *ExpressionsSchemaSuite"

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

No.

@github-actions github-actions bot added the SQL label Mar 24, 2025
@MaxGekk
Copy link
Member Author

MaxGekk commented Mar 24, 2025

@HyukjinKwon @the-sakthi Please, have a look at the PR.

@the-sakthi
Copy link
Member

the-sakthi commented Mar 24, 2025 •
edited
Loading

LGTM and apologies for missing this in the original PR.

beliefer
beliefer approved these changes Mar 24, 2025
View reviewed changes
Copy link
Contributor

@beliefer beliefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
But I want know why the test cases not failure ?

@LuciferYang
Copy link
Contributor

LuciferYang commented Mar 24, 2025 •
edited
Loading

LGTM.
But I want know why the test cases not failure ?

a good question

@MaxGekk
Copy link
Member Author

MaxGekk commented Mar 24, 2025 •
edited
Loading

LGTM.
But I want know why the test cases not failure ?
a good question

Because we don't compare class names but only sql and schema:
sql = SELECT minute('2009-07-30 12:58:59')
schema = struct<minute(2009-07-30 12:58:59):int>
where both are the same in our case, see

spark/sql/core/src/test/scala/org/apache/spark/sql/ExpressionsSchemaSuite.scala

Lines 189 to 192 in dc76834

outputs.zip(expectedOutputs).foreach { case (output, expected) =>
assert(expected.sql == output.sql, "SQL query did not match")
assert(expected.schema == output.schema, s"Schema did not match for query ${expected.sql}")
}

@MaxGekk
Copy link
Member Author

MaxGekk commented Mar 24, 2025

Merging to master. Thank you, @HyukjinKwon @yaooqinn @beliefer @LuciferYang for review.

@MaxGekk MaxGekk closed this in b229044 Mar 24, 2025
@MaxGekk MaxGekk mentioned this pull request Mar 24, 2025
Open
@beliefer
Copy link
Contributor

@MaxGekk Is anyone fix it? I will fix it if on one.

@beliefer beliefer mentioned this pull request Mar 25, 2025
Closed
HyukjinKwon pushed a commit that referenced this pull request Mar 25, 2025
@beliefer @HyukjinKwon
[SPARK-51598][SQL][TEST] Improve the ExpressionsSchemaSuite
0c99d96 
### What changes were proposed in this pull request?
This PR aims to improve the `ExpressionsSchemaSuite`.

### Why are the changes needed?
As time passes, we can register the expression builders into `FunctionRegistry`, such as: `MinuteExpressionBuilder`.
The change causes `ExpressionsSchemaSuite` works not well due to it just compares the schema and sql. We should add the support to compare expression name.

In addition to, I fix some english grammar errors and make them more clearly.

### Does this PR introduce _any_ user-facing change?
'No'.
Just update tests.

### How was this patch tested?
GA and manual tests.
Without #50361, developers could see the error show below.
```
[info] ExpressionsSchemaSuite:
10:51:51.174 WARN org.apache.hadoop.util.NativeCodeLoader: Unable to load native-hadoop library for your platform... using builtin-java classes where applicable
[info] - Check schemas for expression examples *** FAILED *** (1 second, 650 milliseconds)
[info]   "...t.expressions.Minute[]" did not equal "...t.expressions.Minute[ExpressionBuilder]" The expected class name org.apache.spark.sql.catalyst.expressions.Minute does not match the output class name org.apache.spark.sql.catalyst.expressions.MinuteExpressionBuilder (ExpressionsSchemaSuite.scala:190)
[info]   Analysis:
[info]   "...t.expressions.Minute[]" -> "...t.expressions.Minute[ExpressionBuilder]"
[info]   org.scalatest.exceptions.TestFailedException:
[info]   at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:472)
[info]   at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:471)
[info]   at org.scalatest.Assertions$.newAssertionFailedException(Assertions.scala:1231)
[info]   at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:1295)
[info]   at org.apache.spark.sql.ExpressionsSchemaSuite.$anonfun$new$12(ExpressionsSchemaSuite.scala:190)
[info]   at scala.collection.IterableOnceOps.foreach(IterableOnce.scala:619)
[info]   at scala.collection.IterableOnceOps.foreach$(IterableOnce.scala:617)
[info]   at scala.collection.AbstractIterable.foreach(Iterable.scala:935)
[info]   at org.apache.spark.sql.ExpressionsSchemaSuite.$anonfun$new$1(ExpressionsSchemaSuite.scala:189)
```

### Was this patch authored or co-authored using generative AI tooling?
'No'.

Closes #50379 from beliefer/SPARK-51598.

Authored-by: beliefer <[email protected]>
Signed-off-by: Hyukjin Kwon <[email protected]>
SauronShepherd pushed a commit to SauronShepherd/spark that referenced this pull request Mar 25, 2025
@MaxGekk @SauronShepherd
[SPARK-51420][SQL][TESTS][FOLLOWUP] Re-generate `sql-expression-schem…
426aed9 
…a.md`

### What changes were proposed in this pull request?
In the PR, I propose to regenerate `sql-expression-schema.md` via:
```
$ SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "test:testOnly *ExpressionsSchemaSuite"
```

### Why are the changes needed?
To have `sql-expression-schema.md` up-to-date, and avoid unrelated changes in other PRs.

### Does this PR introduce _any_ user-facing change?
No.

### How was this patch tested?
By running the related test:
```
$ build/sbt "test:testOnly *ExpressionsSchemaSuite"
```

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

Closes apache#50361 from MaxGekk/followup-the-sakthi_SPARK-51420.

Authored-by: Max Gekk <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LuciferYang LuciferYang LuciferYang approved these changes

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

@yaooqinn yaooqinn yaooqinn approved these changes

@beliefer beliefer beliefer approved these changes

Assignees
No one assigned
Labels
SQL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MaxGekk @the-sakthi @LuciferYang @beliefer @HyukjinKwon @yaooqinn