You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "szehon-ho (via GitHub)" <gi...@apache.org> on 2025/06/05 00:00:24 UTC
[PR] [SPARK-52235][SQL][FOLLOW-UP] Add ANSI mode check for new tests [spark]
szehon-ho opened a new pull request, #51092:
URL: https://github.com/apache/spark/pull/51092
### What changes were proposed in this pull request?
Fix non-ANSI test breakage as per https://github.com/apache/spark/pull/50959#pullrequestreview-2890552215
### Why are the changes needed?
Many V2 Expressions are only converted successfully in ANSI mode, so this test for V2 Expression only makes sense in that mode.
### Does this PR introduce _any_ user-facing change?
No, test only
### How was this patch tested?
Run test in NON-ANSI mode
### Was this patch authored or co-authored using generative AI tooling?
No
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52235][SQL][FOLLOW-UP] Add ANSI mode check for new tests [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51092:
URL: https://github.com/apache/spark/pull/51092#discussion_r2129417988
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala:
##########
@@ -676,104 +676,107 @@ class DataSourceV2DataFrameSuite
test("create/replace table default value expression should have a cast") {
val tableName = "testcat.ns1.ns2.tbl"
- withTable(tableName) {
-
- val createExec = executeAndKeepPhysicalPlan[CreateTableExec] {
- sql(
- s"""
- |CREATE TABLE $tableName (
- | col1 int,
- | col2 timestamp DEFAULT '2018-11-17 13:33:33',
- | col3 double DEFAULT 1)
- |""".stripMargin)
- }
- checkDefaultValues(
- createExec.columns,
- Array(
- null,
- new ColumnDefaultValue(
- "'2018-11-17 13:33:33'",
- new LiteralValue(1542490413000000L, TimestampType),
- new LiteralValue(1542490413000000L, TimestampType)),
- new ColumnDefaultValue(
- "1",
- new V2Cast(LiteralValue(1, IntegerType), IntegerType, DoubleType),
- LiteralValue(1.0, DoubleType))))
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> "true") {
Review Comment:
to avoid similar issues in the future, can we turn on ANSI for the entire test suite? we can override `def sparkConf` like https://github.com/apache/spark/blob/master/sql/core/src/test/scala/org/apache/spark/sql/TPCDSQuerySuite.scala#L32
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51987][SPARK-52235][SQL][FOLLOW-UP] Add ANSI mode check for new tests [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #51092: [SPARK-51987][SPARK-52235][SQL][FOLLOW-UP] Add ANSI mode check for new tests
URL: https://github.com/apache/spark/pull/51092
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51987][SPARK-52235][SQL][FOLLOW-UP] Add ANSI mode check for new tests [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #51092:
URL: https://github.com/apache/spark/pull/51092#issuecomment-2949818457
thanks, merging to master!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52235][SQL][FOLLOW-UP] Add ANSI mode check for new tests [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #51092:
URL: https://github.com/apache/spark/pull/51092#discussion_r2129661913
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala:
##########
@@ -676,104 +676,107 @@ class DataSourceV2DataFrameSuite
test("create/replace table default value expression should have a cast") {
val tableName = "testcat.ns1.ns2.tbl"
- withTable(tableName) {
-
- val createExec = executeAndKeepPhysicalPlan[CreateTableExec] {
- sql(
- s"""
- |CREATE TABLE $tableName (
- | col1 int,
- | col2 timestamp DEFAULT '2018-11-17 13:33:33',
- | col3 double DEFAULT 1)
- |""".stripMargin)
- }
- checkDefaultValues(
- createExec.columns,
- Array(
- null,
- new ColumnDefaultValue(
- "'2018-11-17 13:33:33'",
- new LiteralValue(1542490413000000L, TimestampType),
- new LiteralValue(1542490413000000L, TimestampType)),
- new ColumnDefaultValue(
- "1",
- new V2Cast(LiteralValue(1, IntegerType), IntegerType, DoubleType),
- LiteralValue(1.0, DoubleType))))
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> "true") {
Review Comment:
yea let me do that
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52235][SQL][FOLLOW-UP] Add ANSI mode check for new tests [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51092:
URL: https://github.com/apache/spark/pull/51092#discussion_r2130246800
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala:
##########
@@ -43,6 +43,7 @@ class DataSourceV2DataFrameSuite
before {
spark.conf.set("spark.sql.catalog.testcat", classOf[InMemoryTableCatalog].getName)
spark.conf.set("spark.sql.catalog.testcat2", classOf[InMemoryTableCatalog].getName)
+ spark.conf.set(SQLConf.ANSI_ENABLED.key, "true")
Review Comment:
This works but we shouldn't set this conf repeatedly for each test case. I still prefer to override `def sparkConf` to set the conf once for the entire suite.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51987][SPARK-52235][SQL][FOLLOW-UP] Add ANSI mode check for new tests [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #51092:
URL: https://github.com/apache/spark/pull/51092#issuecomment-2947181775
Thanks! Sorry should have seen its cleared by after()
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52235][SQL][FOLLOW-UP] Add ANSI mode check for new tests [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51092:
URL: https://github.com/apache/spark/pull/51092#discussion_r2130603097
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala:
##########
@@ -40,6 +40,10 @@ class DataSourceV2DataFrameSuite
import org.apache.spark.sql.connector.catalog.CatalogV2Implicits._
import testImplicits._
+ override protected def sparkConf: SparkConf =
+ // Disable read-side char padding so that the generated code is less than 8000.
Review Comment:
```suggestion
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52235][SQL][FOLLOW-UP] Add ANSI mode check for new tests [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #51092:
URL: https://github.com/apache/spark/pull/51092#discussion_r2129950451
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala:
##########
@@ -676,104 +676,107 @@ class DataSourceV2DataFrameSuite
test("create/replace table default value expression should have a cast") {
val tableName = "testcat.ns1.ns2.tbl"
- withTable(tableName) {
-
- val createExec = executeAndKeepPhysicalPlan[CreateTableExec] {
- sql(
- s"""
- |CREATE TABLE $tableName (
- | col1 int,
- | col2 timestamp DEFAULT '2018-11-17 13:33:33',
- | col3 double DEFAULT 1)
- |""".stripMargin)
- }
- checkDefaultValues(
- createExec.columns,
- Array(
- null,
- new ColumnDefaultValue(
- "'2018-11-17 13:33:33'",
- new LiteralValue(1542490413000000L, TimestampType),
- new LiteralValue(1542490413000000L, TimestampType)),
- new ColumnDefaultValue(
- "1",
- new V2Cast(LiteralValue(1, IntegerType), IntegerType, DoubleType),
- LiteralValue(1.0, DoubleType))))
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> "true") {
Review Comment:
Hm it didnt work, i think this goes to another sparkconf that's not used by this suite? I guess that's why there's a separate before to override the conf, i put it there.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52235][SQL][FOLLOW-UP] Add ANSI mode check for new tests [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #51092:
URL: https://github.com/apache/spark/pull/51092#issuecomment-2943049890
Test failure look unrelated, re-running
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org