-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core: Use properties while initializing default HadoopFileIO for Hadoop catalog. #9283
Conversation
@nastra, Could you please help me with review on this? |
@agrawalreetika: Previously also I think only problem was properties was not considered for the default I expect PR title to be
And a simple PR description. |
Thank you @ajantha-bhat for the details. I have made the changes in the Title and description. |
PR message "to Initialise" -> to initialize |
677033b
to
b3aeea6
Compare
catalog.initialize("hadoop", catalogProps); | ||
FileIO fileIO = catalog.newTableOps(tableIdent).io(); | ||
|
||
Assertions.assertThat(fileIO.properties().get("warehouse")).isEqualTo("/hive/testwarehouse"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assertions.assertThat(fileIO.properties().get("warehouse")).isEqualTo("/hive/testwarehouse"); | |
Assertions.assertThat(fileIO.properties()).containsEntry("warehouse", "/hive/testwarehouse"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is generally better, because if the assertion ever fails, it will show the content of properties()
FileIO fileIO = catalog.newTableOps(tableIdent).io(); | ||
|
||
Assertions.assertThat(fileIO.properties().get("warehouse")).isEqualTo("/hive/testwarehouse"); | ||
Assertions.assertThat(fileIO.properties().get("io.manifest.cache-enabled")).isEqualTo("true"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assertions.assertThat(fileIO.properties().get("io.manifest.cache-enabled")).isEqualTo("true"); | |
Assertions.assertThat(fileIO.properties()).containsEntry("io.manifest.cache-enabled", "true"); |
b3aeea6
to
cbca4ed
Compare
@@ -110,13 +110,18 @@ public void initialize(String name, Map<String, String> properties) { | |||
|
|||
this.catalogName = name; | |||
this.warehouseLocation = LocationUtil.stripTrailingSlash(inputWarehouseLocation); | |||
|
|||
if (conf == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I just saw this change now. Why do we need this? I think this should be reverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nastra If Hadoop Configuration is not set then subsequent would have NullPointer Excpetion. While adding the Tests I noticed we don't have conf check in HadoopCatalog
the way we have it in HiveCatalog
I can explicitly initialize conf in my Tests if you think this check is not required here. Let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conf
always needs to be set explicitly. This is because the HadoopCatalog
(and a few others) implement Configurable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to revert the change that creates a conf
when it was null
cbca4ed
to
656248b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, thanks @agrawalreetika !
Merged, thanks @agrawalreetika and thanks for reviews @nastra @ajantha-bhat ! |
Make HadoopFileIO as default for Hadoop Catalog to initialize HadoopFileIO catalog configs similar to NessieCatalog