gopidesupavan commented on PR #62867: URL: https://github.com/apache/airflow/pull/62867#issuecomment-4155117320
> I'm not in favor of this solution, it should be refactored accordingly (see comments) so that common-sql stays provider agnostic which is not the case here. > > Make sure to remove the amazon code from common.sql, but keep it backward-compatible via common.compat. > > Check following PR on how to make this registration mechanismfor ObjectStorageProvider generic (which would in my PR translate to Dialect, the idea is the same): > > Added notion of dialects into ProvidersManager (#43726) > > Once the providers has support to register ObjectStorageProviders, you can finish this PR with the actual implementation of the provider specific code like AWS/Google/Azure. > > For that you can also check my second PR (as reference if needed) in which I actually added the different dialects, which is the equivalent of this PR. > > Introduce notion of dialects in DbApiHook (#41327) > > So I think best for a all would be to create a new PR (and reference this PR in it so we know they are linked) in which the registration mechanism is being done for any provider. And then finish this one by moving provider specific code to the corresponding providers once the other PR is finished/merged. I understand your intent, but DataFusion only requires three providers. For just these three, I wouldn’t introduce new interfaces based on dialects for each provider—it’s a relatively simple case. We can fetch the connection details using the connection ID via BaseHook. We started with in commonSQL, because DataFusion primarily works with SQL. Eventually, there’s a plan to move this entire part into a dedicated DataFusion provider that we have discussion with kaxil and me when we started working on common.ai provider. i also have discussion quite long back with @potiuk have this in separate provider. That’s why all DataFusion-related components can be organized under a single module. and move them .. Also, even if we add dailect functionality within the providers, it won’t be utilized by those providers unless it’s explicitly invoked from DataFusion. so IMHO its extra overhead for the 3 providers adding dilect interface.. -- 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]
