AWS: Fix issue that GlueTableOperations fails to initialize FileIO when GlueCatalog is not initialized with catalogProperties#5797
Conversation
|
@amogh-jahagirdar Would you mind reviewing this fix and giving some suggestions if needed? Thank you in advance for your help |
| lockManager, | ||
| catalogName, | ||
| awsProperties, | ||
| catalogProperties, |
There was a problem hiding this comment.
The catalogProperties here is always null. Hence, I replaced it with a properties() to initialize an empty ImmutableMap to avoid any NullPointerException
There was a problem hiding this comment.
Hey @JonasJ-ap, I think this means that the GlueCatalog isn't being correctly initialized in the integration tests. The initialize() call should set the value correctly. I'm guessing some of the testing init methods aren't consistent with the implementation in from the catalog interface.
There was a problem hiding this comment.
Thank you so much for your review and suggestions. Now I see that there are some integration tests using the wrong initialize() call and thus skip initializing this particular field. I will update the changes to those tests in #5784.
Meanwhile, I am curious if the return statement here is worth a change. Given that this return statement always receives null for catalogProperties and methods in GlueTableOperations do not fully handle the null case, it seems not a good idea to initialize GlueTableOperations in this way. Maybe we could still call properties() here or delete this return statement since it is unreachable under the current code logic? I appreciate your time and help.
There was a problem hiding this comment.
Yeah the expectation is that for loading the catalog dynamically, all the fields (including catalogProperties) are set via initialize() as @danielcweeks suggested, so I think let's just fix the integration test catalog initialization.
As for simplifying the code, sure I think we can have a common return point through properties(); I would say let's correct the integration tests in #5784 and then we can come back here, and re-run the tests after the common return point change. If they succeed, then we should be good.
|
@danielcweeks, @danielcweeks. Thank you for your suggestions. In this case, I will close this PR and postpone the code simplification after the correction in #5784. |
3 participants
When the user initialize
GlueCatalogwithout providing acatalogProperties, the correspondingGlueTableOperationswill face aNullPointerExceptionwhen initializing FileIO usinginitializeFileIO. This bug causes several tests in the AWS Integration test fail.before this fix:
after this fix:
The remaining test errors are either due to the test itself or outdated environment settings. Since they are not related to
GlueCatalog, they will be addressed in another PR: #5784