yihua commented on code in PR #11597:
URL: https://github.com/apache/hudi/pull/11597#discussion_r1691875001
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java:
##########
@@ -211,7 +200,7 @@ public HoodieMetadataPayload(Option<GenericRecord>
recordOpt) {
key = record.get(KEY_FIELD_NAME).toString();
type = (int) record.get(SCHEMA_FIELD_NAME_TYPE);
- if (type == METADATA_TYPE_FILE_LIST || type ==
METADATA_TYPE_PARTITION_LIST) {
+ if (type == MetadataPartitionType.FILES.getRecordType() || type ==
MetadataPartitionType.FILES.getRecordType(RECORDKEY_PARTITION_LIST)) {
Review Comment:
If we follow this, there is no need to add `getRecordType()`.
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java:
##########
@@ -211,7 +200,7 @@ public HoodieMetadataPayload(Option<GenericRecord>
recordOpt) {
key = record.get(KEY_FIELD_NAME).toString();
type = (int) record.get(SCHEMA_FIELD_NAME_TYPE);
- if (type == METADATA_TYPE_FILE_LIST || type ==
METADATA_TYPE_PARTITION_LIST) {
+ if (type == MetadataPartitionType.FILES.getRecordType() || type ==
MetadataPartitionType.FILES.getRecordType(RECORDKEY_PARTITION_LIST)) {
Review Comment:
The idea is to keep all custom logic of each index type in a single class as
much as possible, so it's easy to add new index in the future.
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java:
##########
@@ -211,7 +200,7 @@ public HoodieMetadataPayload(Option<GenericRecord>
recordOpt) {
key = record.get(KEY_FIELD_NAME).toString();
type = (int) record.get(SCHEMA_FIELD_NAME_TYPE);
- if (type == METADATA_TYPE_FILE_LIST || type ==
METADATA_TYPE_PARTITION_LIST) {
+ if (type == MetadataPartitionType.FILES.getRecordType() || type ==
MetadataPartitionType.FILES.getRecordType(RECORDKEY_PARTITION_LIST)) {
Review Comment:
Could we avoid if-else handling here and make it extensible by incorporating
the payload handing through `MetadataPartitionType` enum? One way is to
introduce `static MetadataPartitionType get(int type)` to get the enum based on
the type ID and `void constructMetadataPayload(HoodieMetadataPayload payload,
GenericRecord record)` so that each `MetadataPartitionType` handles the payload
construction differently in `MetadataPartitionType`. Here simply calls
`MetadataPartitionType.get(type).constructMetadataPayload(this, record)`.
HoodieMetadataPayload may need to add new setters on different members like
`recordIndexMetadata` for `MetadataPartitionType#constructMetadataPayload` to
use.
--
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]
