[SPARK-33393][SQL] Support SHOW TABLE EXTENDED in v2 #37588

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

Jump to bottom

Closed

panbingkun wants to merge 44 commits into apache:master from panbingkun:v2_SHOW_TABLE_EXTENDED

Contributor

panbingkun commented Aug 20, 2022 •

edited

Loading

What changes were proposed in this pull request?

The pr aim to implement v2 SHOW TABLE EXTENDED as ShowTableExec

Why are the changes needed?

To have feature parity with the datasource V1.

Does this PR introduce any user-facing change?

Yes, Support SHOW TABLE EXTENDED in v2.

How was this patch tested?

Add new UT.
By running the unified tests for v2 implementation:

$ build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *ShowTablesSuite"
$ build/sbt "test:testOnly *ShowTablesSuite"


          [SPARK-33393][SQL] Support SHOW TABLE EXTENDED in DSv2

39f3c42

github-actions bot added the SQL label

panbingkun changed the title ~~[SPARK-33393][SQL] Support SHOW TABLE EXTENDED in v2~~ [Don't Review][TEST][SPARK-33393][SQL] Support SHOW TABLE EXTENDED in v2

panbingkun added 2 commits

August 20, 2022 21:28


          [SPARK-33393][SQL] Support SHOW TABLE EXTENDED in v2

cbe7433


          [SPARK-33393][SQL] Support SHOW TABLE EXTENDED in v

023414b

panbingkun changed the title ~~[Don't Review][TEST][SPARK-33393][SQL] Support SHOW TABLE EXTENDED in v2~~ [SPARK-33393][SQL] Support SHOW TABLE EXTENDED in v2

Contributor Author

panbingkun commented Aug 20, 2022

AmplabJenkins commented Aug 21, 2022

Can one of the admins verify this patch?

MaxGekk requested changes

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala Outdated Show resolved Hide resolved


          [SPARK-33393][SQL] Support SHOW TABLE EXTENDED in v2

panbingkun commented

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolvePartitionSpec.scala Outdated Show resolved Hide resolved

panbingkun requested a review from MaxGekk

August 28, 2022 10:57

panbingkun commented

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolvePartitionSpec.scala Outdated Show resolved Hide resolved

panbingkun commented

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/v2ResolutionPlans.scala Outdated Show resolved Hide resolved

Contributor

Fokko commented Nov 28, 2022

@panbingkun @MaxGekk Any progress on this? Thanks!

Fokko mentioned this pull request

show table extended not supported for v2 table. apache/iceberg#5782

Closed

ja-michel commented Nov 30, 2022

+1

1 similar comment

rshanmugam1 commented Dec 2, 2022

+1

Member

MaxGekk commented Feb 1, 2023

@panbingkun Could you resolve conflicts when you have time, please.


          [SPARK-33393][SQL] Support SHOW TABLE EXTENDED in v2

676455e

Contributor Author

panbingkun commented Feb 5, 2023

@panbingkun Could you resolve conflicts when you have time, please.

Done


          [SPARK-33393][SQL] Support SHOW TABLE EXTENDED in v2

Contributor

xkrogen commented Feb 13, 2023

@MaxGekk are you able to take another look at this PR? Would love to see it go in, this is an annoying feature gap that we need to work around currently for DSv2 sources :(

Member

MaxGekk commented Feb 13, 2023

are you able to take another look at this PR?

No, I haven't looked at this yet because of my wedding. :-)

this is an annoying feature gap that we need to work around currently for DSv2 sources :(

@xkrogen If you really need this feature, please, review this PR. I will join to you slightly later.

MaxGekk reviewed

View reviewed changes

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

-                  pattern: Option[String]) extends V2CommandExec with LeafExecNode {
+                  pattern: Option[String],
+                  isExtended: Boolean = false,
+                  partitionSpec: Option[TablePartitionSpec] = None) extends V2CommandExec with LeafExecNode {

Member

MaxGekk Feb 16, 2023

TablePartitionSpec is legacy one, can't you use ResolvedPartitionSpec? For example, see

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

Line 36 in 0494dc9

    
           partitionSpec: Option[ResolvedPartitionSpec]) extends V2CommandExec with LeafExecNode {

Contributor Author

panbingkun Feb 20, 2023

Done

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

+                  // "Partition Values"
+                  val partitionSchema = partitionTable.partitionSchema()
+                  val normalizedSpec = normalizePartitionSpec(

Member

MaxGekk Feb 16, 2023

This one is not needed. The job should be done by ResolvePartitionSpec.resolvePartitionSpec, or there is some reason to bypass it?

Contributor Author

panbingkun Feb 20, 2023

Yes, the latest version has eliminated the above logic.

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

+                  requireExactMatchedPartitionSpec(identifier.toString,
+                    normalizedSpec, partitionSchema.fieldNames)
+                  val partitionNames = normalizedSpec.keySet

Member

MaxGekk Feb 16, 2023

Where is it used?

Contributor Author

panbingkun Feb 20, 2023

Done

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

+                    convertToPartIdent(normalizedSpec, partitionSchema))
+                  val partitionIdentifiers = partitionTable.listPartitionIdentifiers(names.toArray, ident)
+                  partitionIdentifiers.length match {
+                    case 0 =>

Member

MaxGekk Feb 16, 2023

The functions extendedPartition() is invoked only for non-empty partition spec as I can see, or not? Is there any test for this case?

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

+                    case 0 =>
+                      throw QueryExecutionErrors.notExistPartitionError(
+                        identifier.toString, ident, partitionSchema)
+                    case len if (len > 1) =>

Member

MaxGekk Feb 16, 2023

nit:

Suggested change

      
                  case len if (len > 1) =>
          
                  case len if len > 1 =>

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

Comment on lines 171 to 172

		var i = 0
		while (i < len) {

Member

MaxGekk Feb 16, 2023

This loop:

    var i = 0
    while (i < len) {

      i += 1
    }

can be simplified by:

    for (i <- 0 until len) {

    }

panbingkun added 3 commits

February 20, 2023 17:08


          [SPARK-33393][SQL] Support SHOW TABLE EXTENDED in v2

7d71eb0


          Merge branch 'master' into v2_SHOW_TABLE_EXTENDED

5c40c7c


          [SPARK-33393][SQL] Support SHOW TABLE EXTENDED in v2

3c3651a

panbingkun requested a review from MaxGekk

February 20, 2023 11:45


          [SPARK-33393][SQL] Support SHOW TABLE EXTENDED in v2

b1cc56c

cloud-fan reviewed

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/command/ShowTablesSuiteBase.scala Outdated Show resolved Hide resolved

cloud-fan reviewed

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/ShowTablesSuite.scala

+                    catalog: String,
+                    namespace: String,
+                    table: String): (String, Map[String, String]) = {
+                  ("_LEGACY_ERROR_TEMP_1251",

Contributor

cloud-fan Nov 2, 2023

how hard it is to unify this error between v1 and v2 tables?

Contributor Author

panbingkun Nov 3, 2023

_LEGACY_ERROR_TEMP_1231(QueryCompilationErrors#invalidPartitionColumnKeyInTableError), there are approximately 13 code point to call it:

spark/common/utils/src/main/resources/error/error-classes.json

Lines 4815 to 4819 in 1359c13

    
           "_LEGACY_ERROR_TEMP_1231" : { 
        
             "message" : [ 
        
               "<key> is not a valid partition column in table <tblName>." 
        
             ] 
        
           },

_LEGACY_ERROR_TEMP_1251(QueryCompilationErrors#actionNotAllowedOnTableSincePartitionMetadataNotStoredError), there are approximately 8 code point to call it:

spark/common/utils/src/main/resources/error/error-classes.json

Lines 4860 to 4864 in 1359c13

    
           "_LEGACY_ERROR_TEMP_1251" : { 
        
             "message" : [ 
        
               "<action> is not allowed on <tableName> since its partition metadata is not stored in the Hive metastore. To import this information into the metastore, run `msck repair table <tableName>`." 
        
             ] 
        
           },

Because there are many scopes involved, in order to reduce the interference of logic on this PR, I suggest doing the merging or unification in a new separate PR.
Do you think this is appropriate?

Contributor

cloud-fan Nov 3, 2023

make senses, let's do it in followup

cloud-fan reviewed

View reviewed changes

Contributor

cloud-fan left a comment

I think it's pretty close now, thanks for your patience!

Contributor Author

panbingkun commented Nov 2, 2023 •

edited

Loading

I think it's pretty close now, thanks for your patience!

I will update it today.
Thank you very much for your patience and carefulness, which has given me great help! ❤️

panbingkun and others added 3 commits

November 3, 2023 09:47


          Apply suggestions from code review

b47e619

Co-authored-by: Wenchen Fan <cloud0fan@gmail.com>


          [SPARK-33393][SQL] Support SHOW TABLE EXTENDED in v2

be5b337


          Merge branch 'master' into v2_SHOW_TABLE_EXTENDED

73ff15b

panbingkun requested a review from cloud-fan

November 3, 2023 13:12

cloud-fan reviewed

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala Outdated

+                  val globalTempViews = if (dbName == globalTempViewManager.database) {
+                    globalTempViewManager.listViewNames(pattern).map { viewName =>
+                      globalTempViewManager.get(viewName).map(_.tableMeta).getOrElse(
+                        throw new NoSuchTableException(globalTempViewManager.database, viewName))

Contributor

cloud-fan Nov 6, 2023

It's weird to throw this error during listing views. Shall we use flatMap and just skip the temp views that were deleted immediately after globalTempViewManager.listViewNames?

Contributor Author

panbingkun Nov 6, 2023

Okay

cloud-fan reviewed

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala Outdated

+                  val localTempViews = listLocalTempViews(pattern).map { viewIndent =>
+                    tempViews.get(viewIndent.table).map(_.tableMeta).getOrElse(
+                      throw new NoSuchTableException(viewIndent.database.getOrElse(""), viewIndent.table))

Contributor

cloud-fan Nov 6, 2023

ditto

cloud-fan reviewed

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala Outdated

                  */
                 override def visitShowTableExtended(
                     ctx: ShowTableExtendedContext): LogicalPlan = withOrigin(ctx) {
-                  val partitionKeys = Option(ctx.partitionSpec).map { specCtx =>
-                    UnresolvedPartitionSpec(visitNonOptionalPartitionSpec(specCtx), None)
+                  @inline def createUnresolvedTable(

Contributor

cloud-fan Nov 6, 2023

not this kind of inline... we can remove this function and put the code in where we call the function

Contributor Author

panbingkun Nov 6, 2023

Okay，I misunderstood the meaning, haha

panbingkun added 2 commits

November 6, 2023 19:51


          [SPARK-33393][SQL] Support SHOW TABLE EXTENDED in v2

e6978e3


          Merge branch 'master' into v2_SHOW_TABLE_EXTENDED

1bab140

github-actions bot removed the BUILD label

panbingkun requested a review from cloud-fan

November 6, 2023 12:02

beliefer reviewed

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala Outdated

Comment on lines 1097 to 1098

		val dbName = format(db)
		val globalTempViews = if (dbName == globalTempViewManager.database) {

Contributor

beliefer Nov 6, 2023

Suggested change

      
                val dbName = format(db)
          
                val globalTempViews = if (dbName == globalTempViewManager.database) {
          
                val globalTempViews = if (format(db) == globalTempViewManager.database) {

Contributor Author

panbingkun Nov 7, 2023

Done


          fix minor

26ae679

panbingkun mentioned this pull request

[SPARK-45880][SQL] Make the like pattern semantics used in all commands consistent #43751

Closed

beliefer reviewed

View reviewed changes

...re/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ShowTablesExtendedExec.scala

+                  if (table.supportsPartitions && table.asPartitionable.partitionSchema().nonEmpty) {
+                    partitionColumns = table.asPartitionable.partitionSchema()
+                    results.put("Partition Provider", "Catalog")
+                    results.put("Partition Columns", table.asPartitionable.partitionSchema().map(

Contributor

beliefer Nov 11, 2023

Suggested change

      
                  results.put("Partition Columns", table.asPartitionable.partitionSchema().map(
          
                  results.put("Partition Columns", partitionColumns.map(

Contributor Author

panbingkun Nov 12, 2023

Done

Contributor

cloud-fan commented Nov 12, 2023

unfortunately this has conflicts now...


          [SPARK-33393][SQL] Support SHOW TABLE EXTENDED in v2

ef4213a

Contributor Author

panbingkun commented Nov 12, 2023

unfortunately this has conflicts now...

Done, I have resolved these conflicts.


          Trigger build

d318f6b

Contributor Author

panbingkun commented Nov 16, 2023 •

edited

Loading

@cloud-fan If you have time, could you please take a look at this PR? Thank you very much!

cloud-fan approved these changes

View reviewed changes

Contributor

cloud-fan commented Nov 16, 2023

thanks, merging to master!

cloud-fan closed this in

c91dbde

Contributor Author

panbingkun commented Nov 16, 2023 •

edited

Loading

thanks, merging to master!

Thank for all reviewing and great help again @cloud-fan @MaxGekk @beliefer @LuciferYang ❤️❤️❤️

cccs-jc mentioned this pull request

[Bug] Improve dbt-spark table listing dbt-labs/dbt-adapters#862

Open

11 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

SQL