huaxingao commented on code in PR #14984:
URL: https://github.com/apache/iceberg/pull/14984#discussion_r3696219727
##########
spark/v4.2/spark-extensions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/IcebergAlterV2ViewUnsetPropertiesExec.scala:
##########
@@ -35,19 +44,38 @@ case class AlterV2ViewUnsetPropertiesExec(
override lazy val output: Seq[Attribute] = Nil
override protected def run(): Seq[InternalRow] = {
+ val viewInfo = catalog.loadView(ident)
+ val properties = viewInfo.properties.asScala.toMap
+
if (!ifExists) {
-
propertyKeys.filterNot(catalog.loadView(ident).properties.containsKey).foreach
{ property =>
+ propertyKeys.filterNot(properties.contains).foreach { property =>
throw new IcebergAnalysisException(s"Cannot remove property that is
not set: '$property'")
}
}
- val changes = propertyKeys.map(ViewChange.removeProperty)
- catalog.alterView(ident, changes: _*)
+ propertyKeys.foreach(verifyNonReservedPropertyIsUnset)
+
+ ViewUtil.icebergViewCatalog(catalog, ident) match {
+ case Some(icebergViewCatalog) =>
+ val view =
icebergViewCatalog.loadView(Spark3Util.identifierToTableIdentifier(ident))
+ val update = view.updateProperties()
+ propertyKeys.foreach(update.remove)
+ update.commit()
+
+ case _ =>
Review Comment:
This `case _ => replaceView(...)` fallback was dropped from
`IcebergAlterV2ViewSetPropertiesExec` but is still here (and in
`IcebergAlterV2ViewSchemaBindingExec`:55). It's effectively unreachable, and if
it ever fired, `replaceView` would bump the view version instead of doing a
property-only update. Could we use the same `getOrElse(throw ...)` pattern as
`SetProperties` here (and in `SchemaBindingExec`) for consistency?
##########
spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/BaseCatalog.java:
##########
@@ -99,6 +103,45 @@ public boolean useNullableQuerySchema() {
return useNullableQuerySchema;
}
+ @Override
+ public Relation loadRelation(Identifier ident) throws NoSuchTableException {
+ try {
+ return loadTable(ident);
+ } catch (NoSuchTableException e) {
+ try {
+ return loadView(ident);
+ } catch (NoSuchViewException viewException) {
+ e.addSuppressed(viewException);
+ throw e;
+ }
+ }
+ }
+
+ protected View normalizeViewCurrentCatalog(String catalogName, View view) {
+ if (view == null || !Objects.equals(catalogName, view.currentCatalog())) {
+ return view;
+ }
+
+ View.Builder builder =
+ new View.Builder()
+ .withQueryText(view.queryText())
+ .withCurrentNamespace(view.currentNamespace())
+ .withSchema(view.schema())
+ .withSchemaMode(view.schemaMode())
Review Comment:
The three `View.Builder` rebuild sites handle null
`schemaMode`/`sqlConfigs`/`viewDependencies` inconsistently:
`SparkView`.`toView` guards them, but here `.withSchemaMode(view.schemaMode())`
is unconditional, and `ViewUtil.withProperties` (ViewUtil.scala:88,90) passes
`schemaMode` and `viewDependencies` through unconditionally too. Rather than
patching each spot, could we extract the optional-field application into one
small shared helper (e.g. `SparkView.applyOptionalFields(builder, schemaMode,
sqlConfigs, dependencies)` with a `!= null` guard on each) that all three call?
That makes the null-handling a single source of truth instead of three copies
that drifted.
##########
spark/v4.2/spark-extensions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ExtendedDataSourceV2Strategy.scala:
##########
@@ -135,43 +140,69 @@ case class ExtendedDataSourceV2Strategy(spark:
SparkSession) extends Strategy wi
query,
columnAliases,
columnComments,
- queryColumnNames,
comment,
+ collation,
properties,
allowExisting,
replace,
+ viewSchemaMode,
_,
_) =>
+ val engineVersion = "Spark " + org.apache.spark.SPARK_VERSION
+ val viewProperties =
+ properties ++ Map(
+ SparkView.PROP_CREATE_ENGINE_VERSION -> engineVersion,
+ SparkView.PROP_ENGINE_VERSION -> engineVersion)
Review Comment:
These two engine-version props are added here, but on the create path
they're immediately discarded and regenerated.
`CreateV2ViewExec.buildViewInfo()` stamps them into the View via
.`withProperties(userProperties.asJava)`, then `catalog.createView(...)`
reaches `SparkCatalog.commitView` → `ViewUtil.createProperties`, which strips
reserved props at `ViewUtil.scala`:47 (including
create_engine_version/engine_version) and re-adds them at :50–51 with the same
"Spark " + SPARK_VERSION value. So the addition here is effectively dead for
Iceberg views. Could we drop it to avoid the "set here, stripped there,
re-added" round-trip?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
