dennishuo commented on code in PR #724:
URL: https://github.com/apache/polaris/pull/724#discussion_r1919406621
##########
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:
I still think we should just pass in the PolarisResolvedPathWrapper, which
is the superset of all the other suggested fields in a way that doesn't
prematurely mix or cherrypick fields from the entity.
1. Even if *today* the only "interesting" field in the resolved entity is
`internalProperties` that doesn't always have to be true. If we pass the
resolved entity, then in the future if we introduce additional structured
fields, it doesn't change what we're plumbing into the factory
2. The semantic here is to make it so that BasePolarisCatalog isn't trying
to cherrypick which "interesting" properties we think a factory might need.
Give the factory all the info to make a decision. Even if we decided to extract
specifically the `PolarisStorageConfigurationInfo` that's also trying to state
that we think the `PolarisStorageConfigurationInfo` is the only thing the
factory might need, which isn't true
3. My other reason for suggesting `PolarisResolvedPathWrapper` is that we
actually then get the entity hierarchy as well in the factory. There are
various reasons why custom factories might need the full hierarchy.
--
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]
