Skip to content
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

Spark: Add support for describing/showing views #9513

Merged
merged 1 commit into from
Jan 31, 2024
Merged

Spark: Add support for describing/showing views #9513

merged 1 commit into from
Jan 31, 2024

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Jan 18, 2024 •
edited
Loading

This adds support for:

  • DESCRIBE <viewName> / DESCRIBE EXTENDED <viewName>
  • SHOW VIEWS / SHOW VIEWS LIKE <pattern>
  • SHOW TBLPROPERTIES <viewName>
  • SHOW CREATE TABLE <viewName>

@nastra nastra added this to the Iceberg 1.5.0 milestone Jan 18, 2024
@nastra nastra marked this pull request as draft January 18, 2024 14:16
@github-actions github-actions bot added the spark label Jan 18, 2024
@nastra nastra force-pushed the spark-view-describe-support branch 2 times, most recently from 21c494f to eb629be Compare January 19, 2024 15:41
@nastra nastra force-pushed the spark-view-describe-support branch 2 times, most recently from 9d20e42 to 89aa0e7 Compare January 23, 2024 13:22
@nastra nastra marked this pull request as ready for review January 25, 2024 10:30
@nastra nastra force-pushed the spark-view-describe-support branch from 89aa0e7 to da61115 Compare January 26, 2024 17:42
rdblue
rdblue reviewed Jan 28, 2024
View reviewed changes
...ions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ShowCreateV2ViewExec.scala Outdated
}

private def showColumns(view: View, builder: StringBuilder): Unit = {
val columns = view.schema().fields
Copy link
Contributor

@rdblue rdblue Jan 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This should only be produced if it was part of the original CREATE VIEW command. I think we probably need to store these somewhere in ViewVersion. In addition, if they are set then the schema should be validated against them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran into this case when I was implementing this so I compared this with how SHOW CREATE TABLE <tbl> does it. Here it can be seen that it just takes the latest schema of the table and uses it for the output. So should we be aligning SHOW CREATE VIEW with SHOW CREATE TABLE here?

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 this is fine for now, but we should consider how to handle the aliases in the future

@nastra nastra force-pushed the spark-view-describe-support branch 3 times, most recently from aa786db to c3b1165 Compare January 29, 2024 12:37
rdblue
rdblue reviewed Jan 30, 2024
View reviewed changes
...k-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/RewriteViewCommands.scala
@@ -60,6 +63,13 @@ case class RewriteViewCommands(spark: SparkSession) extends Rule[LogicalPlan] wi
properties = properties,
allowExisting = allowExisting,
replace = replace)

case ShowViews(UnresolvedNamespace(Seq()), pattern, output) if isViewCatalog(catalogManager.currentCatalog) =>
ShowIcebergViews(ResolvedNamespace(catalogManager.currentCatalog, Seq.empty), pattern, output)
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this case be handled the same way by running CatalogAndNamespace.unapply(Seq()) in the next rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately no, as CatalogAndNamespace requires the namespace to be non-empty as can be seen in https://github.com/apache/spark/blob/abaec2804fb48d2d378f7c3e99733af97283fa22/sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/LookupCatalog.scala#L84. Therefore we need to handle the case with an empty namespace separately here (the same is done in Spark)

rdblue
rdblue reviewed Jan 30, 2024
View reviewed changes
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java
row(NAMESPACE.toString(), "prefixV2", false),
row(NAMESPACE.toString(), "prefixV3", false),
row(NAMESPACE.toString(), "v1", false),
tempView);
Copy link
Contributor

Choose a reason for hiding this comment

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

I trust you that this is Spark's behavior. But this is weird because it isn't in the catalog.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also weird when filtering by NAMESPACE below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is the expected Spark behavior as can be seen in https://spark.apache.org/docs/3.5.0/sql-ref-syntax-aux-show-views.html. Spark always lists temp views (which don't belong to any catalog)

@rdblue
Copy link
Contributor

rdblue commented Jan 30, 2024

I pointed out a few things that I think I would change, but there is nothing blocking this. +1 overall. Please fix what you agree with (including the SHOW VIEWS behavior with temp views) and merge when you're ready!

@nastra
Spark: Add support for describing/showing views
8a6063f 
This adds support for:
* `DESCRIBE <viewName>` / `DESCRIBE EXTENDED <viewName>`
* `SHOW VIEWS` / `SHOW VIEWS LIKE <pattern>`
* `SHOW TBLPROPERTIES <viewName>`
* `SHOW CREATE TABLE <viewName>`
@nastra nastra force-pushed the spark-view-describe-support branch from c3b1165 to 8a6063f Compare January 31, 2024 08:14
@nastra
Copy link
Contributor Author

nastra commented Jan 31, 2024

thank for reviewing @rdblue, I addressed everything and will go ahead and merge this

@nastra nastra merged commit d5a9d34 into apache:main Jan 31, 2024
31 checks passed
@nastra nastra deleted the spark-view-describe-support branch January 31, 2024 09:04
@nastra nastra mentioned this pull request Jan 31, 2024
Merged
@nastra nastra mentioned this pull request Mar 11, 2024
Merged
devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024
@nastra @devangjhabakh
Spark: Add support for describing/showing views (apache#9513)
8ca462b 
This adds support for:
* `DESCRIBE <viewName>` / `DESCRIBE EXTENDED <viewName>`
* `SHOW VIEWS` / `SHOW VIEWS LIKE <pattern>`
* `SHOW TBLPROPERTIES <viewName>`
* `SHOW CREATE TABLE <viewName>`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rdblue rdblue rdblue approved these changes

Assignees
No one assigned
Labels
spark
Projects
View support
Status: Done
Milestone
Iceberg 1.5.0
Development

Successfully merging this pull request may close these issues.
2 participants
@nastra @rdblue