[#4952] feat(hudi-catalog): add implementation of HMSBackend for Hudi catalog #4942
Conversation
mchades
force-pushed
the
hudi-hms
branch
2 times, most recently
from
b235c0c
to
17f93c8
Compare
mchades
changed the title
.../src/main/java/org/apache/gravitino/catalog/lakehouse/hudi/HudiSchemaPropertiesMetadata.java
Outdated
Show resolved
Hide resolved
...i/src/main/java/org/apache/gravitino/catalog/lakehouse/hudi/HudiTablePropertiesMetadata.java
Outdated
Show resolved
Hide resolved
| c -> | ||
| c.getTables(schemaIdent.name(), "*").stream() | ||
| .map(table -> NameIdentifier.of(namespace, table)) | ||
| .toArray(NameIdentifier[]::new)); |
There was a problem hiding this comment.
Will here list all the tables, not just Hudi table, should we filter out non-hudi table?
| partitioning = HiveTableConverter.getPartitioning(hmsTable); | ||
| sortOrders = HiveTableConverter.getSortOrders(hmsTable); | ||
| distribution = HiveTableConverter.getDistribution(hmsTable); |
There was a problem hiding this comment.
Does Hudi store such information in HMS, is it compatible with Hive table? As I know, for Iceberg, we need some Iceberg APIs to get partitioning, sortOrders, because Iceberg will store such information in it's metadata file, not in HMS, I guess Hudi is similar, can you please confirm this?
There was a problem hiding this comment.
comments added to the code
mchades
force-pushed
the
hudi-hms
branch
2 times, most recently
from
3d8cdf2
to
591654b
Compare
|
All comments resolved, plz help to review again when you have time, thanks! @jerryshao |
| .filter( | ||
| t -> | ||
| t.getSd().getInputFormat() != null | ||
| && t.getSd().getInputFormat().startsWith("org.apache.hudi")) |
There was a problem hiding this comment.
Can you please make this "org.apache.hudi" a static variable to avoid hard coding here? Also adding some comments about the purpose here. This is quite hacky, because if the Hudi package is changed or somehow, the assumption here will be failed.
| try { | ||
| Table table = | ||
| clientPool.run(client -> client.getTable(schemaIdent.name(), tableIdent.name())); | ||
| return HudiHMSTable.builder().withBackendTable(table).build(); |
There was a problem hiding this comment.
Shall we also make sure that the loaded table is a Hudi table, otherwise throw an exception instead?
...hudi/src/main/java/org/apache/gravitino/catalog/lakehouse/hudi/backend/hms/HudiHMSTable.java
Outdated
Show resolved
Hide resolved
| protected HudiHMSTable buildFromTable(Table hmsTable) { | ||
| name = hmsTable.getTableName(); | ||
| comment = hmsTable.getParameters().get(COMMENT); | ||
| columns = HiveTableConverter.getColumns(hmsTable, HudiColumn.builder()); |
There was a problem hiding this comment.
Is Hudi's column type exactly the same as Hive table?
There was a problem hiding this comment.
According to the table in the Hudi document, the data types supported by Hudi are fewer than those of Hive. I assume they are a subset of Hive. So I think there should be no problem with the conversion here.
…r Hudi catalog (apache#4942) support read operations for Hudi catalog HMS backend Fix: apache#4952 no UTs added
What changes were proposed in this pull request?
support read operations for Hudi catalog HMS backend
Why are the changes needed?
Fix: #4952
Does this PR introduce any user-facing change?
no
How was this patch tested?
UTs added