XJDKC commented on code in PR #724:
URL: https://github.com/apache/polaris/pull/724#discussion_r1919343072
##########
service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -1627,6 +1627,12 @@ private FileIO refreshIOWithCredentials(
// the credentials should always override table-level properties, since
// storage configuration will be found at whatever entity defines it
tableProperties.putAll(credentialsMap);
+
+ // Populate the internal properties to FileIO in case the FileIO
implementation needs them
+ Map<String, String> internalProperties =
+
storageInfoEntity.map(PolarisEntity::getInternalPropertiesAsMap).orElse(Map.of());
Review Comment:
Thanks for sharing your thoughts!
To inject dynamic properties from persistence layer to `BasePolarisCatalog`,
the dynamic properties will be attached to the entity object. So PolarisEntity
it serves as the data source, while `FileIO` acts as the data consumer. A
translation layer between them is indeed necessary.
Regarding the `PolarisCatalogConfig` solution, it does involve some
translation in `BasePolarisCatalog`, where we need to derive a
`PolarisCatalogConfig` instance from the `PolarisEntity` object. However, this
approach seems limiting. For instance, while we can infer the `type` from
`PolarisStorageConfigurationInfo`, it doesn’t provide a flexible way to handle
custom types like `MY-CUSTOM-S3` and passing additional properties to
`FileIOFactory`. If the translation logic is fixed in BasePolarisCatalog, we
lose the flexibility to adapt to varying types dynamically. i.e. How could we
pass `MY-CUSTOM-S3` and additional properties to `FileIOFactory` with a fixed
translation logic in OSS Polaris? We need to come up with a solution for it.
By shifting the translation logic to `FileIOFactory`, we centralize and
streamline it. This ensures that the `FileIOFactory` can directly interpret the
dynamic properties and handle custom logic, such as supporting `MY-CUSTOM-S3`,
without relying on a predefined translation mechanism in `BasePolarisCatalog`.
Additionally, even if we pass a general type like `S3` to `FileIOFactory`,
it can still interpret dynamic properties to handle specific cases like
`MY-CUSTOM-S3`. This ensures extensibility and keeps BasePolarisCatalog more
generic.
For the short term solution, I think we can just still pass `impl` to
`FileIOFactory` while passing additional `internalProperties`. `impl` is a kind
of `type` from `FileIOFactory` perspective.
I think both solutions are reasonable, I'm okay with either options but
prefer the short term solution.
--
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]
