Copilot commented on code in PR #9570:
URL: https://github.com/apache/gravitino/pull/9570#discussion_r2744957847
##########
clients/client-python/gravitino/filesystem/gvfs_config.py:
##########
@@ -85,3 +85,8 @@ class GVFSConfig:
# The configuration key for whether to enable auto-creation of fileset
location when the
# server-side filesystem ops are disabled and the location does not exist.
The default is true.
GVFS_FILESYSTEM_AUTO_CREATE_LOCATION = "auto_create_location"
+
+ # The configuration prefix for user-defined path-specific configurations.
+ # Format: fs.path.config.<base_location>.<property_name>=<property_value>
+ # Example: fs.path.config.s3.aws-access-key=XXX
Review Comment:
The docstring/comments for FS_GRAVITINO_PATH_CONFIG_PREFIX describe the
format as "fs.path.config.<base_location>.<property_name>=..." with example
"fs.path.config.s3.aws-access-key=...", but the implementation/tests use a
two-step mapping: "fs.path.config.<locationName>=<base_location>" plus
"fs.path.config.<locationName>.<property>=<value>". Please update the comment
to match the actual configuration format to avoid misleading users.
```suggestion
# Configuration is defined in two steps:
# 1) Map a logical location name to a base location:
# fs.path.config.<location_name>=<base_location>
# 2) Define properties for that logical location:
# fs.path.config.<location_name>.<property_name>=<property_value>
# Example:
# fs.path.config.my_s3_location=s3://bucket/path
# fs.path.config.my_s3_location.aws-access-key=XXX
```
##########
clients/client-python/gravitino/filesystem/gvfs_base_operations.py:
##########
@@ -489,6 +493,30 @@ def _get_actual_filesystem(
fileset_ident, location_name
)
+ def _merge_fileset_properties(
+ self,
+ catalog: FilesetCatalog,
+ schema: Schema,
+ fileset: Fileset,
+ actual_location: str,
+ ) -> Dict[str, str]:
+ """Merge properties from catalog, schema, fileset, options, and
user-defined configs.
+ :param catalog: The fileset catalog
+ :param schema: The schema
+ :param fileset: The fileset
+ :param actual_location: The actual storage location
+ :return: Merged properties dictionary
+ """
+ fileset_props = dict(catalog.properties())
+ fileset_props.update(schema.properties())
+ fileset_props.update(fileset.properties())
Review Comment:
_merge_fileset_properties() assumes catalog.properties() and
schema.properties() are dicts, but SchemaDTO.properties() can return None (it
stores Optional[Dict]) and CatalogDTO can be constructed with properties=None.
dict(catalog.properties()) and fileset_props.update(schema.properties()) will
raise at runtime in those cases. Please defensively treat missing properties as
empty dicts before merging (e.g., (catalog.properties() or {}),
(schema.properties() or {}), (fileset.properties() or {})).
```suggestion
fileset_props = dict(catalog.properties() or {})
fileset_props.update(schema.properties() or {})
fileset_props.update(fileset.properties() or {})
```
--
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]
