wmoustafa commented on code in PR #4925: URL: https://github.com/apache/iceberg/pull/4925#discussion_r1015154082
########## api/src/main/java/org/apache/iceberg/view/ViewVersion.java: ########## @@ -0,0 +1,71 @@ +/* + * 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 java.util.List; +import java.util.Map; + +/** + * A version of the view at a point in time. + * + * <p>A version consists of a view metadata file. + * + * <p>Versions are created by view operations, like Create and Replace. + */ +public interface ViewVersion { + + /** Return this version's id. Version ids are monotonically increasing */ + int versionId(); + + /** Returns this version's parent id or null if there is no parent */ + int parentId(); + + /** + * Return this version's timestamp. + * + * <p>This timestamp is the same as those produced by {@link System#currentTimeMillis()}. + * + * @return a long timestamp in milliseconds + */ + long timestampMillis(); + + /** + * Return the version summary such as the name of the operation that created that version of the + * view + * + * @return a version summary + */ + Map<String, String> summary(); + + /** + * Return the main SQL view representation. + * + * @return a SQL view representation + */ + SQLViewRepresentation sqlViewRepresentation(); + + /** + * Return the list of other view representations. + * + * <p>May contain SQL view representations for other dialects. + * + * @return the list of view representations + */ + List<ViewRepresentation> representations(); Review Comment: `The main SQL view representation` sounds like something new to the spec. ########## api/src/main/java/org/apache/iceberg/view/SQLViewRepresentation.java: ########## @@ -0,0 +1,53 @@ +/* + * 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 java.util.List; +import org.apache.iceberg.catalog.Namespace; + +public interface SQLViewRepresentation extends ViewRepresentation { + + @Override + default Type type() { + return Type.SQL; + } + + /** The view query SQL text. */ + String query(); Review Comment: Why would not this method be also common with other representations? ########## api/src/main/java/org/apache/iceberg/view/SQLViewRepresentation.java: ########## @@ -0,0 +1,53 @@ +/* + * 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 java.util.List; +import org.apache.iceberg.catalog.Namespace; + +public interface SQLViewRepresentation extends ViewRepresentation { + + @Override + default Type type() { + return Type.SQL; + } + + /** The view query SQL text. */ + String query(); + + /** The view query SQL dialect. */ + String dialect(); Review Comment: I feel this needs to be an `Enum` and somehow be merged with `Type`. @rdblue, @danielcweeks, @amogh-jahagirdar, thoughts? Further, it is better for the spec to take a position on supported dialects, or not take a position at all, but in this case SQL should not be a special type. Right now the position sounds a bit fuzzy. ########## api/src/main/java/org/apache/iceberg/view/ViewBuilder.java: ########## @@ -0,0 +1,133 @@ +/* + * 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 java.util.List; +import java.util.Map; +import org.apache.iceberg.Schema; +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 { + /** + * Set the view schema. + * + * @param schema view schema + * @return this for method chaining + */ + ViewBuilder withSchema(Schema schema); + + /** + * Set the view query. + * + * @param query view query + * @return this for method chaining + */ + ViewBuilder withQuery(String query); Review Comment: My original question was that this builder returns a `View`, and in this discussion, you mentioned that `withQuery()` is the method responsible for the fact that it should return `SQLViewRepresentation`. However, I think this method still does not enforce it in this iteration. -- 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-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
