Spark: Support altering view properties #9582

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

Merged

nastra merged 2 commits into apache:main from nastra:spark-view-alter-properties-support

Feb 1, 2024

Contributor

nastra commented Jan 29, 2024

This adds support for the below ALTER cases as defined in https://spark.apache.org/docs/latest/sql-ref-syntax-ddl-alter-view.html:

ALTER VIEW <viewName> SET TBLPROPERTIES (...)
ALTER VIEW <viewName> UNSET TBLPROPERTIES (...)

nastra requested a review from rdblue

January 29, 2024 10:00

github-actions bot added the spark label

nastra added this to the Iceberg 1.5.0 milestone

nastra mentioned this pull request

Spark: Throw exception on ALTER VIEW <viewName> AS <query> #9510

Merged

rdblue reviewed

View reviewed changes

...v3.5/spark-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckViews.scala Outdated

    
                      verifyColumnCount(ident, columnAliases, query)

                      SchemaUtils.checkColumnNameDuplication(query.schema.fieldNames, SQLConf.get.resolver)

                    case UnsetViewProperties(ResolvedV2View(catalog, ident), propertyKeys, ifExists) =>

Contributor

rdblue Jan 30, 2024

Check rules are intended to ensure that SQL is correct and valid, not to check the execution. If you have this check here, it would fail when you run EXPLAIN ALTER VIEW ... because this is doing execution work (checking for the property) in the planner. Another way to think about this is that this isn't an analysis failure, it is a runtime failure due to data. The SQL is perfectly valid.

This should either be done in AlterV2ViewExec or not at all. I would lean toward not doing this check at all. Shouldn't this be idempotent? Is there value in failing to remove a property if it isn't there? I don't see value so I'd ignore this and let the catalog throw an exception if it chooses to.

Contributor

rdblue Jan 30, 2024

Okay, I missed the ifExists part. looks like this behavior is required by Spark.

In that case, this should be moved to the Exec plan.

rdblue reviewed

View reviewed changes

...k-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/RewriteViewCommands.scala Outdated Show resolved Hide resolved

rdblue reviewed

View reviewed changes

...k-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/RewriteViewCommands.scala Show resolved Hide resolved

rdblue reviewed

View reviewed changes

spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkCatalog.java

    
                  if (null != asViewCatalog) {

                    try {

                      org.apache.iceberg.view.View view = asViewCatalog.loadView(buildIdentifier(ident));

                      UpdateViewProperties updateViewProperties = view.updateProperties();

Contributor

rdblue Jan 30, 2024

Can you also reject reserved properties? I don't think that you should be able to set format-version, location, or provider.

Contributor Author

nastra Jan 31, 2024

this is a miss on my end. For some reason reason I manually checked provider / location (which Spark would prevent from being set/unset) and assumed Spark would do this for all reserved properties.

I've added some tests and fixed this. Note that I'm rejecting both setting/removing reserved properties as it seems a bit confusing from a user's perspective if Spark rejects setting/removing certain properties but we only reject setting those.
It also appears that the behavior for tables is slightly different, where we're only rejecting setting reserved table properties, but we're not rejecting the removal of reserved table properties.

rdblue requested changes

View reviewed changes

Contributor

rdblue left a comment

Mostly looks good. Just two major things:

Checking if a property to unset exists should be done in the exec node
Iceberg should fail if the caller attempts to set reserved properties

nastra force-pushed the spark-view-alter-properties-support branch 2 times, most recently from 06461fe to 54a6abe Compare

January 31, 2024 10:25

nastra commented

View reviewed changes

spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java Outdated

    
                      .hasMessageContaining(

                          "The feature is not supported: location is a reserved table property");

                  sql("ALTER VIEW %s UNSET TBLPROPERTIES ('format-version')", viewName);

Contributor Author

nastra Jan 31, 2024

@rdblue it feels like maybe we should also prevent removing reserverd properties to be more explicit (even though they would be set by default when the view is loaded)? But I also see that we don't do this when removing reserved properties from a table

Contributor

rdblue Jan 31, 2024

I agree. You should not be able to unset these properties.

nastra force-pushed the spark-view-alter-properties-support branch 2 times, most recently from a2e312a to 32bc1d3 Compare

January 31, 2024 10:44

nastra requested a review from rdblue

January 31, 2024 10:52

nastra force-pushed the spark-view-alter-properties-support branch from 32bc1d3 to 646a645 Compare

January 31, 2024 11:52

rdblue reviewed

View reviewed changes

spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java

    
                          () -> sql("ALTER VIEW %s SET TBLPROPERTIES ('location' = 'random_location')", viewName))

                      .isInstanceOf(AnalysisException.class)

                      .hasMessageContaining(

                          "The feature is not supported: location is a reserved table property");

Contributor

rdblue Jan 31, 2024

Nit: Just because Spark has a bad error message doesn't mean Iceberg should. We could make this conform to our standard, which is more clear: "Cannot set reserved property: location"

Contributor

rdblue Jan 31, 2024

Ah, I see this is coming from Spark. I think the other messages (that we generate) could also improve.

Contributor Author

nastra Jan 31, 2024

yeah we don't have much control over the error msg coming from Spark unfortunately

apache deleted a comment from nastra

rdblue reviewed

View reviewed changes

spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java Outdated

    
                          () -> sql("ALTER VIEW %s SET TBLPROPERTIES ('format-version' = '99')", viewName))

                      .isInstanceOf(UnsupportedOperationException.class)

                      .hasMessageContaining(

                          "Cannot specify 'format-version' because it's a reserved view property");

