You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2023/07/05 12:59:37 UTC
[GitHub] [iceberg] nastra opened a new pull request, #7992: Core: Simplify/improve View APIs
nastra opened a new pull request, #7992:
URL: https://github.com/apache/iceberg/pull/7992
(no comment)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1264760426
##########
api/src/main/java/org/apache/iceberg/view/ViewVersion.java:
##########
@@ -74,6 +76,14 @@ default String operation() {
/** The query output schema at version create time, without aliases */
int schemaId();
+ /** The default catalog when the view is created. */
+ @Nullable
+ String defaultCatalog();
+
+ /** The default namespace to use when the SQL does not contain a namespace. */
+ @Nullable
Review Comment:
Is this optional? What if you don't set it? Does resolution fail at runtime for any unqualified table names?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1253437549
##########
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:
I think we need a better name than `UpdateViewRepresentation`. That doesn't capture what is happening.
I'm also not sure we even need this in the initial API. Are these operations that people will need to use? What is the SQL that exercises them or what is the use case?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1274204374
##########
format/view-spec.md:
##########
@@ -37,6 +37,8 @@ Each compute engine stores the metadata of the view in its proprietary format in
View metadata storage mirrors how Iceberg table metadata is stored and retrieved. View metadata is maintained in metadata files. All changes to view state create a new view metadata file and completely replace the old metadata using an atomic swap. Like Iceberg tables, this atomic swap is delegated to the metastore that tracks tables and/or views by name. The view metadata file tracks the view schema, custom properties, current and past versions, as well as other metadata.
+When view metadata is replaced, then previously set properties are carried over.
+
Review Comment:
```suggestion
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue merged PR #7992:
URL: https://github.com/apache/iceberg/pull/7992
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#issuecomment-1650676760
> Once both PRs are merged, I'll add a follow-up and mention in the docs that @Value.Immutable usage is discouraged on iceberg-api (with a checkstyle rule that enforces it) and will also remove the Immutable dependency
I think we should disallow it, not just discourage it. A checkstyle rule sounds like the right approach though, so maybe we're saying the same thing.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1259292615
##########
api/src/main/java/org/apache/iceberg/view/ViewBuilder.java:
##########
@@ -18,18 +18,16 @@
*/
package org.apache.iceberg.view;
-import java.util.List;
import java.util.Map;
import org.apache.iceberg.Schema;
-import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.ViewCatalog;
/**
* A builder used to create or replace a SQL {@link View}.
*
* <p>Call {@link ViewCatalog#buildView} to create a new builder.
*/
-public interface ViewBuilder {
+public interface ViewBuilder extends VersionBuilder<ViewBuilder> {
Review Comment:
At this point I'm not entirely sure whether we'd need to have a `withLocation()` method here. Should we wait until we get to https://github.com/apache/iceberg/pull/7913 and we can re-evaluate and add it when necessary?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1274206534
##########
format/view-spec.md:
##########
@@ -75,13 +77,17 @@ Notes:
Each version in `versions` is a struct with the following fields:
-| Requirement | Field name | Description |
-|-------------|-------------------|--------------------------------------------------------------------------|
-| _required_ | `version-id` | ID for the version |
-| _required_ | `schema-id` | ID of the schema for the view version |
-| _required_ | `timestamp-ms` | Timestamp when the version was created (ms from epoch) |
-| _required_ | `summary` | A string to string map of [summary metadata](#summary) about the version |
-| _required_ | `representations` | A list of [representations](#representations) for the view definition |
+| Requirement | Field name | Description |
+|-------------|---------------------|-------------------------------------------------------------------------------|
+| _required_ | `version-id` | ID for the version |
+| _required_ | `schema-id` | ID of the schema for the view version |
+| _required_ | `timestamp-ms` | Timestamp when the version was created (ms from epoch) |
+| _required_ | `summary` | A string to string map of [summary metadata](#summary) about the version |
+| _required_ | `representations` | A list of [representations](#representations) for the view definition |
+| _optional_ | `default-catalog` | Catalog name to use when a reference in the SELECT does not contain a catalog |
+| _optional_ | `default-namespace` | Namespace to use when a reference in the SELECT is a single identifier |
+
+When writing a new view version, all the information from the previous view version is replaced, meaning that `representations` / `default-catalog` / `default-namespace` / `summary` are not carried over.
Review Comment:
```suggestion
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1274204530
##########
format/view-spec.md:
##########
@@ -75,13 +77,17 @@ Notes:
Each version in `versions` is a struct with the following fields:
-| Requirement | Field name | Description |
-|-------------|-------------------|--------------------------------------------------------------------------|
-| _required_ | `version-id` | ID for the version |
-| _required_ | `schema-id` | ID of the schema for the view version |
-| _required_ | `timestamp-ms` | Timestamp when the version was created (ms from epoch) |
-| _required_ | `summary` | A string to string map of [summary metadata](#summary) about the version |
-| _required_ | `representations` | A list of [representations](#representations) for the view definition |
+| Requirement | Field name | Description |
+|-------------|---------------------|-------------------------------------------------------------------------------|
+| _required_ | `version-id` | ID for the version |
+| _required_ | `schema-id` | ID of the schema for the view version |
+| _required_ | `timestamp-ms` | Timestamp when the version was created (ms from epoch) |
+| _required_ | `summary` | A string to string map of [summary metadata](#summary) about the version |
+| _required_ | `representations` | A list of [representations](#representations) for the view definition |
+| _optional_ | `default-catalog` | Catalog name to use when a reference in the SELECT does not contain a catalog |
+| _optional_ | `default-namespace` | Namespace to use when a reference in the SELECT is a single identifier |
Review Comment:
This should not be optional.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1253521377
##########
api/src/main/java/org/apache/iceberg/view/View.java:
##########
@@ -83,4 +83,13 @@ public interface View {
* @return a new {@link UpdateViewProperties}
*/
UpdateViewProperties updateProperties();
+
+ /**
+ * Create a new {@link UpdateViewRepresentation} to update the view's representations.
+ *
+ * @return a new {@link UpdateViewRepresentation}
+ */
+ default UpdateViewRepresentation updateRepresentation() {
Review Comment:
I think we want a `newViewVersion()` method instead of this. It should have the same API as the `ViewBuilder`, probably sharing a `ViewVersionBuilder` interface.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1254086434
##########
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:
> I'm also not sure we even need this in the initial API. Are these operations that people will need to use? What is the SQL that exercises them or what is the use case?
The main use case would be to have an API to update a view's representation without having to reconstruct all other representations. Say you have representations for `trino` and `spark` and you'd want to only update the SQL definition of `spark`. In a normal `replace` you would replace the existing view completely with the "new" information, but you don't want to lose the representation for `trino`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#issuecomment-1650693714
I had a few comments on the spec that I added as changes and committed. +1 now, so I'll merge when tests are passing.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1256347597
##########
api/src/main/java/org/apache/iceberg/view/ReplaceViewVersion.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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;
+
+/**
+ * API for replacing a view's version.
+ *
+ * <p>Apply returns the updated view version 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 ReplaceViewVersion
+ extends PendingUpdate<ViewVersion>, VersionBuilder<ReplaceViewVersion> {
+
+ /**
+ * Remove the representation for the given dialect from the view's version.
+ *
+ * @param dialect The dialect for which the view representation should be removed.
+ * @return this for method chaining
+ */
+ ReplaceViewVersion remove(String dialect);
Review Comment:
I don't think there's a dialect to remove. Everything must be added specifically for this API right?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#issuecomment-1621822842
I am actually surprised that RevAPI doesn't fail on those changes. It appears that this is related to to recent Gradle version bump in #7955
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1253522782
##########
format/view-spec.md:
##########
@@ -144,13 +143,14 @@ This create statement would produce the following `sql` representation metadata:
| `type` | `"sql"` |
| `sql` | `"SELECT\n COUNT(1), CAST(event_ts AS DATE)\nFROM events\nGROUP BY 2"` |
| `dialect` | `"spark"` |
-| `default-catalog` | `"prod"` |
| `default-namespace` | `["default"]` |
-| `field-aliases` | `["event_count", "event_date"]` |
-| `field-comments` | `["Count of events", null]` |
If a create statement does not include column names or comments before `AS`, the fields should be omitted.
+The `event_count` (with the `Count of events` comment) and `event_date` field aliases must be part of the view's `schema`.
Review Comment:
The view version's schema.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1253524674
##########
format/view-spec.md:
##########
@@ -101,6 +101,8 @@ A view version can have more than one representation. All representations for a
View versions are immutable. Once a version is created, it cannot be changed. This means that representations for a version cannot be changed. If a view definition changes (or new representations are to be added), a new version must be created.
+There should be no more than one view representation per dialect.
Review Comment:
Dialect is not something that can be used here because it is defined in the SQL representation. There is no "dialect" for the view version and there could be multiple dialects from multiple SQL representations.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1259290379
##########
api/src/main/java/org/apache/iceberg/view/VersionBuilder.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.catalog.Namespace;
+
+public interface VersionBuilder<T> {
+ /**
+ * Add a view representation for the given dialect and the SQL to the view.
+ *
+ * @param dialect The dialect of the view representation
+ * @param sql The SQL of the view representation
+ * @return this for method chaining
+ */
+ default T withQuery(String dialect, String sql) {
+ return withQuery(dialect, sql, null);
+ }
+
+ /**
+ * Add a view representation for the given dialect and the SQL to the view.
+ *
+ * @param dialect The dialect of the view representation
+ * @param sql The SQL of the view representation
+ * @param defaultNamespace The default namespace to use when the SQL does not contain a namespace
+ * @return this for method chaining
+ */
+ T withQuery(String dialect, String sql, Namespace defaultNamespace);
Review Comment:
sorry, I was under the impression we would do https://github.com/apache/iceberg/pull/7992#discussion_r1254112125. I've updated the API to properly reflect this and moved the default catalog to the `ViewVersion`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1274205352
##########
format/view-spec.md:
##########
@@ -75,13 +77,17 @@ Notes:
Each version in `versions` is a struct with the following fields:
-| Requirement | Field name | Description |
-|-------------|-------------------|--------------------------------------------------------------------------|
-| _required_ | `version-id` | ID for the version |
-| _required_ | `schema-id` | ID of the schema for the view version |
-| _required_ | `timestamp-ms` | Timestamp when the version was created (ms from epoch) |
-| _required_ | `summary` | A string to string map of [summary metadata](#summary) about the version |
-| _required_ | `representations` | A list of [representations](#representations) for the view definition |
+| Requirement | Field name | Description |
+|-------------|---------------------|-------------------------------------------------------------------------------|
+| _required_ | `version-id` | ID for the version |
+| _required_ | `schema-id` | ID of the schema for the view version |
+| _required_ | `timestamp-ms` | Timestamp when the version was created (ms from epoch) |
+| _required_ | `summary` | A string to string map of [summary metadata](#summary) about the version |
+| _required_ | `representations` | A list of [representations](#representations) for the view definition |
+| _optional_ | `default-catalog` | Catalog name to use when a reference in the SELECT does not contain a catalog |
+| _optional_ | `default-namespace` | Namespace to use when a reference in the SELECT is a single identifier |
+
+When writing a new view version, all the information from the previous view version is replaced, meaning that `representations` / `default-catalog` / `default-namespace` / `summary` are not carried over.
Review Comment:
This is also incorrect for the spec.
There is nothing preventing us from creating an operation that does exactly this carry over, as long as it is the user's intent. For example:
```sql
ALTER VIEW v ADD REPRESENTATION spark AS
SELECT * FROM foo
```
This SQL clearly **adds** to the existing representations. It isn't correct or incorrect to do that. It is simply the behavior that a SQL operation could have. The spec allows all these things.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1264947023
##########
.palantir/revapi.yml:
##########
@@ -743,6 +743,116 @@ acceptedBreaks:
new: "method java.util.List<org.apache.iceberg.UpdateRequirement> org.apache.iceberg.rest.requests.UpdateTableRequest::requirements()"
justification: "Signature changed to an interface, but this is safe because\
\ of type erasure and the original type is always returned"
+ "1.3.0":
+ org.apache.iceberg:iceberg-api:
+ - code: "java.method.addedToInterface"
+ new: "method java.lang.String org.apache.iceberg.view.ViewVersion::defaultCatalog()"
+ justification: "Acceptable break due to updating View APIs and the View Spec"
+ - code: "java.method.addedToInterface"
+ new: "method org.apache.iceberg.catalog.Namespace org.apache.iceberg.view.ViewVersion::defaultNamespace()"
+ justification: "Acceptable break due to updating View APIs and the View Spec"
+ - code: "java.method.addedToInterface"
+ new: "method org.apache.iceberg.view.ReplaceViewVersion org.apache.iceberg.view.View::replaceVersion()"
+ justification: "Acceptable break due to updating View APIs and the View Spec"
+ - code: "java.method.numberOfParametersChanged"
+ old: "method org.apache.iceberg.view.ViewBuilder org.apache.iceberg.view.ViewBuilder::withQuery(java.lang.String)"
+ new: "method T org.apache.iceberg.view.VersionBuilder<T>::withQuery(java.lang.String,\
+ \ java.lang.String) @ org.apache.iceberg.view.ViewBuilder"
+ justification: "Acceptable break due to updating View APIs and the View Spec"
+ - code: "java.method.removed"
+ old: "method java.lang.String org.apache.iceberg.view.ImmutableSQLViewRepresentation::defaultCatalog()"
Review Comment:
Immutables has a a way to configure the visibility or the package of the generated class, but unfortunately there's nothing currently available that would put the generated class into a different project :(
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1264761106
##########
.palantir/revapi.yml:
##########
@@ -743,6 +743,116 @@ acceptedBreaks:
new: "method java.util.List<org.apache.iceberg.UpdateRequirement> org.apache.iceberg.rest.requests.UpdateTableRequest::requirements()"
justification: "Signature changed to an interface, but this is safe because\
\ of type erasure and the original type is always returned"
+ "1.3.0":
+ org.apache.iceberg:iceberg-api:
+ - code: "java.method.addedToInterface"
+ new: "method java.lang.String org.apache.iceberg.view.ViewVersion::defaultCatalog()"
+ justification: "Acceptable break due to updating View APIs and the View Spec"
+ - code: "java.method.addedToInterface"
+ new: "method org.apache.iceberg.catalog.Namespace org.apache.iceberg.view.ViewVersion::defaultNamespace()"
+ justification: "Acceptable break due to updating View APIs and the View Spec"
+ - code: "java.method.addedToInterface"
+ new: "method org.apache.iceberg.view.ReplaceViewVersion org.apache.iceberg.view.View::replaceVersion()"
+ justification: "Acceptable break due to updating View APIs and the View Spec"
+ - code: "java.method.numberOfParametersChanged"
+ old: "method org.apache.iceberg.view.ViewBuilder org.apache.iceberg.view.ViewBuilder::withQuery(java.lang.String)"
+ new: "method T org.apache.iceberg.view.VersionBuilder<T>::withQuery(java.lang.String,\
+ \ java.lang.String) @ org.apache.iceberg.view.ViewBuilder"
+ justification: "Acceptable break due to updating View APIs and the View Spec"
+ - code: "java.method.removed"
+ old: "method java.lang.String org.apache.iceberg.view.ImmutableSQLViewRepresentation::defaultCatalog()"
Review Comment:
I think there's a problem: the generated Immutable classes should not be in the `api` module. All of the implementation classes for API interfaces are in core and it makes no sense to publicly expose these in API when we don't publicly expose the others.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1254112125
##########
api/src/main/java/org/apache/iceberg/view/SQLViewRepresentation.java:
##########
@@ -37,17 +36,7 @@ default String type() {
/** The view query SQL dialect. */
String dialect();
- /** The default catalog when the view is created. */
- @Nullable
- String defaultCatalog();
Review Comment:
We discussed this internally yesterday and we can
1) remove the `default-catalog`
2) leave it at the representation level, because e.g. Trino's default catalog can be different from Spark's default catalog. Also Trino stores the catalog in its [ViewDefinition](https://github.com/trinodb/trino/blob/e9ac9a1107cdbf4fec889bf26d8fa94199e706da/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorViewDefinition.java#L39C1-L39C1)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks commented on pull request #7992: Core: Simplify/improve View APIs
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#issuecomment-1622249389
I feel like we lost some context from #7880, specifically the [discussion](https://github.com/apache/iceberg/pull/7880#discussion_r1250671566) around how we want to represent these APIs.
I agree with @jackye1995 about favoring option B. I just feel like this is too SQL specific and limits our options going forward with different representations and creates unnecessary differences with the spec representation.
Also, I'm not convinced that you can/should have `field-aliases` and `field-comments` at the top-level. Some engines may support field aliases, but others do not so they don't apply equally to the representations and is inconsistent with the spec, which has them at the representation level.
I think it's fine to omit the default catalog and default namespaces, especially if we can appropriately qualify them in the engine when constructing the sql representation.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1253522120
##########
api/src/main/java/org/apache/iceberg/view/ViewBuilder.java:
##########
@@ -39,60 +38,25 @@ public interface ViewBuilder {
ViewBuilder withSchema(Schema schema);
/**
- * Set the view query.
+ * Add a view representation for the given dialect and the SQL to the view.
*
- * @param query view query
+ * @param dialect The dialect of the view representation
+ * @param sql The SQL of the view representation
* @return this for method chaining
*/
- ViewBuilder withQuery(String query);
+ default ViewBuilder withQuery(String dialect, String sql) {
+ return withQuery(dialect, sql, null);
+ }
/**
- * Set the view SQL dialect.
+ * Add a view representation for the given dialect and the SQL to the view.
*
- * @param dialect view SQL dialect
+ * @param dialect The dialect of the view representation
+ * @param sql The SQL of the view representation
+ * @param defaultNamespace The default namespace to use when the SQL does not contain a namespace
* @return this for method chaining
*/
- ViewBuilder withDialect(String dialect);
-
- /**
- * Set the view default catalog.
- *
- * @param defaultCatalog view default catalog
- * @return this for method chaining
- */
- ViewBuilder withDefaultCatalog(String defaultCatalog);
-
- /**
- * Set the view default namespace.
- *
- * @param defaultNamespace view default namespace
- * @return this for method chaining
- */
- ViewBuilder withDefaultNamespace(Namespace defaultNamespace);
-
- /**
- * Set the view query column names.
- *
- * @param queryColumnNames view query column names
- * @return this for method chaining
- */
- ViewBuilder withQueryColumnNames(List<String> queryColumnNames);
-
- /**
- * Set the view field aliases.
- *
- * @param fieldAliases view field aliases
- * @return this for method chaining
- */
- ViewBuilder withFieldAliases(List<String> fieldAliases);
-
- /**
- * Set the view field comments.
- *
- * @param fieldComments view field comments
- * @return this for method chaining
- */
- ViewBuilder withFieldComments(List<String> fieldComments);
+ ViewBuilder withQuery(String dialect, String sql, Namespace defaultNamespace);
Review Comment:
@nastra, what we talked about was that this should be a property of the `ViewVersion`, not the SQL. You can't define two SQL queries with different default namespaces.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#issuecomment-1622327521
@nastra, the example you gave would not be allowed:
```java
View view = catalog.buildView(identifier)
.withSchema(schema)
.withQuery("spark", "select a,b from ns.tbl")
.withQuery("trino", "select a,b,c from ns.tbl")
.createOrReplace()
```
The representations must produce the same schema.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1274196821
##########
api/src/main/java/org/apache/iceberg/view/ViewVersion.java:
##########
@@ -74,6 +76,14 @@ default String operation() {
/** The query output schema at version create time, without aliases */
int schemaId();
+ /** The default catalog when the view is created. */
+ @Nullable
+ String defaultCatalog();
+
+ /** The default namespace to use when the SQL does not contain a namespace. */
+ @Nullable
Review Comment:
I think that this should be required in all cases.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1274204028
##########
format/view-spec.md:
##########
@@ -37,6 +37,8 @@ Each compute engine stores the metadata of the view in its proprietary format in
View metadata storage mirrors how Iceberg table metadata is stored and retrieved. View metadata is maintained in metadata files. All changes to view state create a new view metadata file and completely replace the old metadata using an atomic swap. Like Iceberg tables, this atomic swap is delegated to the metastore that tracks tables and/or views by name. The view metadata file tracks the view schema, custom properties, current and past versions, as well as other metadata.
+When view metadata is replaced, then previously set properties are carried over.
Review Comment:
```suggestion
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1274206693
##########
format/view-spec.md:
##########
@@ -75,13 +77,17 @@ Notes:
Each version in `versions` is a struct with the following fields:
-| Requirement | Field name | Description |
-|-------------|-------------------|--------------------------------------------------------------------------|
-| _required_ | `version-id` | ID for the version |
-| _required_ | `schema-id` | ID of the schema for the view version |
-| _required_ | `timestamp-ms` | Timestamp when the version was created (ms from epoch) |
-| _required_ | `summary` | A string to string map of [summary metadata](#summary) about the version |
-| _required_ | `representations` | A list of [representations](#representations) for the view definition |
+| Requirement | Field name | Description |
+|-------------|---------------------|-------------------------------------------------------------------------------|
+| _required_ | `version-id` | ID for the version |
+| _required_ | `schema-id` | ID of the schema for the view version |
+| _required_ | `timestamp-ms` | Timestamp when the version was created (ms from epoch) |
+| _required_ | `summary` | A string to string map of [summary metadata](#summary) about the version |
+| _required_ | `representations` | A list of [representations](#representations) for the view definition |
+| _optional_ | `default-catalog` | Catalog name to use when a reference in the SELECT does not contain a catalog |
+| _optional_ | `default-namespace` | Namespace to use when a reference in the SELECT is a single identifier |
Review Comment:
```suggestion
| _required_ | `default-namespace` | Namespace to use when a reference in the SELECT is a single identifier |
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1274420285
##########
format/view-spec.md:
##########
@@ -75,13 +77,17 @@ Notes:
Each version in `versions` is a struct with the following fields:
-| Requirement | Field name | Description |
-|-------------|-------------------|--------------------------------------------------------------------------|
-| _required_ | `version-id` | ID for the version |
-| _required_ | `schema-id` | ID of the schema for the view version |
-| _required_ | `timestamp-ms` | Timestamp when the version was created (ms from epoch) |
-| _required_ | `summary` | A string to string map of [summary metadata](#summary) about the version |
-| _required_ | `representations` | A list of [representations](#representations) for the view definition |
+| Requirement | Field name | Description |
+|-------------|---------------------|-------------------------------------------------------------------------------|
+| _required_ | `version-id` | ID for the version |
+| _required_ | `schema-id` | ID of the schema for the view version |
+| _required_ | `timestamp-ms` | Timestamp when the version was created (ms from epoch) |
+| _required_ | `summary` | A string to string map of [summary metadata](#summary) about the version |
+| _required_ | `representations` | A list of [representations](#representations) for the view definition |
+| _optional_ | `default-catalog` | Catalog name to use when a reference in the SELECT does not contain a catalog |
+| _optional_ | `default-namespace` | Namespace to use when a reference in the SELECT is a single identifier |
Review Comment:
thanks
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1266790240
##########
api/src/main/java/org/apache/iceberg/view/ViewVersion.java:
##########
@@ -74,6 +76,14 @@ default String operation() {
/** The query output schema at version create time, without aliases */
int schemaId();
+ /** The default catalog when the view is created. */
+ @Nullable
+ String defaultCatalog();
+
+ /** The default namespace to use when the SQL does not contain a namespace. */
+ @Nullable
Review Comment:
I've looked at how Trino handles this.
Trino always sets the **catalog** and the **schema name** on view creation (even though both fields are optional). During View creation the underlying SQL is parsed and checked. If the SQL is unqualified, it is checked that the table exists within the **catalog/schema** that the view is created under. If the SQL is qualified, it is checked that the qualified table exists.
When a SELECT against a View is performed, Trino parses the [underlying SQL](https://github.com/trinodb/trino/blob/10d3d1f4ef3f9d60581cbb2b5c4e60249abc6dc7/core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java#L2448-L2451) and then sets the [Session scope for the view](https://github.com/trinodb/trino/blob/10d3d1f4ef3f9d60581cbb2b5c4e60249abc6dc7/core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java#L4578) using the **catalog/schema** from the stored view definition.
The [parsed SQL](https://github.com/trinodb/trino/blob/10d3d1f4ef3f9d60581cbb2b5c4e60249abc6dc7/core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java#L2448) knows whether the table is qualified/unqualified. If it's qualified, then it's executed against the qualified table name. If it's unqualified, then it's executed against the **catalog/schema** that was known at view definition time.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1253439422
##########
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:
One more thing: this may actually be dangerous with multiple people interacting. If you replace a view with a new query at the same time that I add a dialect to the old verison, we could end up mixing SQL that doesn't produce the same result.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1254112125
##########
api/src/main/java/org/apache/iceberg/view/SQLViewRepresentation.java:
##########
@@ -37,17 +36,7 @@ default String type() {
/** The view query SQL dialect. */
String dialect();
- /** The default catalog when the view is created. */
- @Nullable
- String defaultCatalog();
Review Comment:
hm good point, should we maybe store `defaultCatalog` at the `ViewVersion` level in this case (same as the `defaultNamespace`?)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1274420786
##########
format/view-spec.md:
##########
@@ -37,6 +37,8 @@ Each compute engine stores the metadata of the view in its proprietary format in
View metadata storage mirrors how Iceberg table metadata is stored and retrieved. View metadata is maintained in metadata files. All changes to view state create a new view metadata file and completely replace the old metadata using an atomic swap. Like Iceberg tables, this atomic swap is delegated to the metastore that tracks tables and/or views by name. The view metadata file tracks the view schema, custom properties, current and past versions, as well as other metadata.
+When view metadata is replaced, then previously set properties are carried over.
Review Comment:
makes sense, thanks for updating this
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#issuecomment-1651039473
> > Once both PRs are merged, I'll add a follow-up and mention in the docs that @Value.Immutable usage is discouraged on iceberg-api (with a checkstyle rule that enforces it) and will also remove the Immutable dependency
>
> I think we should disallow it, not just discourage it. A checkstyle rule sounds like the right approach though, so maybe we're saying the same thing.
Yes we are saying the same thing here. The goal with the docs part was mainly to describe why `@Value.Immutable` should not be in `iceberg-api` and show 1-2 examples of how to apply `@Value.Immutable` to an API that is defined in `iceberg-api`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1274196210
##########
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:
@nastra, it isn't clear when you talk about carrying over representations because you're missing a layer of metadata. When you replace a view, the previous properties and versions are preserved. Replace also creates an entirely new version of the view that is independent of the other versions and the representations within those versions.
Iceberg is not responsible for somehow merging a view's current version with new SQL provided by the replace.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1274671955
##########
api/src/main/java/org/apache/iceberg/view/ViewVersion.java:
##########
@@ -74,6 +76,14 @@ default String operation() {
/** The query output schema at version create time, without aliases */
int schemaId();
+ /** The default catalog when the view is created. */
+ @Nullable
+ String defaultCatalog();
+
+ /** The default namespace to use when the SQL does not contain a namespace. */
+ @Nullable
Review Comment:
I've updated this and made it required in https://github.com/apache/iceberg/pull/8156
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1274208296
##########
format/view-spec.md:
##########
@@ -75,13 +77,17 @@ Notes:
Each version in `versions` is a struct with the following fields:
-| Requirement | Field name | Description |
-|-------------|-------------------|--------------------------------------------------------------------------|
-| _required_ | `version-id` | ID for the version |
-| _required_ | `schema-id` | ID of the schema for the view version |
-| _required_ | `timestamp-ms` | Timestamp when the version was created (ms from epoch) |
-| _required_ | `summary` | A string to string map of [summary metadata](#summary) about the version |
-| _required_ | `representations` | A list of [representations](#representations) for the view definition |
+| Requirement | Field name | Description |
+|-------------|---------------------|-------------------------------------------------------------------------------|
+| _required_ | `version-id` | ID for the version |
+| _required_ | `schema-id` | ID of the schema for the view version |
+| _required_ | `timestamp-ms` | Timestamp when the version was created (ms from epoch) |
+| _required_ | `summary` | A string to string map of [summary metadata](#summary) about the version |
+| _required_ | `representations` | A list of [representations](#representations) for the view definition |
+| _optional_ | `default-catalog` | Catalog name to use when a reference in the SELECT does not contain a catalog |
+| _optional_ | `default-namespace` | Namespace to use when a reference in the SELECT is a single identifier |
Review Comment:
Updated to required and added clarification for how to handle this when it is null or missing.
##########
format/view-spec.md:
##########
@@ -75,13 +77,17 @@ Notes:
Each version in `versions` is a struct with the following fields:
-| Requirement | Field name | Description |
-|-------------|-------------------|--------------------------------------------------------------------------|
-| _required_ | `version-id` | ID for the version |
-| _required_ | `schema-id` | ID of the schema for the view version |
-| _required_ | `timestamp-ms` | Timestamp when the version was created (ms from epoch) |
-| _required_ | `summary` | A string to string map of [summary metadata](#summary) about the version |
-| _required_ | `representations` | A list of [representations](#representations) for the view definition |
+| Requirement | Field name | Description |
+|-------------|---------------------|-------------------------------------------------------------------------------|
+| _required_ | `version-id` | ID for the version |
+| _required_ | `schema-id` | ID of the schema for the view version |
+| _required_ | `timestamp-ms` | Timestamp when the version was created (ms from epoch) |
+| _required_ | `summary` | A string to string map of [summary metadata](#summary) about the version |
+| _required_ | `representations` | A list of [representations](#representations) for the view definition |
+| _optional_ | `default-catalog` | Catalog name to use when a reference in the SELECT does not contain a catalog |
+| _optional_ | `default-namespace` | Namespace to use when a reference in the SELECT is a single identifier |
+
+When writing a new view version, all the information from the previous view version is replaced, meaning that `representations` / `default-catalog` / `default-namespace` / `summary` are not carried over.
Review Comment:
Removed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1256083463
##########
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:
To summarize: doing a replace should carry over properties, but previous representations will not be carried over (because we don't know whether a previous view representation created for spark is compatible with a new view representation created for trino). I'll add a note to the view spec so that this behavior is clear and doesn't cause surprises.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1254817163
##########
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:
> The semantics of .replace() at the View API level should mean that previously configured representations and properties are carried over.
Yes, this is what we do for tables. If you want to erase the history, then you can drop and re-create the object.
> we can probably remove the UpdateViewRepresentation
I think we should remove this. What we want instead is a way to replace the version, like `newVersion()` or `replaceVersion()`. I think that has mostly the same API as the `ViewBuilder`, probably through a shared interface like `ViewVersionBuilder` that has all the version-related methods.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1258477213
##########
api/src/main/java/org/apache/iceberg/view/ReplaceViewVersion.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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;
+
+/**
+ * API for replacing a view's version.
+ *
+ * <p>Apply returns the updated view version 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 ReplaceViewVersion
+ extends PendingUpdate<ViewVersion>, VersionBuilder<ReplaceViewVersion> {
+
+ /**
+ * Remove the representation for the given dialect from the view's version.
+ *
+ * @param dialect The dialect for which the view representation should be removed.
+ * @return this for method chaining
+ */
+ ReplaceViewVersion remove(String dialect);
Review Comment:
@nastra, I don't think that we want an API that carries over _anything_ from the last version. That's dangerous, so we should remove this method.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1258604469
##########
api/src/main/java/org/apache/iceberg/view/VersionBuilder.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.catalog.Namespace;
+
+public interface VersionBuilder<T> {
+ /**
+ * Add a view representation for the given dialect and the SQL to the view.
+ *
+ * @param dialect The dialect of the view representation
+ * @param sql The SQL of the view representation
+ * @return this for method chaining
+ */
+ default T withQuery(String dialect, String sql) {
+ return withQuery(dialect, sql, null);
+ }
+
+ /**
+ * Add a view representation for the given dialect and the SQL to the view.
+ *
+ * @param dialect The dialect of the view representation
+ * @param sql The SQL of the view representation
+ * @param defaultNamespace The default namespace to use when the SQL does not contain a namespace
+ * @return this for method chaining
+ */
+ T withQuery(String dialect, String sql, Namespace defaultNamespace);
Review Comment:
The default namespace should be a property of the view version, not a representation. `withDefaultNamespace` should be a separate method.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1253436167
##########
api/src/main/java/org/apache/iceberg/view/SQLViewRepresentation.java:
##########
@@ -37,17 +36,7 @@ default String type() {
/** The view query SQL dialect. */
String dialect();
- /** The default catalog when the view is created. */
- @Nullable
- String defaultCatalog();
Review Comment:
The rationale for removing this isn't necessarily true. We could make this implicit, but it actually causes some problems with how we want to maintain the SQL without modification.
This was added to fill in necessary information that may be missing from the statement that created the view. For example:
```sql
USE prod.default; -- catalog=prod, schema=default
CREATE OR REPLACE VIEW other_catalog.views.new_view (id, row_count) AS
SELECT id, count(1)
FROM some_table
GROUP BY id
```
As you can see, the current catalog and namespace don't necessarily match the catalog and namespace that were current when the view was created. That means we either have to rewrite the SQL text or store the current catalog and namespace from the environment.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1254081793
##########
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:
> One more thing: this may actually be dangerous with multiple people interacting. If you replace a view with a new query at the same time that I add a dialect to the old verison, we could end up mixing SQL that doesn't produce the same result.
Modifying any of this would write a new view version id and conflict detection would detect that the base view version of the second update changed (that's how it's currently implemented in https://github.com/apache/iceberg/pull/7913)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Simplify/improve View APIs [iceberg]
Posted by "wmoustafa (via GitHub)" <gi...@apache.org>.
wmoustafa commented on PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#issuecomment-1763172619
> Also, I'm not convinced that you can/should have `field-aliases` and `field-comments` at the top-level. Some engines may support field aliases, but others do not so they don't apply equally to the representations and is inconsistent with the spec, which has them at the representation level.
+1. Also worth considering whether "Representation" really refers to an engine or a dialect. Right now it looks like dialect, but even if we go with the above we will be making an implicit correlation between dialect and engine.
> I think it's fine to omit the default catalog and default namespaces, especially if we can appropriately qualify them in the engine when constructing the sql representation.
+1. How are we thinking about the fact that catalog names are not the same across engines, even when the underlying tables end up being looked up from the same metastore/catalog? E.g., `hive.db.t` in Trino might refer to the same table as `spark_catalog.db.t` in Spark?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1259290931
##########
api/src/main/java/org/apache/iceberg/view/VersionBuilder.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.catalog.Namespace;
+
+public interface VersionBuilder<T> {
Review Comment:
should be fixed now
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1264760836
##########
format/view-spec.md:
##########
@@ -144,13 +146,13 @@ This create statement would produce the following `sql` representation metadata:
| `type` | `"sql"` |
| `sql` | `"SELECT\n COUNT(1), CAST(event_ts AS DATE)\nFROM events\nGROUP BY 2"` |
| `dialect` | `"spark"` |
-| `default-catalog` | `"prod"` |
-| `default-namespace` | `["default"]` |
-| `field-aliases` | `["event_count", "event_date"]` |
-| `field-comments` | `["Count of events", null]` |
If a create statement does not include column names or comments before `AS`, the fields should be omitted.
+The `event_count` (with the `Count of events` comment) and `event_date` field aliases must be part of the view version's `schema`.
+
+The `default-catalog` and `default-namespace` of the view version should use the same value across multiple SQL representations.
Review Comment:
I think "must be" rather than "should be", although this can probably be removed entirely because those are no longer part of the representation. It just IS the same across all representations.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1258605553
##########
api/src/main/java/org/apache/iceberg/view/ViewBuilder.java:
##########
@@ -18,18 +18,16 @@
*/
package org.apache.iceberg.view;
-import java.util.List;
import java.util.Map;
import org.apache.iceberg.Schema;
-import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.ViewCatalog;
/**
* A builder used to create or replace a SQL {@link View}.
*
* <p>Call {@link ViewCatalog#buildView} to create a new builder.
*/
-public interface ViewBuilder {
+public interface ViewBuilder extends VersionBuilder<ViewBuilder> {
Review Comment:
Do we need a `withLocation` to set the location used for storing metadata files?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#issuecomment-1642304557
> In particular, it's really unfortunate that I didn't notice that Immutables was generating the implementations in the api module until now. That means far more was exposed and will break with these changes -- otherwise we could have simply made the removed methods return null or update them to return values from the ViewVersion and kept the getters. But the implementations are changing and had public builders as well.
It's a mess up on my part and I should have noticed this when `@Value.Immutable` usage was added to `iceberg-api`. I was the main driving factor to get `@Value.Immutable` into Iceberg and it should have been my responsibility to make sure we're not exposing more than we should, especially in the `iceberg-api` module.
I have moved the View-specific Immutable classes as part of this PR to `iceberg-core` and I also opened https://github.com/apache/iceberg/pull/8099 to do the same for the other places in `iceberg-api`.
Once both PRs are merged, I'll add a follow-up and mention in the docs that `@Value.Immutable` usage is discouraged on `iceberg-api` (with a checkstyle rule that enforces it) and will also remove the Immutable dependency
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1274203864
##########
format/view-spec.md:
##########
@@ -37,6 +37,8 @@ Each compute engine stores the metadata of the view in its proprietary format in
View metadata storage mirrors how Iceberg table metadata is stored and retrieved. View metadata is maintained in metadata files. All changes to view state create a new view metadata file and completely replace the old metadata using an atomic swap. Like Iceberg tables, this atomic swap is delegated to the metastore that tracks tables and/or views by name. The view metadata file tracks the view schema, custom properties, current and past versions, as well as other metadata.
+When view metadata is replaced, then previously set properties are carried over.
Review Comment:
I think it's incorrect to make this statement in the spec. The "replace" operation for `REPLACE VIEW` is a SQL operation with chosen semantics that are represented in the Iceberg view metadata as you describe. But it's a very different statement to say that "when view metadata is replaced" something happens.
Saying that when metadata is "replaced" here could mean any update to the view metadata object, which is a much broader statement than you intended.
As in the table spec, we don't want to add broad statements like this. You can add a section explaining how a SQL `REPLACE VIEW` works, but even then I wouldn't say that it is a requirement of the spec.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1274207599
##########
format/view-spec.md:
##########
@@ -75,13 +75,15 @@ Notes:
Each version in `versions` is a struct with the following fields:
-| Requirement | Field name | Description |
-|-------------|-------------------|--------------------------------------------------------------------------|
-| _required_ | `version-id` | ID for the version |
-| _required_ | `schema-id` | ID of the schema for the view version |
-| _required_ | `timestamp-ms` | Timestamp when the version was created (ms from epoch) |
-| _required_ | `summary` | A string to string map of [summary metadata](#summary) about the version |
-| _required_ | `representations` | A list of [representations](#representations) for the view definition |
+| Requirement | Field name | Description |
+|-------------|---------------------|-------------------------------------------------------------------------------|
+| _required_ | `version-id` | ID for the version |
+| _required_ | `schema-id` | ID of the schema for the view version |
+| _required_ | `timestamp-ms` | Timestamp when the version was created (ms from epoch) |
+| _required_ | `summary` | A string to string map of [summary metadata](#summary) about the version |
+| _required_ | `representations` | A list of [representations](#representations) for the view definition |
+| _optional_ | `default-catalog` | Catalog name to use when a reference in the SELECT does not contain a catalog |
+| _required_ | `default-namespace` | Namespace to use when a reference in the SELECT is a single identifier |
Review Comment:
```suggestion
| _required_ | `default-namespace` | Namespace to use when a reference in the SELECT is a single identifier |
When `default-catalog` is `null` or not set, the catalog in which the view is stored must be used as the default catalog.
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1264935131
##########
api/src/main/java/org/apache/iceberg/view/ViewVersion.java:
##########
@@ -74,6 +76,14 @@ default String operation() {
/** The query output schema at version create time, without aliases */
int schemaId();
+ /** The default catalog when the view is created. */
+ @Nullable
+ String defaultCatalog();
+
+ /** The default namespace to use when the SQL does not contain a namespace. */
+ @Nullable
Review Comment:
Yes this is optional and query resolution would fail at runtime if this isn't set for unqualified names. Engines like Spark or Trino would have to make sure to set this appropriately when the query of the view isn't qualified
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1258076736
##########
api/src/main/java/org/apache/iceberg/view/ReplaceViewVersion.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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;
+
+/**
+ * API for replacing a view's version.
+ *
+ * <p>Apply returns the updated view version 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 ReplaceViewVersion
+ extends PendingUpdate<ViewVersion>, VersionBuilder<ReplaceViewVersion> {
+
+ /**
+ * Remove the representation for the given dialect from the view's version.
+ *
+ * @param dialect The dialect for which the view representation should be removed.
+ * @return this for method chaining
+ */
+ ReplaceViewVersion remove(String dialect);
Review Comment:
The `.remove()` is mainly to cover this particular use case:
```
View view = catalog.buildView(identifier)
.withSchema(schema)
.withQuery("spark", "sql")
.withQuery("trino", "sql")
.createOrReplace()
// updates the SQL for trino and removes the representation for spark
view.replaceViewVersion()
.withQuery("trino", "updated sql")
.remove("spark")
.commit()
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1254106924
##########
format/view-spec.md:
##########
@@ -101,6 +101,8 @@ A view version can have more than one representation. All representations for a
View versions are immutable. Once a version is created, it cannot be changed. This means that representations for a version cannot be changed. If a view definition changes (or new representations are to be added), a new version must be created.
+There should be no more than one view representation per dialect.
Review Comment:
I've removed this sentence as I just noticed that this is already being mentioned:
> A view version can have multiple SQL representations of different dialects, but only one SQL representation per dialect.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1253437549
##########
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:
I think we need a better name than `UpdateViewRepresentation`. That doesn't capture what is happening.
I'm also not sure we even need this in the initial API. Are these operations that people will need to use? What is the SQL that exercises them or what is the use case?
At this stage, I think it is more important to focus on the ViewVersion API. We can add a way to add/remove representations based on the current version later.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1258604964
##########
api/src/main/java/org/apache/iceberg/view/VersionBuilder.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.catalog.Namespace;
+
+public interface VersionBuilder<T> {
Review Comment:
Where are the other methods, like `withSchema` and `withDefaultCatalog`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1258582197
##########
api/src/main/java/org/apache/iceberg/view/ReplaceViewVersion.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * 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;
+
+/**
+ * API for replacing a view's version.
+ *
+ * <p>Apply returns the updated view version 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 ReplaceViewVersion
+ extends PendingUpdate<ViewVersion>, VersionBuilder<ReplaceViewVersion> {
+
+ /**
+ * Remove the representation for the given dialect from the view's version.
+ *
+ * @param dialect The dialect for which the view representation should be removed.
+ * @return this for method chaining
+ */
+ ReplaceViewVersion remove(String dialect);
Review Comment:
ok I've removed that method.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1264760118
##########
api/src/main/java/org/apache/iceberg/view/VersionBuilder.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.Schema;
+import org.apache.iceberg.catalog.Namespace;
+
+public interface VersionBuilder<T> {
+ /**
+ * Set the view schema.
+ *
+ * @param schema The schema to use for this view version
+ * @return this for method chaining
+ */
+ T withSchema(Schema schema);
+
+ /**
+ * Add a view representation for the given dialect and the SQL to the view.
+ *
+ * @param dialect The dialect of the view representation
+ * @param sql The SQL of the view representation
+ * @return this for method chaining
+ */
+ T withQuery(String dialect, String sql);
+
+ /**
+ * Add a view representation for the given dialect and the SQL to the view.
Review Comment:
This Javadoc isn't correct. Looks like a copy/paste problem.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1265675643
##########
api/src/main/java/org/apache/iceberg/view/ViewVersion.java:
##########
@@ -74,6 +76,14 @@ default String operation() {
/** The query output schema at version create time, without aliases */
int schemaId();
+ /** The default catalog when the view is created. */
+ @Nullable
+ String defaultCatalog();
+
+ /** The default namespace to use when the SQL does not contain a namespace. */
+ @Nullable
Review Comment:
@nastra, how do you know whether a name is unqualified at runtime?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1274204028
##########
format/view-spec.md:
##########
@@ -37,6 +37,8 @@ Each compute engine stores the metadata of the view in its proprietary format in
View metadata storage mirrors how Iceberg table metadata is stored and retrieved. View metadata is maintained in metadata files. All changes to view state create a new view metadata file and completely replace the old metadata using an atomic swap. Like Iceberg tables, this atomic swap is delegated to the metastore that tracks tables and/or views by name. The view metadata file tracks the view schema, custom properties, current and past versions, as well as other metadata.
+When view metadata is replaced, then previously set properties are carried over.
Review Comment:
```suggestion
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1274208162
##########
format/view-spec.md:
##########
@@ -37,6 +37,8 @@ Each compute engine stores the metadata of the view in its proprietary format in
View metadata storage mirrors how Iceberg table metadata is stored and retrieved. View metadata is maintained in metadata files. All changes to view state create a new view metadata file and completely replace the old metadata using an atomic swap. Like Iceberg tables, this atomic swap is delegated to the metastore that tracks tables and/or views by name. The view metadata file tracks the view schema, custom properties, current and past versions, as well as other metadata.
+When view metadata is replaced, then previously set properties are carried over.
Review Comment:
Removed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #7992: Core: Simplify/improve View APIs
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#issuecomment-1650694604
Thanks, @nastra!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1264932280
##########
api/src/main/java/org/apache/iceberg/view/VersionBuilder.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.Schema;
+import org.apache.iceberg.catalog.Namespace;
+
+public interface VersionBuilder<T> {
+ /**
+ * Set the view schema.
+ *
+ * @param schema The schema to use for this view version
+ * @return this for method chaining
+ */
+ T withSchema(Schema schema);
+
+ /**
+ * Add a view representation for the given dialect and the SQL to the view.
+ *
+ * @param dialect The dialect of the view representation
+ * @param sql The SQL of the view representation
+ * @return this for method chaining
+ */
+ T withQuery(String dialect, String sql);
+
+ /**
+ * Add a view representation for the given dialect and the SQL to the view.
Review Comment:
thanks for catching this, fixed
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7992: Core: Simplify/improve View APIs
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7992:
URL: https://github.com/apache/iceberg/pull/7992#discussion_r1264928428
##########
format/view-spec.md:
##########
@@ -144,13 +146,13 @@ This create statement would produce the following `sql` representation metadata:
| `type` | `"sql"` |
| `sql` | `"SELECT\n COUNT(1), CAST(event_ts AS DATE)\nFROM events\nGROUP BY 2"` |
| `dialect` | `"spark"` |
-| `default-catalog` | `"prod"` |
-| `default-namespace` | `["default"]` |
-| `field-aliases` | `["event_count", "event_date"]` |
-| `field-comments` | `["Count of events", null]` |
If a create statement does not include column names or comments before `AS`, the fields should be omitted.
+The `event_count` (with the `Count of events` comment) and `event_date` field aliases must be part of the view version's `schema`.
+
+The `default-catalog` and `default-namespace` of the view version should use the same value across multiple SQL representations.
Review Comment:
indeed this can be removed now
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org