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 2026/01/08 22:53:39 UTC
[PR] [SPARK-54975][SQL][TESTS] Refactor Merge Into Schema Evolution Tests [spark]
szehon-ho opened a new pull request, #53738:
URL: https://github.com/apache/spark/pull/53738
### What changes were proposed in this pull request?
The MergeIntoTableSuiteBase class had become unwieldy (7200+ lines), of which around 5000 lines were schema evolution tests
1. Move the many Merge Into Schema Evolution tests into their own file
2. Make helper methods testEvolution , etc (this is inspired by Delta Lake spark/src/test/scala/org/apache/spark/sql/delta/MergeIntoSchemaEvolutionSuite.scala). These will run the tests a. with schema evolution, 2. without schema evolution, and 3. using coerceNestedTypes on and off if applicable
4. Migrate all ~70 tests to use that framework
5. Add even more dimensions (group-based vs delta-based MERGE, Scala API, SQL)
### Why are the changes needed?
1. This cuts down the tests from 5000 lines to less than 3000 lines.
1. Inspired by Delta Lake's test approach, this makes it a bit easier to see what is being tested by focusing on the differences in data/schema and expectations.
### Does this PR introduce _any_ user-facing change?
No, only test
### How was this patch tested?
Run existing test
### Was this patch authored or co-authored using generative AI tooling?
A bit using Claude 4.5 opus, but mostly hand tuning
--
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-54975][SQL][TESTS] Refactor Merge Into Schema Evolution Tests [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #53738:
URL: https://github.com/apache/spark/pull/53738#issuecomment-3731935328
@cloud-fan can you help take a look? thanks
--
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-54975][SQL][TESTS] Refactor Merge Into Schema Evolution Tests [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #53738:
URL: https://github.com/apache/spark/pull/53738#discussion_r2696423647
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DeltaBasedMergeIntoSchemaEvolutionSuite.scala:
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.connector
+
+
+// SQL-based tests for delta-based row-level operations
+class DeltaBasedMergeIntoSchemaEvolutionSQLSuite
+ extends MergeIntoSchemaEvolutionSQLSuiteBase
Review Comment:
I see. yes i was hoping to eventually run some Delta tests here. it would need different override of testEvolution with different default value (like the condition, source, target tables). Hence was thinking to have DeltabasedMergeIntoCompatSuite extends MergeIntoSchemaEvolutionSQLSuiteBase with MergeIntoCompatTests. It could be for other things to test as well.
But its just my hope, let me know if its too unlike current Spark tests and i can undo the separation
--
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-54975][SQL][TESTS] Refactor Merge Into Schema Evolution Tests [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #53738:
URL: https://github.com/apache/spark/pull/53738#issuecomment-3761256345
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-54975][SQL][TESTS] Refactor Merge Into Schema Evolution Tests [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #53738:
URL: https://github.com/apache/spark/pull/53738#discussion_r2683478625
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/MergeIntoSchemaEvolutionTests.scala:
##########
@@ -0,0 +1,2841 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.connector
+
+import org.apache.spark.sql.Row
+import org.apache.spark.sql.types._
+
+/**
+ * Base trait containing merge into schema evolution tests.
+ */
+trait MergeIntoSchemaEvolutionTests extends MergeIntoSchemaEvolutionSuiteBase {
Review Comment:
whoops, fixed the extension in sql/core/src/test/scala/org/apache/spark/sql/connector/GroupBasedMergeIntoSchemaEvolutionSuite.scala
--
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-54975][SQL][TESTS] Refactor Merge Into Schema Evolution Tests [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #53738:
URL: https://github.com/apache/spark/pull/53738#issuecomment-3726235314
## JIRA Issue Information
=== Test SPARK-54975 ===
Summary: Refactor Merge Into Schema Evolution Tests
Assignee: None
Status: Open
Affected: ["4.1.0"]
---
*This comment was automatically generated by GitHub Actions*
--
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-54975][SQL][TESTS] Refactor Merge Into Schema Evolution Tests [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #53738:
URL: https://github.com/apache/spark/pull/53738#discussion_r2685917400
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DeltaBasedMergeIntoSchemaEvolutionSuite.scala:
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.connector
+
+
+// SQL-based tests for delta-based row-level operations
+class DeltaBasedMergeIntoSchemaEvolutionSQLSuite
+ extends MergeIntoSchemaEvolutionSQLSuiteBase
Review Comment:
it's a clean design to separate the test utils and test cases, but it also means the concrete test suites need to extend/mix multiple parent classes/traits.
Shall we let `MergeIntoSchemaEvolutionSQLSuiteBase` extends `MergeIntoSchemaEvolutionTests` and `MergeIntoSchemaEvolutionExtraSQLTests` directly? Or we do plan to have special suites that need different MERGE INTO test cases?
--
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-54975][SQL][TESTS] Refactor Merge Into Schema Evolution Tests [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #53738: [SPARK-54975][SQL][TESTS] Refactor Merge Into Schema Evolution Tests
URL: https://github.com/apache/spark/pull/53738
--
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-54975][SQL][TESTS] Refactor Merge Into Schema Evolution Tests [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #53738:
URL: https://github.com/apache/spark/pull/53738#discussion_r2683478625
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/MergeIntoSchemaEvolutionTests.scala:
##########
@@ -0,0 +1,2841 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.connector
+
+import org.apache.spark.sql.Row
+import org.apache.spark.sql.types._
+
+/**
+ * Base trait containing merge into schema evolution tests.
+ */
+trait MergeIntoSchemaEvolutionTests extends MergeIntoSchemaEvolutionSuiteBase {
Review Comment:
whoops, fixed the extension in sql/core/src/test/scala/org/apache/spark/sql/connector/GroupBasedMergeIntoSchemaEvolutionSuite.scala
thanks for catching it
--
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-54975][SQL][TESTS] Refactor Merge Into Schema Evolution Tests [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #53738:
URL: https://github.com/apache/spark/pull/53738#discussion_r2680715644
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/MergeIntoSchemaEvolutionTests.scala:
##########
@@ -0,0 +1,2841 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.connector
+
+import org.apache.spark.sql.Row
+import org.apache.spark.sql.types._
+
+/**
+ * Base trait containing merge into schema evolution tests.
+ */
+trait MergeIntoSchemaEvolutionTests extends MergeIntoSchemaEvolutionSuiteBase {
Review Comment:
is this for delta-based merge into only? because group-based test suites do not extend it.
--
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