wombatu-kun commented on code in PR #19247:
URL: https://github.com/apache/hudi/pull/19247#discussion_r3564196340
##########
hudi-common/src/main/java/org/apache/hudi/common/util/FileFormatUtils.java:
##########
@@ -231,6 +231,18 @@ public String[] readMinMaxRecordKeys(HoodieStorage
storage, StoragePath filePath
public abstract Map<String, String> readFooter(HoodieStorage storage,
boolean required, StoragePath filePath,
String... footerNames);
+ /**
+ * Read data file footer entries whose keys start with the given prefix.
+ *
+ * @param storage {@link HoodieStorage} instance
+ * @param required require at least one matching footer entry
+ * @param filePath data file path
+ * @param footerPrefix prefix of the footer entries to read
+ * @return matching footer entries keyed by their full names
+ */
+ public abstract Map<String, String> readFooterWithPrefix(
Review Comment:
`HeaderMetadataType` is a closed enum and `fromFooterMetadata` drops any key
whose suffix is not one of its values, so a prefix scan cannot surface anything
an explicit key list would miss - including in the forward-compat case, where
an old reader discards a new writer's unknown type under either design. Both
call sites could pass an enum-derived key array to the existing varargs
`readFooter`, which drops this abstract method, its four implementations, and
the two new public methods on `HFileReader` / `HFileInfo` in `hudi-io`.
If the prefix API is kept deliberately, consider adding one `protected
abstract Map<String, String> readAllFooterEntries(...)` and implementing
`readFooterWithPrefix` concretely here, so the filter-then-`required`-check is
written once rather than copy-pasted across the four impls with four different
message strings.
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/common/util/LanceUtils.java:
##########
@@ -166,6 +166,37 @@ public Map<String, String> readFooter(HoodieStorage
storage, boolean required, S
}
}
+ @Override
+ public Map<String, String> readFooterWithPrefix(
Review Comment:
This impl has no coverage.
`TestHoodieSparkLanceWriter#testFooterMetadataIsWrittenToLanceSchemaMetadata`
already asserts `readFooter` for Lance both ways, and the PR extended the HFile
and ORC footer tests with the matching `readFooterWithPrefix` assertions, but
Lance was skipped. No native-log e2e test reaches Lance either, since
`HoodieFileSliceTestUtils.createNativeLogWriter` only branches HFILE/Parquet -
so nothing exercises the allocator or the `catch (MetadataNotFoundException) {
throw e; }` arm that keeps the trailing `catch (Exception)` from re-wrapping it.
Adding the same two assertions next to the existing `readFooter` ones in
that test would close it.
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/io/hadoop/TestHoodieHFileReaderWriter.java:
##########
@@ -208,19 +209,25 @@ protected void verifySchema(HoodieStorage storage, String
schemaPath) throws IOE
public void testReadFooterMetadata() throws Exception {
HoodieSchema schema =
getSchemaFromResource(TestHoodieOrcReaderWriter.class,
"/exampleSchemaWithMetaFields.avsc");
HoodieAvroHFileWriter writer = createWriter(schema, false);
+ String schemaMetadataKey =
NativeLogFooterMetadata.getFooterMetadataKey(HeaderMetadataType.SCHEMA);
Map<String, String> footerMetadata = new TreeMap<>();
- footerMetadata.put(NativeLogFooterMetadata.FOOTER_METADATA_KEY,
"{\"SCHEMA\":\"schema\"}");
+ footerMetadata.put(schemaMetadataKey, "schema");
footerMetadata.put("custom", "value");
writer.addFooterMetadata(footerMetadata);
writer.close();
HoodieStorage storage = HoodieTestUtils.getStorage(getFilePath());
Map<String, String> footer = new HFileUtils().readFooter(
- storage, false, getFilePath(),
NativeLogFooterMetadata.FOOTER_METADATA_KEY, "custom", "missing");
+ storage, false, getFilePath(), schemaMetadataKey, "custom", "missing");
assertEquals(2, footer.size());
- assertEquals("{\"SCHEMA\":\"schema\"}",
footer.get(NativeLogFooterMetadata.FOOTER_METADATA_KEY));
+ assertEquals("schema", footer.get(schemaMetadataKey));
assertEquals("value", footer.get("custom"));
+
+ Map<String, String> footerByPrefix = new HFileUtils().readFooterWithPrefix(
+ storage, true, getFilePath(),
NativeLogFooterMetadata.FOOTER_METADATA_KEY_PREFIX);
Review Comment:
`required=true` is passed here with a matching entry present, so the new
`MetadataNotFoundException` branch never executes - and the same holds for the
ORC assertion and for the Parquet and Lance impls, leaving that branch untested
across all four. The `assertThrows` below covers the old `readFooter`, not this
method.
One `assertThrows(MetadataNotFoundException.class, () -> new
HFileUtils().readFooterWithPrefix(storage, true, getFilePath(),
"no.such.prefix."))` here, and the equivalent in the ORC test, would pin it.
##########
hudi-common/src/main/java/org/apache/hudi/common/util/HFileUtils.java:
##########
@@ -128,6 +128,31 @@ public Map<String, String> readFooter(HoodieStorage
storage, boolean required, S
}
}
+ @Override
+ public Map<String, String> readFooterWithPrefix(
+ HoodieStorage storage, boolean required, StoragePath filePath, String
footerPrefix) {
+ Map<String, String> footerVals = new HashMap<>();
+ try (HFileReader reader = HFileReaderFactory.builder()
+ .withStorage(storage)
+ .withPath(filePath)
+ .build()
+ .createHFileReader()) {
+ reader.getMetaInfo().forEach((key, value) -> {
+ String footerName = fromUTF8Bytes(key.getBytes());
Review Comment:
`Key.getBytes()` returns the backing array and ignores `offset`/`length`.
This is correct today only because every `UTF8StringKey` happens to be built
over an exact-size array (`HFileFileInfoBlock` constructs them from
`ByteString.toByteArray()`), and it breaks silently the moment one is
constructed over a shared buffer. `key.getContentInString()` is the offset-safe
accessor, and is what `HoodieNativeAvroHFileReader` and
`HFileBootstrapIndexReader` use.
##########
hudi-io/src/main/java/org/apache/hudi/io/hfile/HFileReader.java:
##########
@@ -78,6 +79,14 @@ public interface HFileReader extends Closeable {
*/
Option<byte[]> getMetaInfo(UTF8StringKey key) throws IOException;
+ /**
+ * Gets all entries from the file info block.
+ *
+ * @return all file info entries
+ * @throws IOException upon read errors
+ */
+ Map<UTF8StringKey, byte[]> getMetaInfo() throws IOException;
Review Comment:
Overloading `getMetaInfo` by arity gives the two methods unrelated return
types (`Option<byte[]>` for a single key, the whole map here), so
`reader.getMetaInfo()` reads at a call site as if it returned one value.
`getAllMetaInfo()` would be unambiguous.
The javadoc should also state that the returned map is an unmodifiable view,
since `HFileInfo.getInfoMap()` wraps it in `Collections.unmodifiableMap` and
callers cannot otherwise tell.
##########
rfc/rfc-103/rfc-103.md:
##########
@@ -223,19 +223,21 @@ Footer
- key-value metadata <-- Hudi log format metadata goes in here
```
-Hudi log format metadata can be stored as a single entry in the Parquet footer
with key = `hudi.log.format.metadata` and value being a serialized JSON of the
metadata entries:
-
-| Hudi log format metadata |
-|:---------------------------------------------|
-| `VERSION` |
-| `INSTANT_TIME` |
-| `TARGET_INSTANT_TIME` |
-| `COMMAND_BLOCK_TYPE` |
-| `COMPACTED_BLOCK_TIMES` |
-| `RECORD_POSITIONS` |
-| `BLOCK_IDENTIFIER` |
-| `IS_PARTIAL` |
-| `BASE_FILE_INSTANT_TIME_OF_RECORD_POSITIONS` |
+Each Hudi log format metadata value is stored as a separate Parquet footer
entry under the
Review Comment:
This restores the exact per-key mapping table that was removed during the
RFC-103 review. In
https://github.com/apache/hudi/pull/17827#discussion_r2830722993 @vinothchandar
marked it IMPORTANT ("Can't we just store this in a single entry as a
serialized map... Anyways, parquet readers will read all footer entries at
once. We don't get any benefit by splitting them out?") and @xushiyan agreed to
make it a single entry, which is how the RFC merged and how #19067 / #19072
implemented it two weeks ago.
Issue #19246 concedes the I/O point ("Parquet already reads the complete
footer, so this is mainly a cleaner contract"), so the case rests on the
tooling/escaping arguments, which were not on the table then. Worth linking
that thread in the description and getting an ack from @vinothchandar and
@xushiyan before flipping the on-disk contract back, since they are RFC-103's
approver and proposer.
--
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]
