Skip to content

[SPARK-29592][SQL] ALTER TABLE (set partition location) should look up catalog/table like v2 commands #26304

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

Closed
imback82 wants to merge 3 commits into from
Closed

[SPARK-29592][SQL] ALTER TABLE (set partition location) should look up catalog/table like v2 commands #26304

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
400525e
initial commit
imback82 Oct 30, 2019
bc76fdf
Fix test
imback82 Oct 30, 2019
1f59072
Address PR comments
imback82 Oct 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix test
  • Loading branch information
@imback82
imback82 committed Oct 30, 2019
commit bc76fdf75b6da6f06d52201338c5817e1b8a90dd
15 changes: 11 additions & 4 deletions sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,14 +136,21 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils {
assertNoSuchTable(s"ALTER TABLE $viewName SET SERDE 'whatever'")
assertNoSuchTable(s"ALTER TABLE $viewName PARTITION (a=1, b=2) SET SERDE 'whatever'")
assertNoSuchTable(s"ALTER TABLE $viewName SET SERDEPROPERTIES ('p' = 'an')")
assertNoSuchTable(s"ALTER TABLE $viewName PARTITION (a='4') SET LOCATION '/path/to/home'")
assertNoSuchTable(s"ALTER TABLE $viewName ADD IF NOT EXISTS PARTITION (a='4', b='8')")
assertNoSuchTable(s"ALTER TABLE $viewName DROP PARTITION (a='4', b='8')")
assertNoSuchTable(s"ALTER TABLE $viewName PARTITION (a='4') RENAME TO PARTITION (a='5')")
assertNoSuchTable(s"ALTER TABLE $viewName RECOVER PARTITIONS")

// For v2 ALTER TABLE statements, we have better error message saying view is not supported.
assertViewNotSupported(s"ALTER TABLE $viewName SET LOCATION '/path/to/your/lovely/heart'")
assertAnalysisExceptionThrown(
s"ALTER TABLE $viewName SET LOCATION '/path/to/your/lovely/heart'",
s"'$viewName' is a view not a table")

// For the following v2 ALERT TABLE statements, unsupported operations are checked first
// before resolving the relations.
assertAnalysisExceptionThrown(
s"ALTER TABLE $viewName PARTITION (a='4') SET LOCATION '/path/to/home'",
"ALTER TABLE SET LOCATION does not support partition for v2 tables")
Copy link
Contributor Author

@imback82 imback82 Oct 30, 2019

Choose a reason for hiding this comment

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

@cloud-fan, If we need to retain the same behavior to throw an exception with "'$viewName' is a view not a table" message, we need to pass partitionSpec to AlterTable, then check if partitionSpec is set inside AlterTableExec. I don't think it's necessary, but please let me know if that is preferred.

Copy link
Contributor

@cloud-fan cloud-fan Oct 30, 2019

Choose a reason for hiding this comment

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

looks reasonable to me

Copy link
Contributor Author

@imback82 imback82 Oct 30, 2019

Choose a reason for hiding this comment

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

I assume you meant the current approach looks reasonable, right?

Copy link
Contributor

@cloud-fan cloud-fan Oct 30, 2019

Choose a reason for hiding this comment

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

yea, the unsupported feature should be reported first.

}
}

Expand Down Expand Up @@ -177,9 +184,9 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils {
}
}

private def assertViewNotSupported(query: String): Unit = {
private def assertAnalysisExceptionThrown(query: String, message: String): Unit = {
val e = intercept[AnalysisException](sql(query))
assert(e.message.contains("'testView' is a view not a table"))
assert(e.message.contains(message))
}

test("error handling: insert/load/truncate table commands against a view") {
Expand Down