[SPARK-47265][SQL][TESTS] Replace `createTable(..., schema: StructType, ...)` with `createTable(..., columns: Array[Column], ...)` in UT #45368

panbingkun · 2024-03-04T03:09:56Z

What changes were proposed in this pull request?

The pr aims to use createTable(..., schema: StructType, ...) instead of createTable(..., columns: Array[Column], ...) in UT.

Why are the changes needed?

Because the TableCatalog#createTable(..., schema: StructType, ...) method has been marked as deprecated after version 3.4.0.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass GA.
Update existed UT.

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

No.

…dDataSourceSuite`

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

…..)` instead of `createTable(..., columns: Array[Column], ...)` in UT

…FileBasedDataSourceSuite`" This reverts commit 3618b30.

LuciferYang · 2024-03-04T11:43:22Z

hmm.... Is the substitution relationship written in reverse...

LuciferYang · 2024-03-04T13:39:09Z

[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/connector/KeyGroupedPartitioningSuite.scala:26:48: Unused import
[error] Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=unused-imports, site=org.apache.spark.sql.connector
[error] import org.apache.spark.sql.connector.catalog.{CatalogV2Util, Column, Identifier, InMemoryTableCatalog}
[error]

panbingkun · 2024-03-05T05:38:05Z

cc @cloud-fan

cloud-fan · 2024-03-05T07:54:18Z

...st/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryPartitionTableCatalog.scala


  override def createTable(
      ident: Identifier,
      schema: StructType,


can we remove this overload now if no one is calling it?

cloud-fan · 2024-03-05T07:54:36Z

...est/scala/org/apache/spark/sql/connector/catalog/InMemoryRowLevelOperationTableCatalog.scala


  override def createTable(
      ident: Identifier,
      schema: StructType,


panbingkun · 2024-03-06T07:13:54Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala

-  }
-
-  // TODO: remove it when no tests calling this deprecated method.
-  override def createTable(


I will move the specific implementation of createTable from createTable(..., StructType, ...) to createTable(..., Array[Column], ...), so that when we delete the deprecated method createTable(..., StructType, ...) in TableCatalog, we can directly delete the method createTable(..., StructType, ...) here.

cloud-fan · 2024-03-06T09:23:32Z

sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala

  }

-  // TODO: remove it when no tests calling this deprecated method.
+  // TODO: remove it when the deprecated method `createTable(..., StructType, ...)`


I think we can remove it now? Spark won't call this method, only tests will

If we want to delete this now, then we need to remove the method TableCatalog#createTable(..., StructType, ...),

I am a bit worried that although TableCatalog is marked with @Evolving, third parties (eg: iceberg) relying on this interface will immediately feel the break changes, such as:
https://github.com/apache/iceberg/blob/f0b3733e0e7071894bdf63133bcc8c00754a1080/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkSessionCatalog.java#L172-L182

Do we really need to do this now?
At present, Spark can indeed directly delete it internally.

Oh wait, so we only keep it to pass compilation? Then we can just throw an internal exception in this method?

Okay, very good suggestion! I will try it out, thanks.

Simply from the definition of @Evolving, it is indeed possible to make this change in 4.0.

I'm not sure if we need to pay attention to direct calls to this method from non-Spark internal code(s there really such a possibility?). If so, throwing an exception only delays the problem from compile time to runtime...

It's not about callers, but about existing TableCatalog implementations and it's better to not break them.

cloud-fan · 2024-03-06T09:25:33Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala

    null // Return null to save the `loadTable` call for CREATE TABLE without AS SELECT.
  }

+  // TODO: remove it when the deprecated method `createTable(..., StructType, ...)`


LuciferYang · 2024-03-07T06:42:37Z

sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala

  }

-  // TODO: remove it when no tests calling this deprecated method.
+  // TODO: remove it when the deprecated method `createTable(..., StructType, ...)`


Should we remove this TODO? Isn't the current solution the final one?

I'm actually a bit hesitant, because if we eventually decide to completely remove this in a future version, keeping it now may be a good reminder, and I want to hear everyone's opinions. @cloud-fan

I think we can remove the TODO now. We should never remove the API and implementation will have to provide a fake implementation that just fails.

I suggest adding some new comments to explain the reason for retaining this API and throwing exceptions by default while removing the TODO

I think we can remove the TODO now. We should never remove the API and implementation will have to provide a fake implementation that just fails.

Done.

LuciferYang · 2024-03-07T06:43:19Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala

    null // Return null to save the `loadTable` call for CREATE TABLE without AS SELECT.
  }

+  // TODO: remove it when the deprecated method `createTable(..., StructType, ...)`


cloud-fan · 2024-03-08T08:48:13Z

thanks, merging to master!

…fail ### What changes were proposed in this pull request? This is a followup of #45368 . `DelegatingCatalogExtension` can invoke any method in `V2SessionCatalog`, and we should make sure the two overloads of `createTable` in `V2SessionCatalog` both work. ### Why are the changes needed? Avoid breaking custom catalogs that extend `DelegatingCatalogExtension`. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? N/A ### Was this patch authored or co-authored using generative AI tooling? no Closes #47755 from cloud-fan/create-table. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Kent Yao <[email protected]>

[SPARK-47265][SQL][TESTS] Enable test of v2 data sources in `FileBase…

