Skip to content

Spark 3.4: Migrate tests in sql#12934

Merged
nastra merged 2 commits into
apache:mainfrom
tomtongue:migrate-spark3.4-sql
Apr 30, 2025
Merged

Spark 3.4: Migrate tests in sql#12934
nastra merged 2 commits into
apache:mainfrom
tomtongue:migrate-spark3.4-sql

Conversation

@tomtongue

Copy link
Copy Markdown
Contributor

Migrate Spark 3.4 tests based on JUnit 4 to Junit5 with AssertJ style. This is related to #7160

This PR migrates the following Spark 3.4 tests in the spark.sql package to JUnit 5 with AssertJ:

  • TestAggregatePushDown
  • TestCreateTable
  • TestCreateTableAsSelect
  • TestDeleteFrom
  • TestDropTable
  • TestFilterPushDown
  • TestNamespaceSQL
  • TestRefreshTable
  • TestSelect
@github-actions github-actions Bot added the spark label Apr 30, 2025
@tomtongue

Copy link
Copy Markdown
Contributor Author

@nastra Sorry for the delay, could you review this when you have a chance? I believe one or two PRs will complete the spark.sql package migration.

@TestTemplate
public void testDefaultNamespace() {
assumeThat(isHadoopCatalog).as("Hadoop has no default namespace configured").isFalse();
assumeThat(catalogConfig.get(ICEBERG_CATALOG_TYPE))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm a little surprised to see this change here. Can you elaborate why this is being added here?

@tomtongue tomtongue Apr 30, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry I was confused with this and the other part. I tried to add this part from Spark 3.4 to Spark 3.5 (I just checked #11093 and #12934)
However, this assumption is no longer needed in Spark 3.4 as well. So let me remove this line.

.as("Should have only db namespace")
.isEqualTo(ImmutableSet.of("db"));
if (isHadoopCatalog
|| catalogConfig.get(ICEBERG_CATALOG_TYPE).equals(ICEBERG_CATALOG_TYPE_REST)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why is this check for REST being added here? we're not testing with REST in this class, so I'm surprised to see this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As well as my reply above, this line also is only for Spark 3.4, and it's no longer needed. Let me remove this as well. Thanks so much for pointing this.

@nastra nastra merged commit f7be3a6 into apache:main Apr 30, 2025
@tomtongue

Copy link
Copy Markdown
Contributor Author

@nastra Thanks so much for the review!

@tomtongue tomtongue deleted the migrate-spark3.4-sql branch April 30, 2025 09:40
anuragmantri added a commit to anuragmantri/iceberg that referenced this pull request Jul 25, 2025
Co-authored-by: Tom Tanaka <43331405+tomtongue@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2 participants