yihua commented on code in PR #11453:
URL: https://github.com/apache/hudi/pull/11453#discussion_r1717932624
##########
hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java:
##########
@@ -72,15 +73,16 @@ public void
setSchemaHandler(HoodieFileGroupReaderSchemaHandler<T> schemaHandler
this.schemaHandler = schemaHandler;
}
- public String getTablePath() {
- if (tablePath == null) {
- throw new IllegalStateException("Table path not set in reader context.");
- }
- return tablePath;
+ public StoragePath getTablePath() {
+ return metaClient.getBasePath();
Review Comment:
Verify that meta client is set and non-null?
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/HoodieFileGroupReader.java:
##########
@@ -105,7 +103,7 @@ public HoodieFileGroupReader(HoodieReaderContext<T>
readerContext,
HoodieTableConfig tableConfig = hoodieTableMetaClient.getTableConfig();
this.recordMerger =
readerContext.getRecordMerger(tableConfig.getRecordMergerStrategy());
readerContext.setRecordMerger(this.recordMerger);
Review Comment:
Could you file a follow-up ticket to refactor the reader context
construction? It's still an anti-pattern to allow any setters which make it
mutable and not safe.
##########
hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java:
##########
@@ -72,15 +73,16 @@ public void
setSchemaHandler(HoodieFileGroupReaderSchemaHandler<T> schemaHandler
this.schemaHandler = schemaHandler;
}
- public String getTablePath() {
- if (tablePath == null) {
- throw new IllegalStateException("Table path not set in reader context.");
- }
- return tablePath;
+ public StoragePath getTablePath() {
+ return metaClient.getBasePath();
+ }
+
+ public HoodieTableMetaClient getMetaClient() {
+ return metaClient;
}
- public void setTablePath(String tablePath) {
- this.tablePath = tablePath;
+ public void setMetaClient(HoodieTableMetaClient metaClient) {
Review Comment:
`HoodieReaderContext` is meant to handle only engine-specific implementation
on reading data files, getting field values from a record, transforming a
record, etc., but now it's getting more and more complex and hard to maintain
by mixing Hudi-specific logic which should be completely avoided. Keep this in
mind. If you need to have common logic for Hudi-specific logic, let's add a
new class that can be used across engines without any changes, just like
`HoodieFileGroupReader`, and keep this class lean.
##########
hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java:
##########
@@ -72,15 +73,16 @@ public void
setSchemaHandler(HoodieFileGroupReaderSchemaHandler<T> schemaHandler
this.schemaHandler = schemaHandler;
}
- public String getTablePath() {
- if (tablePath == null) {
- throw new IllegalStateException("Table path not set in reader context.");
- }
- return tablePath;
+ public StoragePath getTablePath() {
+ return metaClient.getBasePath();
+ }
+
+ public HoodieTableMetaClient getMetaClient() {
+ return metaClient;
}
- public void setTablePath(String tablePath) {
- this.tablePath = tablePath;
+ public void setMetaClient(HoodieTableMetaClient metaClient) {
Review Comment:
Rethinking this, should we keep meta client out of the reader context?
##########
hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java:
##########
@@ -72,15 +73,16 @@ public void
setSchemaHandler(HoodieFileGroupReaderSchemaHandler<T> schemaHandler
this.schemaHandler = schemaHandler;
}
- public String getTablePath() {
- if (tablePath == null) {
- throw new IllegalStateException("Table path not set in reader context.");
- }
- return tablePath;
+ public StoragePath getTablePath() {
+ return metaClient.getBasePath();
+ }
+
+ public HoodieTableMetaClient getMetaClient() {
+ return metaClient;
}
- public void setTablePath(String tablePath) {
- this.tablePath = tablePath;
+ public void setMetaClient(HoodieTableMetaClient metaClient) {
Review Comment:
Could you file a ticket on this as a follow-up?
--
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]
