Copilot commented on code in PR #14124:
URL: https://github.com/apache/gravitino/pull/14124#discussion_r4344103800
##########
api/src/main/java/org/apache/gravitino/file/FilesetChange.java:
##########
@@ -25,7 +25,8 @@
/**
* A fileset change is a change to a fileset. It can be used to rename a
fileset, update the comment
- * of a fileset, set a property and value pair for a fileset, or remove a
property from a fileset.
+ * or type of a fileset, set a property and value pair for a fileset, or
remove a property from a
+ * fileset.
Review Comment:
The updated class-level Javadoc is grammatically ambiguous (“update the
comment or type of a fileset”). Consider rephrasing to list the capabilities
clearly (e.g., “update the comment of a fileset, update the type of a fileset,
…”), which also makes it easier to scan in generated docs.
##########
docs/filesets.md:
##########
@@ -44,8 +44,9 @@ than federating an external one, so no provider is needed
when creating it.
[Catalogs and Schemas](./catalogs-and-schemas.md). A fileset catalog usually
carries a base
`location`, and schemas can narrow it further.
-**2. Create the fileset.** Give it a name, a type, and a storage location.
Creating a managed
-fileset creates the directory; pointing at an existing path makes it external.
+**2. Create the fileset.** Give it a name, a type, and a storage location.
Choose `MANAGED` to let
+Gravitino manage the data lifecycle, or `EXTERNAL` to retain the data when the
fileset is dropped.
+Pointing at an existing path does not automatically make a fileset external.
Review Comment:
This doc change updates *create-time* semantics (“Pointing at an existing
path does not automatically make a fileset external.”), but the PR description
focuses on adding an *alter-time* conversion operation. If create behavior did
not actually change in this PR, this should be called out explicitly in the PR
description (as a doc correction) or adjusted so the documentation doesn’t
imply a behavioral change that wasn’t part of the feature.
##########
clients/client-python/gravitino/api/file/fileset_change.py:
##########
@@ -20,12 +20,13 @@
from dataclasses_json import config
+from gravitino.api.file.fileset import Fileset
from gravitino.api.secret import SecretBinding, SecretReference
class FilesetChange(ABC):
"""A fileset change is a change to a fileset. It can be used to rename a
fileset, update the comment
- of a fileset, set a property and value pair for a fileset, or remove a
property from a fileset.
+ or type of a fileset, set a property and value pair for a fileset, or
remove a property from a fileset.
"""
Review Comment:
This docstring now reads ambiguously (“update the comment or type of a
fileset”) and is split in a way that makes it harder to read. Recommend
rewriting the sentence to explicitly enumerate supported operations (rename,
update comment, update type, set/remove property) to avoid interpretation
issues in rendered docs/help() output.
--
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]
