Copilot commented on code in PR #11019: URL: https://github.com/apache/gravitino/pull/11019#discussion_r3214906804
########## clients/client-python/gravitino/api/rel/view.py: ########## @@ -0,0 +1,55 @@ +# 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. + +from abc import ABC, abstractmethod +from typing import Optional + + +class View(ABC): + """The `View` interface defines the metadata of a view.""" + + @abstractmethod + def name(self) -> str: + """Get the name of the view. + + Returns: + str: The view name. + """ + + @abstractmethod + def comment(self) -> Optional[str]: + """Get the comment of the view. + + Returns: + Optional[str]: The view comment. + """ + + @abstractmethod + def properties(self) -> dict[str, str]: + """Get the properties of the view. + + Returns: + dict[str, str]: The view properties. + """ + + @abstractmethod + def view_definition(self) -> Optional[str]: + """Get the SQL definition of the view. + + Returns: + Optional[str]: The view definition. Review Comment: `view_definition()` is typed as `Optional[str]`, but a view definition appears to be required for a usable view (server-side create request validation requires at least one representation/definition). Consider making this a required `str` and documenting when/if it can ever be absent. ########## clients/client-python/gravitino/api/rel/view.py: ########## @@ -0,0 +1,55 @@ +# 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. + +from abc import ABC, abstractmethod +from typing import Optional + + +class View(ABC): + """The `View` interface defines the metadata of a view.""" + Review Comment: `View` doesn’t extend `Auditable`, unlike `Table` (and unlike the Java `View` API which extends `Auditable`). This makes view metadata inconsistent across entity types and prevents callers from accessing audit info for views. Consider inheriting from `Auditable` and adding the `audit_info()` contract for parity with other relational objects. ########## clients/client-python/gravitino/api/rel/view_catalog.py: ########## @@ -0,0 +1,124 @@ +# 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. + +from abc import ABC, abstractmethod +from contextlib import suppress +from typing import Optional + +from gravitino.api.rel.view import View +from gravitino.exceptions.base import NoSuchViewException +from gravitino.name_identifier import NameIdentifier Review Comment: `NoSuchViewException` is imported and used here, but the Python client codebase does not define it anywhere (e.g., `gravitino/exceptions/base.py` has `NoSuchTableException` but no `NoSuchViewException`). This will raise `ImportError` at runtime; add the missing exception class (and any corresponding error handler mapping) or update the import to an existing exception type. ########## clients/client-python/gravitino/api/rel/view.py: ########## @@ -0,0 +1,55 @@ +# 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. + +from abc import ABC, abstractmethod +from typing import Optional + + +class View(ABC): + """The `View` interface defines the metadata of a view.""" + + @abstractmethod + def name(self) -> str: + """Get the name of the view. + + Returns: + str: The view name. + """ + + @abstractmethod + def comment(self) -> Optional[str]: + """Get the comment of the view. + + Returns: + Optional[str]: The view comment. + """ + + @abstractmethod + def properties(self) -> dict[str, str]: + """Get the properties of the view. + + Returns: + dict[str, str]: The view properties. + """ Review Comment: `comment()` and `properties()` are declared abstract here, but in the existing `Table` interface they have concrete default implementations (`None` / `{}`). Making them abstract increases implementation burden and makes `View` inconsistent with other metadata interfaces. Consider providing the same defaults unless a view is required to always have these fields. ########## clients/client-python/gravitino/api/rel/view_catalog.py: ########## @@ -0,0 +1,124 @@ +# 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. + +from abc import ABC, abstractmethod +from contextlib import suppress +from typing import Optional + +from gravitino.api.rel.view import View +from gravitino.exceptions.base import NoSuchViewException +from gravitino.name_identifier import NameIdentifier +from gravitino.namespace import Namespace + + +class ViewCatalog(ABC): + """The `ViewCatalog` interface defines the public API for managing views in a schema. + + If the catalog implementation supports views, it must implement this interface. + """ + + @abstractmethod + def list_views(self, namespace: Namespace) -> list[NameIdentifier]: + """List the views in a namespace from the catalog. + + Args: + namespace (Namespace): A namespace. + + Returns: + list[NameIdentifier]: An array of view identifiers in the namespace. + + Raises: + NoSuchSchemaException: If the schema does not exist. + """ + + @abstractmethod + def load_view(self, identifier: NameIdentifier) -> View: + """Load view metadata by `NameIdentifier` from the catalog. + + Args: + identifier (NameIdentifier): A view identifier. + + Returns: + View: The view metadata. + + Raises: + NoSuchViewException: If the view does not exist. + """ + + def view_exists(self, identifier: NameIdentifier) -> bool: + """Check if a view exists using an `NameIdentifier` from the catalog. + + Args: + identifier (NameIdentifier): A view identifier. + + Returns: + bool: `True` If the view exists, `False` otherwise. + """ + with suppress(NoSuchViewException): + self.load_view(identifier) + return True + return False + + @abstractmethod + def create_view( + self, + identifier: NameIdentifier, + comment: Optional[str] = None, + properties: Optional[dict[str, str]] = None, + view_definition: Optional[str] = None, + ) -> View: Review Comment: `create_view()` allows `view_definition=None` (default) and doesn’t accept view output columns or dialect-specific representations. Given the server-side view model requires at least one representation (and optionally columns/default catalog/schema), this interface may be too limited for a correct CRUD implementation. Consider requiring `view_definition` (or modeling `representations`/`columns` explicitly) so implementations can construct valid create requests. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
