Skip to content

[WIP][SQL][TESTS] Disable stable column aliases in tests if assumed #51337

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
+731 −646
Draft

[WIP][SQL][TESTS] Disable stable column aliases in tests if assumed #51337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
310ca4b
Disable stable column aliases in tests that it is assumed
MaxGekk Jul 1, 2025
26f6c33
Fix tests
MaxGekk Jul 1, 2025
7f5b43c
Fix tests
MaxGekk Jul 1, 2025
2f734ef
Fix tests
MaxGekk Jul 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 58 additions & 52 deletions connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1762,7 +1762,9 @@ abstract class AvroSuite
}

test("error handling for unsupported Interval data types") {
withSQLConf(SQLConf.LEGACY_INTERVAL_ENABLED.key -> "true") {
withSQLConf(
SQLConf.STABLE_DERIVED_COLUMN_ALIAS_ENABLED.key -> "false",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we change the test query to add explicit alias?

Copy link
Member Author

@MaxGekk MaxGekk Jul 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, this could be another way, but this require much more work. I will try my best, and change some test in this way.

Copy link
Contributor

@cloud-fan cloud-fan Jul 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's better to not randomly switch this conf here and there. We just need to publish a good habit for writing tests: alias the expressions when necessary to have stable alias.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's better to randomly switch this conf here and there.

It is good idea, but before implementing this we should make sure that existing tests are reliable to such behaviour otherwise we will destabilize the existing CI.

We just need to publish a good habit for writing tests

Habits is nice, and I believe if a test writer assumes some naming convention, this assumption should be explicit via a config set otherwise he/she should assign an alias. How about to put this statement to the coding style?

alias the expressions when necessary to have stable alias.

I will do when it is obvious, but some tests are depend SQLConf.STABLE_DERIVED_COLUMN_ALIAS_ENABLED for unknown reasons at least for me (I cannot say why without investigation). For instance, the simple case where names should be the same in plan and in the query independently from the config:

  test("SPARK-42552: select and union without parentheses") {
    val plan = Distinct(OneRowRelation().select(Literal(1))
      .union(OneRowRelation().select(Literal(1))))
    assertEqual("select 1 union select 1", plan)
  }

fails with:

== FAIL: Plans do not match ===
 'Distinct                             'Distinct
 +- 'Union false, false                +- 'Union false, false
    :- 'Project [unresolvedalias(1)]      :- 'Project [unresolvedalias(1)]
    :  +- OneRowRelation                  :  +- OneRowRelation
    +- 'Project [unresolvedalias(1)]      +- 'Project [unresolvedalias(1)]
       +- OneRowRelation                     +- OneRowRelation

SQLConf.LEGACY_INTERVAL_ENABLED.key -> "true") {
withTempDir { dir =>
val tempDir = new File(dir, "files").getCanonicalPath
checkError(
Expand Down Expand Up @@ -3016,34 +3018,36 @@ abstract class AvroSuite
}

test("SPARK-33865: CREATE TABLE DDL with avro should check col name") {
withTable("test_ddl") {
withView("v") {
spark.range(1).createTempView("v")
withTempDir { dir =>
checkError(
exception = intercept[AnalysisException] {
sql(
s"""
|CREATE TABLE test_ddl USING AVRO
|LOCATION '${dir}'
|AS SELECT ID, IF(ID=1,1,0) FROM v""".stripMargin)
},
condition = "INVALID_COLUMN_NAME_AS_PATH",
parameters = Map(
"datasource" -> "AvroFileFormat", "columnName" -> "`(IF((ID = 1), 1, 0))`")
)
}
withSQLConf(SQLConf.STABLE_DERIVED_COLUMN_ALIAS_ENABLED.key -> "false") {
withTable("test_ddl") {
withView("v") {
spark.range(1).createTempView("v")
withTempDir { dir =>
checkError(
exception = intercept[AnalysisException] {
sql(
s"""
|CREATE TABLE test_ddl USING AVRO
|LOCATION '${dir}'
|AS SELECT ID, IF(ID=1,1,0) FROM v""".stripMargin)
},
condition = "INVALID_COLUMN_NAME_AS_PATH",
parameters = Map(
"datasource" -> "AvroFileFormat", "columnName" -> "`(IF((ID = 1), 1, 0))`")
)
}

withTempDir { dir =>
spark.sql(
s"""
|CREATE TABLE test_ddl USING AVRO
|LOCATION '${dir}'
|AS SELECT ID, IF(ID=1,ID,0) AS A, ABS(ID) AS B
|FROM v""".stripMargin)
val expectedSchema = StructType(Seq(StructField("ID", LongType, true),
StructField("A", LongType, true), StructField("B", LongType, true)))
assert(spark.table("test_ddl").schema == expectedSchema)
withTempDir { dir =>
spark.sql(
s"""
|CREATE TABLE test_ddl USING AVRO
|LOCATION '${dir}'
|AS SELECT ID, IF(ID=1,ID,0) AS A, ABS(ID) AS B
|FROM v""".stripMargin)
val expectedSchema = StructType(Seq(StructField("ID", LongType, true),
StructField("A", LongType, true), StructField("B", LongType, true)))
assert(spark.table("test_ddl").schema == expectedSchema)
}
}
}
}
Expand Down Expand Up @@ -3142,31 +3146,33 @@ class AvroV1Suite extends AvroSuite {
.set(SQLConf.USE_V1_SOURCE_LIST, "avro")

test("SPARK-36271: V1 insert should check schema field name too") {
withView("v") {
spark.range(1).createTempView("v")
withTempDir { dir =>
checkError(
exception = intercept[AnalysisException] {
sql("SELECT ID, IF(ID=1,1,0) FROM v").write.mode(SaveMode.Overwrite)
.format("avro").save(dir.getCanonicalPath)
},
condition = "INVALID_COLUMN_NAME_AS_PATH",
parameters = Map(
"datasource" -> "AvroFileFormat", "columnName" -> "`(IF((ID = 1), 1, 0))`")
)
}
withSQLConf(SQLConf.STABLE_DERIVED_COLUMN_ALIAS_ENABLED.key -> "false") {
withView("v") {
spark.range(1).createTempView("v")
withTempDir { dir =>
checkError(
exception = intercept[AnalysisException] {
sql("SELECT ID, IF(ID=1,1,0) FROM v").write.mode(SaveMode.Overwrite)
.format("avro").save(dir.getCanonicalPath)
},
condition = "INVALID_COLUMN_NAME_AS_PATH",
parameters = Map(
"datasource" -> "AvroFileFormat", "columnName" -> "`(IF((ID = 1), 1, 0))`")
)
}

withTempDir { dir =>
checkError(
exception = intercept[AnalysisException] {
sql("SELECT NAMED_STRUCT('(IF((ID = 1), 1, 0))', IF(ID=1,ID,0)) AS col1 FROM v")
.write.mode(SaveMode.Overwrite)
.format("avro").save(dir.getCanonicalPath)
},
condition = "INVALID_COLUMN_NAME_AS_PATH",
parameters = Map(
"datasource" -> "AvroFileFormat", "columnName" -> "`(IF((ID = 1), 1, 0))`")
)
withTempDir { dir =>
checkError(
exception = intercept[AnalysisException] {
sql("SELECT NAMED_STRUCT('(IF((ID = 1), 1, 0))', IF(ID=1,ID,0)) AS col1 FROM v")
.write.mode(SaveMode.Overwrite)
.format("avro").save(dir.getCanonicalPath)
},
condition = "INVALID_COLUMN_NAME_AS_PATH",
parameters = Map(
"datasource" -> "AvroFileFormat", "columnName" -> "`(IF((ID = 1), 1, 0))`")
)
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5580,7 +5580,7 @@ object SQLConf {
"and form them via pretty SQL print.")
.version("3.5.0")
.booleanConf
.createWithDefault(false)
.createWithDefault(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have done it earlier to make sure this feature works well by passing all tests

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should handle test failures carefully and figure out if there is any bug in this feature, instead of blindly turning off the config.

This will take a long time and we should invite the community to work on it together.

Copy link
Member Author

@MaxGekk MaxGekk Jul 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I switched the config only to detect the tests that make some assumption about column aliases algorithm. Not going to merge it.

we should handle test failures carefully and figure out if there is any bug in this feature

I agree. We should.


val LOCAL_RELATION_CACHE_THRESHOLD =
buildConf(SqlApiConfHelper.LOCAL_RELATION_CACHE_THRESHOLD_KEY)
Expand Down
14 changes: 8 additions & 6 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2474,12 +2474,14 @@ class DDLParserSuite extends AnalysisTest {
}

test("alter view: AS Query") {
val parsed = parsePlan("ALTER VIEW a.b.c AS SELECT 1")
val expected = AlterViewAs(
UnresolvedView(Seq("a", "b", "c"), "ALTER VIEW ... AS", true, false),
"SELECT 1",
parsePlan("SELECT 1"))
comparePlans(parsed, expected)
withSQLConf(SQLConf.STABLE_DERIVED_COLUMN_ALIAS_ENABLED.key -> "false") {
val parsed = parsePlan("ALTER VIEW a.b.c AS SELECT 1")
val expected = AlterViewAs(
UnresolvedView(Seq("a", "b", "c"), "ALTER VIEW ... AS", true, false),
"SELECT 1",
parsePlan("SELECT 1"))
comparePlans(parsed, expected)
}
}

test("DESCRIBE FUNCTION") {
Expand Down
52 changes: 29 additions & 23 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/ExpressionParserSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,11 @@ class ExpressionParserSuite extends AnalysisTest {
}

test("exists expression") {
assertEqual(
"exists (select 1 from b where b.x = a.x)",
Exists(table("b").where($"b.x" === $"a.x").select(1)))
withSQLConf(SQLConf.STABLE_DERIVED_COLUMN_ALIAS_ENABLED.key -> "false") {
assertEqual(
"exists (select 1 from b where b.x = a.x)",
Exists(table("b").where($"b.x" === $"a.x").select(1)))
}
}

test("comparison expressions") {
Expand Down Expand Up @@ -333,20 +335,22 @@ class ExpressionParserSuite extends AnalysisTest {
}

test("function expressions with named arguments") {
assertEqual("encode(value => 'abc', charset => 'utf-8')",
$"encode".function(NamedArgumentExpression("value", Literal("abc")),
NamedArgumentExpression("charset", Literal("utf-8"))))
assertEqual("encode('abc', charset => 'utf-8')",
$"encode".function(Literal("abc"), NamedArgumentExpression("charset", Literal("utf-8"))))
assertEqual("encode(charset => 'utf-8', 'abc')",
$"encode".function(NamedArgumentExpression("charset", Literal("utf-8")), Literal("abc")))
assertEqual("encode('abc', charset => 'utf' || '-8')",
$"encode".function(Literal("abc"), NamedArgumentExpression("charset",
Concat(Literal("utf") :: Literal("-8") :: Nil))))
val unresolvedAlias = Project(Seq(UnresolvedAlias(Literal("1"))), OneRowRelation())
assertEqual("encode(value => ((select '1')), charset => 'utf-8')",
$"encode".function(NamedArgumentExpression("value", ScalarSubquery(plan = unresolvedAlias)),
NamedArgumentExpression("charset", Literal("utf-8"))))
withSQLConf(SQLConf.STABLE_DERIVED_COLUMN_ALIAS_ENABLED.key -> "false") {
assertEqual("encode(value => 'abc', charset => 'utf-8')",
$"encode".function(NamedArgumentExpression("value", Literal("abc")),
NamedArgumentExpression("charset", Literal("utf-8"))))
assertEqual("encode('abc', charset => 'utf-8')",
$"encode".function(Literal("abc"), NamedArgumentExpression("charset", Literal("utf-8"))))
assertEqual("encode(charset => 'utf-8', 'abc')",
$"encode".function(NamedArgumentExpression("charset", Literal("utf-8")), Literal("abc")))
assertEqual("encode('abc', charset => 'utf' || '-8')",
$"encode".function(Literal("abc"), NamedArgumentExpression("charset",
Concat(Literal("utf") :: Literal("-8") :: Nil))))
val unresolvedAlias = Project(Seq(UnresolvedAlias(Literal("1"))), OneRowRelation())
assertEqual("encode(value => ((select '1')), charset => 'utf-8')",
$"encode".function(NamedArgumentExpression("value", ScalarSubquery(plan = unresolvedAlias)),
NamedArgumentExpression("charset", Literal("utf-8"))))
}
}

private def lv(s: Symbol) = UnresolvedNamedLambdaVariable(Seq(s.name))
Expand Down Expand Up @@ -477,12 +481,14 @@ class ExpressionParserSuite extends AnalysisTest {
}

test("scalar sub-query") {
assertEqual(
"(select max(val) from tbl) > current",
ScalarSubquery(table("tbl").select($"max".function($"val"))) > $"current")
assertEqual(
"a = (select b from s)",
$"a" === ScalarSubquery(table("s").select($"b")))
withSQLConf(SQLConf.STABLE_DERIVED_COLUMN_ALIAS_ENABLED.key -> "false") {
assertEqual(
"(select max(val) from tbl) > current",
ScalarSubquery(table("tbl").select($"max".function($"val"))) > $"current")
assertEqual(
"a = (select b from s)",
$"a" === ScalarSubquery(table("s").select($"b")))
}
}

test("case when") {
Expand Down
8 changes: 5 additions & 3 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,11 @@ class PlanParserSuite extends AnalysisTest {
}

test("SPARK-42552: select and union without parentheses") {
val plan = Distinct(OneRowRelation().select(Literal(1))
.union(OneRowRelation().select(Literal(1))))
assertEqual("select 1 union select 1", plan)
withSQLConf(SQLConf.STABLE_DERIVED_COLUMN_ALIAS_ENABLED.key -> "false") {
val plan = Distinct(OneRowRelation().select(Literal(1))
.union(OneRowRelation().select(Literal(1))))
assertEqual("select 1 union select 1", plan)
}
}

test("set operations") {
Expand Down
22 changes: 12 additions & 10 deletions sql/connect/client/jvm/src/test/scala/org/apache/spark/sql/connect/ClientE2ETestSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1277,19 +1277,21 @@ class ClientE2ETestSuite
}

test("sql() with positional parameters") {
val result0 = spark.sql("select 1", Array.empty).collect()
assert(result0.length == 1 && result0(0).getInt(0) === 1)
withSQLConf("spark.sql.stableDerivedColumnAlias.enabled" -> "false") {
val result0 = spark.sql("select 1", Array.empty).collect()
assert(result0.length == 1 && result0(0).getInt(0) === 1)

val result1 = spark.sql("select ?", Array(1)).collect()
assert(result1.length == 1 && result1(0).getInt(0) === 1)
val result1 = spark.sql("select ?", Array(1)).collect()
assert(result1.length == 1 && result1(0).getInt(0) === 1)

val result2 = spark.sql("select ?, ?", Array(1, "abc")).collect()
assert(result2.length == 1)
assert(result2(0).getInt(0) === 1)
assert(result2(0).getString(1) === "abc")
val result2 = spark.sql("select ?, ?", Array(1, "abc")).collect()
assert(result2.length == 1)
assert(result2(0).getInt(0) === 1)
assert(result2(0).getString(1) === "abc")

val result3 = spark.sql("select element_at(?, 1)", Array(array(lit(1)))).collect()
assert(result3.length == 1 && result3(0).getInt(0) === 1)
val result3 = spark.sql("select element_at(?, 1)", Array(array(lit(1)))).collect()
assert(result3.length == 1 && result3(0).getInt(0) === 1)
}
}

test("sql() with named parameters") {
Expand Down
Loading