nastra commented on code in PR #7992: URL: https://github.com/apache/iceberg/pull/7992#discussion_r1254388779
########## api/src/main/java/org/apache/iceberg/view/UpdateViewRepresentation.java: ########## @@ -0,0 +1,64 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.view; + +import org.apache.iceberg.PendingUpdate; +import org.apache.iceberg.catalog.Namespace; + +/** + * API for updating a view representation. + * + * <p>Apply returns the updated view for validation. + * + * <p>When committing, these changes will be applied to the current view metadata. Commit conflicts + * will be resolved by applying the pending changes to the new view metadata. + */ +public interface UpdateViewRepresentation extends PendingUpdate<ViewVersion> { Review Comment: So I think the current issue is the semantics of `.replace()` in Iceberg (or maybe I just understood them wrong). While a `CREATE OR REPLACE` is clear from an engine's perspective, I don't think the semantics are clear enough from Iceberg's perspective at the Builder level when calling `createOrReplace()`. ## View representations & replace In #7880 I've implemented `.replace()` as a full replace, meaning that if you had a previous representation for `spark`, it would get completely replaced with whatever new representation was defined (which can be seen [here](url)). In hindsight I think Iceberg's semantics for `.replace()` should keep previous view representations, so that we get the following behavior: ``` // create the view when executing "CREATE OR REPLACE VIEW ..." from Spark View view = catalog.buildView(identifier) .withSchema(schema) .withQuery("spark", "select a,b from ns.tbl") .createOrReplace() // replace the view when executing "CREATE OR REPLACE VIEW ..." from Trino view = catalog.buildView(identifier) .withSchema(schema) .withQuery("trino", "select a,b from ns.tbl") .createOrReplace() ``` Checking the representations of the current version of the view would contain both representations ``` // this would contain both representations assertThat(view.currentVersion().representations()).hasSize(2) ``` ## View Properties & replace To have the same semantics as with view representations during a `replace`, we should keep previous properties, so that `view.properties()` would return `{"key1": "val1", "key2": "val2"}` after executing the below code: ``` // create the view with a property View view = catalog.buildView(identifier) .withSchema(schema) .withQuery("spark", "select a,b from ns.tbl") .withProperty("key1", "val1") .createOrReplace() // replace the view view = catalog.buildView(identifier) .withSchema(schema) .withQuery("trino", "select a,b from ns.tbl") .withProperty("key2", "val2") .createOrReplace() ``` #7880 currently handles properties during a `replace` as an actual replace and doesn't keep previous properties. ## TLDR The semantics of `.replace()` at the View API level should mean that previously configured representations and properties are carried over. That being said, I think if we use these semantics, then we can probably remove the `UpdateViewRepresentation` API (in case we'd ever want to drop a view representation for a particular dialect, then we could add an API that does that) -- 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]
