Spark: Support renaming views #9343
Conversation
nastra
force-pushed
the
spark-view-rename
branch
from
7ee161c to
4be0bfe
Compare
nastra
force-pushed
the
spark-view-rename
branch
2 times, most recently
from
951f811 to
122c23c
Compare
| .map(_ => ResolvedV2View(catalog.asViewCatalog, ident)) | ||
| .getOrElse(u) | ||
|
|
||
| case RenameTable(ResolvedV2View(oldCatalog, oldIdent), CatalogAndIdentifier(newCatalog, newIdent), true) => |
There was a problem hiding this comment.
What is true? Can we add a name to this so that it is more readable?
There was a problem hiding this comment.
Is it possible for the rule on line 61 to resolve the multi-part to identifier before this rule runs?
There was a problem hiding this comment.
I added this to isView@true so that it's clearer. I checked the Spark code and the to identifier isn't resolved anywhere and just passed as-is when doing a rename
| String.format( | ||
| "SELECT spark_catalog.system.bucket(100, 'a') AS bucket_result, 'a' AS value", | ||
| catalogName); | ||
| String sql = "SELECT spark_catalog.system.bucket(100, 'a') AS bucket_result, 'a' AS value"; |
nastra
force-pushed
the
spark-view-rename
branch
from
122c23c to
cab9f16
Compare
nastra
force-pushed
the
spark-view-rename
branch
2 times, most recently
from
9ca1a5d to
7565976
Compare
nastra
force-pushed
the
spark-view-rename
branch
from
7565976 to
9f9d6b5
Compare
| .map(createViewRelation(parts, _)) | ||
| .getOrElse(u) | ||
|
|
||
| case u@UnresolvedTableOrView(CatalogAndIdentifier(catalog, ident), _, _) => |
There was a problem hiding this comment.
Do we need to check whether this matches temp views?
.../main/scala/org/apache/spark/sql/execution/datasources/v2/ExtendedDataSourceV2Strategy.scala
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| public void renameView() throws NoSuchTableException { |
There was a problem hiding this comment.
Can you add tests for names that reference temp views? I think we want to know what Spark does. My initial opinion (which could change if Spark does something else!) is that it should fail because the reference is ambiguous.
There was a problem hiding this comment.
I added a test that renames a view which is hidden by a temp view. The behavior is in-line with what we've seen when reading from a view that is hidden by a temp view. Doing a rename will first rename the temp view and re-executing the same rename will then do it for the Iceberg view.
I think for now we don't need any additional checks when handling UnresolvedTableOrView in ResolveViews
rdblue
left a comment
rdblue
left a comment
There was a problem hiding this comment.
I think this is ready to go, but we should double-check how temp views are handled.
|
|
||
| ViewCatalog viewCatalog = viewCatalog(); | ||
|
|
||
| viewCatalog |
There was a problem hiding this comment.
The SQL tests will be more cleaner if we support the CREATE VIEW SQL first before RENAME VIEW SQL?
There was a problem hiding this comment.
@rdblue and I talked about this and view creation via SQL requires some additional complexity, so we decided to get a few other smaller PRs/functionality in. #9421 is introducing some of that complexity and once #9421 is in, adding view creation via SQL should be much smaller in terms of complexity/diffset.
There was a problem hiding this comment.
No, I don't think this is a good direction. We want the tests to be independent. This allows us to more easily test just rename.
3 participants
This is based on #9340 and adds support for resolving and renaming views:
ALTER VIEW view_identifier RENAME TO view_identifier