Contributor

rdblue Jan 31, 2024

How about Cannot set reserved property: %s? That's more direct and also uses the same verb that the caller used, "set".

rdblue reviewed

View reviewed changes

spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java Outdated

    
                  assertThatThrownBy(() -> sql("ALTER VIEW %s UNSET TBLPROPERTIES ('unknown-key')", viewName))

                      .isInstanceOf(AnalysisException.class)

                      .hasMessageContaining("Attempted to unset non-existing property 'unknown-key'");

Contributor

rdblue Jan 31, 2024

If this isn't Spark, we should fix the message: Cannot remove property that is not set: %s

rdblue reviewed

View reviewed changes

spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java Outdated

    
                  assertThatThrownBy(() -> sql("ALTER VIEW %s UNSET TBLPROPERTIES ('format-version')", viewName))

                      .isInstanceOf(UnsupportedOperationException.class)

                      .hasMessageContaining(

                          "Cannot specify 'format-version' because it's a reserved view property");

Contributor

rdblue Jan 31, 2024

Same as above, it would be nice to improve the error message.

rdblue reviewed

View reviewed changes

spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/source/SparkView.java Outdated Show resolved Hide resolved

rdblue approved these changes

View reviewed changes

Contributor

rdblue left a comment

Looks great! I have some minor style comments mostly about error messages.

nastra added 2 commits

February 1, 2024 08:20


          Spark: Support altering view properties

8be4286

This adds support for the below `ALTER` cases as defined in https://spark.apache.org/docs/latest/sql-ref-syntax-ddl-alter-view.html:
* `ALTER VIEW <viewName> SET TBLPROPERTIES (...)`
* `ALTER VIEW <viewName> UNSET TBLPROPERTIES (...)`


          Improve error msgs and rename queryColumnNames property

9e3eb5a

nastra force-pushed the spark-view-alter-properties-support branch from 646a645 to 9e3eb5a Compare

February 1, 2024 07:32

Contributor Author

nastra commented Feb 1, 2024

Thanks for the review @rdblue, I've updated the error msgs and also renamed the internal property to spark.query-column-names

nastra merged commit 9c8e9ba into apache:main

nastra deleted the spark-view-alter-properties-support branch

February 1, 2024 08:20

nastra mentioned this pull request

Spark 3.4: Support altering view properties #9610

Merged

nastra mentioned this pull request

Site: Update release notes for 1.5.0 #9835

Merged

devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request


          Spark: Support altering view properties (apache#9582)

e99a813

This adds support for the below `ALTER` cases as defined in https://spark.apache.org/docs/latest/sql-ref-syntax-ddl-alter-view.html:
* `ALTER VIEW <viewName> SET TBLPROPERTIES (...)`
* `ALTER VIEW <viewName> UNSET TBLPROPERTIES (...)`

nastra self-assigned this

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

Labels