vinothchandar commented on a change in pull request #4449:
URL: https://github.com/apache/hudi/pull/4449#discussion_r790862482
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/storage/HoodieHFileWriter.java
##########
@@ -122,7 +128,17 @@ public boolean canWrite() {
@Override
public void writeAvro(String recordKey, IndexedRecord object) throws
IOException {
- byte[] value = HoodieAvroUtils.avroToBytes((GenericRecord)object);
+ byte[] value = HoodieAvroUtils.avroToBytes((GenericRecord) object);
Review comment:
this feedback is not addressed from last round. Why do two conversions
here. Based on the schema, can't we just decide and do one `object` to `value`
conversion?
##########
File path:
hudi-common/src/main/java/org/apache/hudi/common/table/log/block/HoodieHFileDataBlock.java
##########
@@ -110,8 +105,8 @@ public HoodieLogBlockType getBlockType() {
boolean useIntegerKey = false;
int key = 0;
int keySize = 0;
- Field keyField = records.get(0).getSchema().getField(this.keyField);
- if (keyField == null) {
+ final Field schemaKeyField =
records.get(0).getSchema().getField(HoodieHFileReader.KEY_FIELD_NAME);
Review comment:
lets file a follow on JIRA to clean this hardcoding up based on my
feedback in the last round. We need the ability to pass in the top level
write/query configs into storage layer
##########
File path:
hudi-common/src/main/java/org/apache/hudi/io/storage/HoodieHFileReader.java
##########
@@ -196,6 +198,8 @@ public BloomFilter readBloomFilter() {
@Override
public Iterator getRecordIterator(Schema readerSchema) throws IOException {
final HFileScanner scanner = reader.getScanner(false, false);
+ final Option<Schema.Field> keySchemaField =
Option.ofNullable(readerSchema.getField(KEY_FIELD_NAME));
+ ValidationUtils.checkState(keySchemaField != null);
Review comment:
message for assertion failing?
##########
File path:
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java
##########
@@ -63,9 +64,9 @@
public class HoodieMetadataPayload implements
HoodieRecordPayload<HoodieMetadataPayload> {
// HoodieMetadata schema field ids
- public static final String SCHEMA_FIELD_ID_KEY = "key";
- public static final String SCHEMA_FIELD_ID_TYPE = "type";
- public static final String SCHEMA_FIELD_ID_METADATA = "filesystemMetadata";
+ public static final String SCHEMA_FIELD_NAME_KEY =
HoodieHFileReader.KEY_FIELD_NAME;
Review comment:
This naming is misleading. SCHEMA_FIELD_NAME_KEY is not a name of a
field in the metadata schema like the others.
##########
File path:
hudi-common/src/main/java/org/apache/hudi/io/storage/HoodieHFileReader.java
##########
@@ -257,16 +265,60 @@ public Option getRecordByKey(String key, Schema
readerSchema) throws IOException
}
if (value != null) {
- R record = (R)HoodieAvroUtils.bytesToAvro(value, getSchema(),
readerSchema);
+ R record = deserialize(key.getBytes(), value, getSchema(), readerSchema,
keySchemaField);
return Option.of(record);
}
return Option.empty();
}
- private R getRecordFromCell(Cell c, Schema writerSchema, Schema
readerSchema) throws IOException {
- byte[] value = Arrays.copyOfRange(c.getValueArray(), c.getValueOffset(),
c.getValueOffset() + c.getValueLength());
- return (R)HoodieAvroUtils.bytesToAvro(value, writerSchema, readerSchema);
+ /**
+ * Get the record from HBase cell.
+ *
+ * @param cell - HBase Cell
Review comment:
these javadocs are gratuitous. remove/
##########
File path:
hudi-common/src/main/java/org/apache/hudi/common/table/log/block/HoodieHFileDataBlock.java
##########
@@ -162,6 +157,20 @@ protected void createRecordsFromContentBytes() throws
IOException {
return records;
}
+ /**
+ * Serialize the record to byte buffer.
+ *
+ * @param record - Record to serialize
+ * @param schemaKeyField - Key field in the schema
+ * @return Serialized byte buffer for the record
+ */
+ private byte[] serializeRecord(final IndexedRecord record, final
Option<Field> schemaKeyField) {
+ if (schemaKeyField.isPresent()) {
+ record.put(schemaKeyField.get().pos(), "");
Review comment:
why not put by name? that would be immune to even field reorders?
##########
File path:
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieBackedMetadata.java
##########
@@ -508,6 +520,255 @@ public void
testMetadataTableWithPendingCompaction(boolean simulateFailedCompact
}
}
+ /**
+ * Test arguments - Table type, populate meta fields, exclude key from
payload.
+ */
+ public static List<Arguments> testMetadataRecordKeyExcludeFromPayloadArgs() {
Review comment:
can we just run the tests with key excluded and metafields disabled? to
save on test runtime?
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/storage/HoodieHFileWriter.java
##########
@@ -77,6 +81,8 @@ public HoodieHFileWriter(String instantTime, Path file,
HoodieHFileConfig hfileC
this.file = HoodieWrapperFileSystem.convertToHoodiePath(file, conf);
this.fs = (HoodieWrapperFileSystem) this.file.getFileSystem(conf);
this.hfileConfig = hfileConfig;
+ this.schema = schema;
+ this.schemaRecordKeyField =
Option.ofNullable(schema.getField(hfileConfig.getKeyFieldName()));
Review comment:
IMO its better its pluggable here. Gives us flexibility at this layer to
pass in something different if needed.
##########
File path:
hudi-common/src/main/java/org/apache/hudi/common/table/log/block/HoodieHFileDataBlock.java
##########
@@ -162,6 +157,20 @@ protected void createRecordsFromContentBytes() throws
IOException {
return records;
}
+ /**
+ * Serialize the record to byte buffer.
+ *
+ * @param record - Record to serialize
+ * @param schemaKeyField - Key field in the schema
+ * @return Serialized byte buffer for the record
+ */
+ private byte[] serializeRecord(final IndexedRecord record, final
Option<Field> schemaKeyField) {
Review comment:
this naming style - is bit unconventional. the schema prefix is
gratuituous, but if you want it there. should nt it be keyFieldSchema
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/storage/HoodieHFileConfig.java
##########
@@ -97,4 +99,8 @@ public BloomFilter getBloomFilter() {
public KeyValue.KVComparator getHfileComparator() {
return hfileComparator;
}
+
+ public String getKeyFieldName() {
Review comment:
lets always match field names and getters. i.e schemaKeyFieldName vs
getKeyFieldName
##########
File path:
hudi-common/src/main/java/org/apache/hudi/io/storage/HoodieHFileReader.java
##########
@@ -196,6 +198,8 @@ public BloomFilter readBloomFilter() {
@Override
public Iterator getRecordIterator(Schema readerSchema) throws IOException {
final HFileScanner scanner = reader.getScanner(false, false);
+ final Option<Schema.Field> keySchemaField =
Option.ofNullable(readerSchema.getField(KEY_FIELD_NAME));
Review comment:
this is another variant of the same naming. keySchemaField -> keyField
or keyFieldSchema
##########
File path:
hudi-common/src/main/java/org/apache/hudi/common/table/log/block/HoodieHFileDataBlock.java
##########
@@ -110,8 +105,8 @@ public HoodieLogBlockType getBlockType() {
boolean useIntegerKey = false;
int key = 0;
int keySize = 0;
- Field keyField = records.get(0).getSchema().getField(this.keyField);
- if (keyField == null) {
+ final Field schemaKeyField =
records.get(0).getSchema().getField(HoodieHFileReader.KEY_FIELD_NAME);
Review comment:
component : Code Cleanup
##########
File path:
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieBackedMetadata.java
##########
@@ -508,6 +520,255 @@ public void
testMetadataTableWithPendingCompaction(boolean simulateFailedCompact
}
}
+ /**
+ * Test arguments - Table type, populate meta fields, exclude key from
payload.
+ */
+ public static List<Arguments> testMetadataRecordKeyExcludeFromPayloadArgs() {
Review comment:
lets think of ways to reduce lines of code in this test or pull stuff
into another test?
##########
File path:
hudi-common/src/main/java/org/apache/hudi/io/storage/HoodieHFileReader.java
##########
@@ -151,15 +153,15 @@ public BloomFilter readBloomFilter() {
}
public List<Pair<String, R>> readAllRecords(Schema writerSchema, Schema
readerSchema) throws IOException {
+ final Option<Schema.Field> keySchemaField =
Option.ofNullable(readerSchema.getField(KEY_FIELD_NAME));
Review comment:
rename this?
--
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]
