joyhaldar commented on PR #4050: URL: https://github.com/apache/polaris/pull/4050#issuecomment-4161986291
> I’m aligned with the direction here overall. I think there are two high-priority issues worth tightening before this moves forward: > > 1. The current BIGQUERY auth contract does not look aligned yet. IIUC, the API/model path still allows `authenticationParameters` to be omitted, while the runtime only supports `IMPLICIT` auth and later dereferences that field as if it were always present. That feels like more than a local null-safety issue, the boundary contract still looks looser than the implementation. > 2. Since this adds a new federation connector across OpenAPI, model conversion, and runtime wiring, I think we should have at least one round-trip/ wiring test for `BIGQUERY` before the contract settles. > > 2 cents: this PR also seems like a useful point to clarify whether federation here is aiming for a common Polaris contract over native Iceberg catalogs, or for backend-specific adapters behind one REST surface. I think that answer affects how strict/explicit the connector contract should be. Thank you for the detailed review. I have tried to address both points: 1. Added null checks in the factory and `asConnectionConfigInfoModel` to handle omitted `authenticationParameters`, which defaults to ADC. 2. Added `testBigQueryMetastoreConnectionConfig` in `ConnectionConfigInfoDpoTest` for round-trip validation. Please let me know if you'd like any changes. -- 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]
