HonahX commented on code in PR #922: URL: https://github.com/apache/iceberg-python/pull/922#discussion_r1677094692
########## mkdocs/docs/configuration.md: ########## @@ -22,35 +22,11 @@ hide: - under the License. --> -# Catalogs - -PyIceberg currently has native support for REST, SQL, Hive, Glue and DynamoDB. - -There are three ways to pass in configuration: - -- Using the `~/.pyiceberg.yaml` configuration file -- Through environment variables -- By passing in credentials through the CLI or the Python API - -The configuration file is recommended since that's the easiest way to manage the credentials. - -Another option is through environment variables: - -```sh -export PYICEBERG_CATALOG__DEFAULT__URI=thrift://localhost:9083 -export PYICEBERG_CATALOG__DEFAULT__S3__ACCESS_KEY_ID=username -export PYICEBERG_CATALOG__DEFAULT__S3__SECRET_ACCESS_KEY=password -``` - -The environment variable picked up by Iceberg starts with `PYICEBERG_` and then follows the yaml structure below, where a double underscore `__` represents a nested field, and the underscore `_` is converted into a dash `-`. - -For example, `PYICEBERG_CATALOG__DEFAULT__S3__ACCESS_KEY_ID`, sets `s3.access-key-id` on the `default` catalog. - -# Tables +## Tables Review Comment: Adding the extra `#` so that the `Table of contents` sidebar can be rendered correctly:  ########## pyiceberg/catalog/dynamodb.py: ########## @@ -78,17 +84,32 @@ ACTIVE = "ACTIVE" ITEM = "Item" +DYNAMODB_PROFILE_NAME = "dynamodb.profile-name" +DYNAMODB_REGION = "dynamodb.region" +DYNAMODB_ACCESS_KEY_ID = "dynamodb.access-key-id" +DYNAMODB_SECRET_ACCESS_KEY = "dynamodb.secret-access-key" +DYNAMODB_SESSION_TOKEN = "dynamodb.session-token" + class DynamoDbCatalog(MetastoreCatalog): def __init__(self, name: str, **properties: str): super().__init__(name, **properties) + + from pyiceberg.table import PropertyUtil + session = boto3.Session( - profile_name=properties.get("profile_name"), - region_name=properties.get("region_name"), - botocore_session=properties.get("botocore_session"), - aws_access_key_id=properties.get("aws_access_key_id"), - aws_secret_access_key=properties.get("aws_secret_access_key"), - aws_session_token=properties.get("aws_session_token"), + profile_name=PropertyUtil.get_first_property_value(properties, DYNAMODB_PROFILE_NAME, DEPRECATED_PROFILE_NAME), + region_name=PropertyUtil.get_first_property_value(properties, DYNAMODB_REGION, AWS_REGION, DEPRECATED_REGION), + botocore_session=properties.get(DEPRECATED_BOTOCORE_SESSION), Review Comment: I think it may be more reasonable to stop exposing the `botocore_session` configuration: 1. Unlike other configs which takes string, the [botocore_session](https://boto3.amazonaws.com/v1/documentation/api/latest/reference/core/session.html) takes a botocore.Session instance, which is not aligned with the convention of catalog properties to be a `Dict[str, str]` 2. I think we should hide this level of details from users. ########## pyiceberg/table/__init__.py: ########## @@ -254,6 +254,13 @@ def property_as_bool(properties: Dict[str, str], property_name: str, default: bo return value.lower() == "true" return default + @staticmethod + def get_first_property_value(properties: Properties, *property_names: str) -> Optional[Any]: + for property_name in property_names: + if property_value := properties.get(property_name): + return property_value + return None + Review Comment: This may be in a follow-up PR: I am considering whether it is worth moving the `PropertyUtil` class to a module in utils. Currently, PropertyUtil is used in the table, catalog, and io modules. Given its wide usage and the fact that it is a standalone utility class, I believe it can be extracted from the table module. -- 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