3618b30

…dDataSourceSuite`

github-actions bot added the SQL label Mar 4, 2024

panbingkun changed the title ~~[SPARK-47265][SQL][TESTS] Enable test of v2 data sources in FileBasedDataSourceSuite~~ [WIP][SPARK-47265][SQL][TESTS] Enable test of v2 data sources in FileBasedDataSourceSuite Mar 4, 2024

LuciferYang reviewed Mar 4, 2024

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/FileBasedDataSourceSuite.scala Show resolved Hide resolved

panbingkun closed this Mar 4, 2024

panbingkun added 2 commits March 4, 2024 19:35

[SPARK-47265][SQL][TESTS] Use `createTable(..., schema: StructType, .…

38cc481

…..)` instead of `createTable(..., columns: Array[Column], ...)` in UT

Revert "[SPARK-47265][SQL][TESTS] Enable test of v2 data sources in `…

cf0bbb4

…FileBasedDataSourceSuite`" This reverts commit 3618b30.

panbingkun reopened this Mar 4, 2024

github-actions bot added the CONNECT label Mar 4, 2024

panbingkun changed the title ~~[WIP][SPARK-47265][SQL][TESTS] Enable test of v2 data sources in FileBasedDataSourceSuite~~ [WIP][SPARK-47265][SQL][TESTS] Use createTable(..., schema: StructType, ...) instead of createTable(..., columns: Array[Column], ...) in UT Mar 4, 2024

improve

64909cf

panbingkun added 2 commits March 4, 2024 22:01

fix unused import

f532e15

fix UT

8f3abbe

github-actions bot added the STRUCTURED STREAMING label Mar 5, 2024

panbingkun marked this pull request as ready for review March 5, 2024 05:38

cloud-fan reviewed Mar 5, 2024

View reviewed changes

panbingkun added 2 commits March 6, 2024 14:39

remove redundant override createTable

7a2bcbb

Merge branch 'master' into SPARK-47265

676379e

panbingkun commented Mar 6, 2024

View reviewed changes

cloud-fan reviewed Mar 6, 2024

View reviewed changes

panbingkun added 3 commits March 7, 2024 09:35

call deprecated method throw internal exception

fc368b6

Merge branch 'master' into SPARK-47265

ad42d3a

fix V2SessionCatalogSuite

9cd53b2

panbingkun requested a review from cloud-fan March 7, 2024 06:38

LuciferYang reviewed Mar 7, 2024

View reviewed changes

panbingkun added 2 commits March 8, 2024 13:58

remove comment

e3dcf4c

Merge branch 'master' into SPARK-47265

649254c

cloud-fan approved these changes Mar 8, 2024

View reviewed changes

cloud-fan closed this in 5013105 Mar 8, 2024

panbingkun mentioned this pull request Jul 29, 2024

[SPARK-49037][SQL][TESTS] Replace schema() with columns() #47515

Closed

cloud-fan mentioned this pull request Aug 14, 2024

[SPARK-47265][SQL][FOLLOWUP] V2SessionCatalog#createTable should not fail #47755

Closed

[SPARK-47265][SQL][TESTS] Replace createTable(..., schema: StructType, ...) with createTable(..., columns: Array[Column], ...) in UT #45368

[SPARK-47265][SQL][TESTS] Replace createTable(..., schema: StructType, ...) with createTable(..., columns: Array[Column], ...) in UT #45368

Uh oh!

Conversation

panbingkun commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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

Uh oh!

Uh oh!

LuciferYang commented Mar 4, 2024

Uh oh!

LuciferYang commented Mar 4, 2024

Uh oh!

panbingkun commented Mar 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LuciferYang Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Mar 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[SPARK-47265][SQL][TESTS] Replace `createTable(..., schema: StructType, ...)` with `createTable(..., columns: Array[Column], ...)` in UT #45368

[SPARK-47265][SQL][TESTS] Replace `createTable(..., schema: StructType, ...)` with `createTable(..., columns: Array[Column], ...)` in UT #45368

panbingkun commented Mar 4, 2024 •

edited

Loading

LuciferYang Mar 6, 2024 •

edited

Loading

LuciferYang Mar 7, 2024 •

edited

Loading