You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "cshuo (via GitHub)" <gi...@apache.org> on 2025/06/09 03:50:36 UTC
[PR] [HUDI-9499] Use customized IndexedRecord to decrease spilling probabi… [hudi]
cshuo opened a new pull request, #13408:
URL: https://github.com/apache/hudi/pull/13408
…lity of FileGroup reader
### Change Logs
Reduce size of Avro IndexedRecord to decrease spilling probability of FIleGroup reader.
### Impact
Solve potential perf regression for Avro FileGroup Reader.
### Risk level (write none, low medium or high below)
medium.
### Documentation Update
_Describe any necessary documentation update if there is any new feature, config, or user-facing change. If not, put "none"._
- _The config description must be updated if new configs are added or the default value of the configs are changed_
- _Any new feature or user-facing change requires updating the Hudi website. Please create a Jira ticket, attach the
ticket number here and follow the [instruction](https://hudi.apache.org/contribute/developer-setup#website) to make
changes to the website._
### Contributor's checklist
- [ ] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute)
- [ ] Change Logs and Impact were stated clearly
- [ ] Adequate tests were added if applicable
- [ ] CI passed
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "cshuo (via GitHub)" <gi...@apache.org>.
cshuo commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2139039505
##########
hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java:
##########
@@ -183,6 +187,14 @@ public Option<Predicate> getKeyFilterOpt() {
return keyFilterOpt;
}
+ public SizeEstimator<BufferedRecord<T>> getRecordSizeEstimator() {
+ return new HoodieRecordSizeEstimator<>(schemaHandler.getRequiredSchema());
+ }
+
+ public CustomSerializer<BufferedRecord<T>> getRecordSerializer() {
+ return new DefaultSerializer<>();
Review Comment:
Currently the inner serializer of `BufferedRecordSerializer` is the new added interface`RecordSerializer`, I guess you mean using BufferedRecordSerializer with the default implementation of `RecordSerializer`.
> specialized handler of the BufferedRecord will help with general performance.
One clear benefit is the size of serialized bytes will be smaller for `BufferedRecordSerializer` comparing with `DefaultSerializer `.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "cshuo (via GitHub)" <gi...@apache.org>.
cshuo commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2139039505
##########
hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java:
##########
@@ -183,6 +187,14 @@ public Option<Predicate> getKeyFilterOpt() {
return keyFilterOpt;
}
+ public SizeEstimator<BufferedRecord<T>> getRecordSizeEstimator() {
+ return new HoodieRecordSizeEstimator<>(schemaHandler.getRequiredSchema());
+ }
+
+ public CustomSerializer<BufferedRecord<T>> getRecordSerializer() {
+ return new DefaultSerializer<>();
Review Comment:
> use BufferedRecordSerializer with the DefaultSerializer
Currently the inner serializer of `BufferedRecordSerializer` is the new added interface`RecordSerializer`, I guess you mean using BufferedRecordSerializer with the default implementation of `RecordSerializer`.
> specialized handler of the BufferedRecord will help with general performance.
One clear benefit is the size of serialized bytes will be smaller for `BufferedRecordSerializer` comparing with `DefaultSerializer `.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Use customized IndexedRecord to decrease spilling probabi… [hudi]
Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2136882150
##########
hudi-common/src/main/java/org/apache/hudi/avro/BinaryIndexedRecord.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.avro;
+
+import org.apache.hudi.exception.HoodieException;
+
+import org.apache.avro.Schema;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.avro.generic.IndexedRecord;
+
+import java.io.IOException;
+
+/**
+ * An implementation of {@link IndexedRecord} which contains serialized bytes of an {@link IndexedRecord}
+ * only, and it's used for buffering in {@code ExternalSpillableMap}, and should be deserialized to object
+ * based {@link IndexedRecord} for merging or iterating.
+ */
+public class BinaryIndexedRecord implements IndexedRecord {
Review Comment:
sinked offline, we will add customized size estimator and serializer for avro records to keep in line with the legacy log scanner behavior first.
##########
hudi-common/src/main/java/org/apache/hudi/avro/BinaryIndexedRecord.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.avro;
+
+import org.apache.hudi.exception.HoodieException;
+
+import org.apache.avro.Schema;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.avro.generic.IndexedRecord;
+
+import java.io.IOException;
+
+/**
+ * An implementation of {@link IndexedRecord} which contains serialized bytes of an {@link IndexedRecord}
+ * only, and it's used for buffering in {@code ExternalSpillableMap}, and should be deserialized to object
+ * based {@link IndexedRecord} for merging or iterating.
+ */
+public class BinaryIndexedRecord implements IndexedRecord {
Review Comment:
synced offline, we will add customized size estimator and serializer for avro records to keep in line with the legacy log scanner behavior first.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Use customized IndexedRecord to decrease spilling probabi… [hudi]
Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2136729325
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java:
##########
@@ -376,14 +377,27 @@ public Option<IndexedRecord> combineAndGetUpdateValue(IndexedRecord oldRecord, S
}
@Override
- public Option<IndexedRecord> getInsertValue(Schema schemaIgnored, Properties propertiesIgnored) throws IOException {
+ public Option<IndexedRecord> getInsertValue(Schema schema, Properties propertiesIgnored) throws IOException {
if (key == null || this.isDeletedRecord) {
return Option.empty();
}
- HoodieMetadataRecord record = new HoodieMetadataRecord(key, type, filesystemMetadata, bloomFilterMetadata,
- columnStatMetadata, recordIndexMetadata, secondaryIndexMetadata);
- return Option.of(record);
+ if (schema != null && schema.getField(HoodieRecord.COMMIT_TIME_METADATA_FIELD) != null) {
+ GenericRecord result = new GenericData.Record(schema);
Review Comment:
not sure why we need 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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Use customized IndexedRecord to decrease spilling probabi… [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #13408:
URL: https://github.com/apache/hudi/pull/13408#issuecomment-2954558583
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* c3747e2e221dac7cb7ecc78f15619bbbad1a09a6 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "cshuo (via GitHub)" <gi...@apache.org>.
cshuo commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2139092656
##########
hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java:
##########
@@ -183,6 +187,14 @@ public Option<Predicate> getKeyFilterOpt() {
return keyFilterOpt;
}
+ public SizeEstimator<BufferedRecord<T>> getRecordSizeEstimator() {
+ return new HoodieRecordSizeEstimator<>(schemaHandler.getRequiredSchema());
+ }
+
+ public CustomSerializer<BufferedRecord<T>> getRecordSerializer() {
+ return new DefaultSerializer<>();
Review Comment:
Btw, local benchmark results for `BufferedRecordSerializer(AvroRecordSerializer)` with `DefualtSerializer`:
~~Legacy default: 1801s~~
Legacy default: 1268s
Legacy default: 1217s
Legacy default: 1204s
Legacy default: 1210s
Avg: 1224
~~New: 769s~~
New: 640s
New: 646s
New: 656s
New: 643s
Avg: 646
The Ser/De throughput of new serializer is about 2x of before.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #13408:
URL: https://github.com/apache/hudi/pull/13408#issuecomment-2960994851
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6081",
"triggerID" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"triggerType" : "PUSH"
}, {
"hash" : "939cadd2886028c09f13695862d88337333aaea2",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6088",
"triggerID" : "939cadd2886028c09f13695862d88337333aaea2",
"triggerType" : "PUSH"
}, {
"hash" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"status" : "CANCELED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6107",
"triggerID" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"triggerType" : "PUSH"
}, {
"hash" : "56325d993f668f220a9b54f8d6a3db2be333c4c6",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "56325d993f668f220a9b54f8d6a3db2be333c4c6",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 90fffa4f58d1f646409af51f89e104e87cd78085 Azure: [CANCELED](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6107)
* 56325d993f668f220a9b54f8d6a3db2be333c4c6 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Use customized IndexedRecord to decrease spilling probabi… [hudi]
Posted by "the-other-tim-brown (via GitHub)" <gi...@apache.org>.
the-other-tim-brown commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2135938929
##########
hudi-common/src/main/java/org/apache/hudi/avro/BinaryIndexedRecord.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.avro;
+
+import org.apache.hudi.exception.HoodieException;
+
+import org.apache.avro.Schema;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.avro.generic.IndexedRecord;
+
+import java.io.IOException;
+
+/**
+ * An implementation of {@link IndexedRecord} which contains serialized bytes of an {@link IndexedRecord}
+ * only, and it's used for buffering in {@code ExternalSpillableMap}, and should be deserialized to object
+ * based {@link IndexedRecord} for merging or iterating.
+ */
+public class BinaryIndexedRecord implements IndexedRecord {
Review Comment:
I think the issue is that the serializer includes the full schema. Instead of creating a custom IndexedRecord, can this be fixed at the serializer level?
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Use customized IndexedRecord to decrease spilling probabi… [hudi]
Posted by "the-other-tim-brown (via GitHub)" <gi...@apache.org>.
the-other-tim-brown commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2136746011
##########
hudi-common/src/main/java/org/apache/hudi/avro/BinaryIndexedRecord.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.avro;
+
+import org.apache.hudi.exception.HoodieException;
+
+import org.apache.avro.Schema;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.avro.generic.IndexedRecord;
+
+import java.io.IOException;
+
+/**
+ * An implementation of {@link IndexedRecord} which contains serialized bytes of an {@link IndexedRecord}
+ * only, and it's used for buffering in {@code ExternalSpillableMap}, and should be deserialized to object
+ * based {@link IndexedRecord} for merging or iterating.
+ */
+public class BinaryIndexedRecord implements IndexedRecord {
Review Comment:
Right, the code that translates the in memory representation to the binary format is the serializer and there is also an estimator that can be provided if the default is not matching the actual size of the binary format
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #13408:
URL: https://github.com/apache/hudi/pull/13408#issuecomment-2960284489
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6081",
"triggerID" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"triggerType" : "PUSH"
}, {
"hash" : "939cadd2886028c09f13695862d88337333aaea2",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6088",
"triggerID" : "939cadd2886028c09f13695862d88337333aaea2",
"triggerType" : "PUSH"
}, {
"hash" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 939cadd2886028c09f13695862d88337333aaea2 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6088)
* 90fffa4f58d1f646409af51f89e104e87cd78085 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Use customized IndexedRecord to decrease spilling probabi… [hudi]
Posted by "the-other-tim-brown (via GitHub)" <gi...@apache.org>.
the-other-tim-brown commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2136774717
##########
hudi-common/src/main/java/org/apache/hudi/avro/BinaryIndexedRecord.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.avro;
+
+import org.apache.hudi.exception.HoodieException;
+
+import org.apache.avro.Schema;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.avro.generic.IndexedRecord;
+
+import java.io.IOException;
+
+/**
+ * An implementation of {@link IndexedRecord} which contains serialized bytes of an {@link IndexedRecord}
+ * only, and it's used for buffering in {@code ExternalSpillableMap}, and should be deserialized to object
+ * based {@link IndexedRecord} for merging or iterating.
+ */
+public class BinaryIndexedRecord implements IndexedRecord {
Review Comment:
Doesn't this incur extra conversion though resulting in a performance regression if there is no need to spill to disk? This seems like it is not addressing underlying issue of the estimator and serializer.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Use customized IndexedRecord to decrease spilling probabi… [hudi]
Posted by "cshuo (via GitHub)" <gi...@apache.org>.
cshuo commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2136761651
##########
hudi-common/src/main/java/org/apache/hudi/avro/BinaryIndexedRecord.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.avro;
+
+import org.apache.hudi.exception.HoodieException;
+
+import org.apache.avro.Schema;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.avro.generic.IndexedRecord;
+
+import java.io.IOException;
+
+/**
+ * An implementation of {@link IndexedRecord} which contains serialized bytes of an {@link IndexedRecord}
+ * only, and it's used for buffering in {@code ExternalSpillableMap}, and should be deserialized to object
+ * based {@link IndexedRecord} for merging or iterating.
+ */
+public class BinaryIndexedRecord implements IndexedRecord {
Review Comment:
The key point now is the in-memory representation should also be binary format to decrease spilling probability, avoiding potential performance regression.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "cshuo (via GitHub)" <gi...@apache.org>.
cshuo commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2139076809
##########
hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java:
##########
@@ -183,6 +187,14 @@ public Option<Predicate> getKeyFilterOpt() {
return keyFilterOpt;
}
+ public SizeEstimator<BufferedRecord<T>> getRecordSizeEstimator() {
+ return new HoodieRecordSizeEstimator<>(schemaHandler.getRequiredSchema());
+ }
+
+ public CustomSerializer<BufferedRecord<T>> getRecordSerializer() {
+ return new DefaultSerializer<>();
Review Comment:
I've run a local micro benchmark comparing `DefaultSerializer` and `BufferedRecordSerializer(DefaultRecordSerializer)`, with 1 million flink `GenericRowData` records.
```
public class DefaultRecordSerializer<T> implements RecordSerializer<T>{
@Override
public byte[] serialize(T record) {
try {
return SerializationUtils.serialize(record);
} catch (IOException e) {
throw new RuntimeException(e);
}
}
@Override
public T deserialize(byte[] bytes, int schemaId) {
return SerializationUtils.deserialize(bytes);
}
}
```
records:
```
GenericRowData record = new GenericRowData(5);
record.setField(0, "lily");
record.setField(1, 23);
record.setField(2, "shanghai");
record.setField(3, 1000L);
record.setField(4, "20240101");
BufferedRecord<GenericRowData> bufferedRecord = new BufferedRecord<>("lily", 1000L, record, 1, false);
```
Results:
~~Legacy default: 1439s~~
Legacy default: 1082s
Legacy default: 982s
Legacy default: 969s
Legacy default: 958s
Avg: 997
~~New: 1164s~~
New: 1164s
New: 1144s
New: 1155s
New: 1165s
Avg: 1157
Seems the legacy `DefaultSerializer` performs little better, so we maybe keep `DefaultSerializer` as default until we implement efficient `RecordSerializer` for other engine-specific rows.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #13408:
URL: https://github.com/apache/hudi/pull/13408#issuecomment-2961800475
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6081",
"triggerID" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"triggerType" : "PUSH"
}, {
"hash" : "939cadd2886028c09f13695862d88337333aaea2",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6088",
"triggerID" : "939cadd2886028c09f13695862d88337333aaea2",
"triggerType" : "PUSH"
}, {
"hash" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6107",
"triggerID" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"triggerType" : "PUSH"
}, {
"hash" : "56325d993f668f220a9b54f8d6a3db2be333c4c6",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6115",
"triggerID" : "56325d993f668f220a9b54f8d6a3db2be333c4c6",
"triggerType" : "PUSH"
}, {
"hash" : "30e9575e4e8a0d86559d2235178ccd213b19cfe5",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6127",
"triggerID" : "30e9575e4e8a0d86559d2235178ccd213b19cfe5",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 56325d993f668f220a9b54f8d6a3db2be333c4c6 Azure: [FAILURE](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6115)
* 30e9575e4e8a0d86559d2235178ccd213b19cfe5 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6127)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #13408:
URL: https://github.com/apache/hudi/pull/13408#issuecomment-2961796081
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6081",
"triggerID" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"triggerType" : "PUSH"
}, {
"hash" : "939cadd2886028c09f13695862d88337333aaea2",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6088",
"triggerID" : "939cadd2886028c09f13695862d88337333aaea2",
"triggerType" : "PUSH"
}, {
"hash" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6107",
"triggerID" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"triggerType" : "PUSH"
}, {
"hash" : "56325d993f668f220a9b54f8d6a3db2be333c4c6",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6115",
"triggerID" : "56325d993f668f220a9b54f8d6a3db2be333c4c6",
"triggerType" : "PUSH"
}, {
"hash" : "30e9575e4e8a0d86559d2235178ccd213b19cfe5",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "30e9575e4e8a0d86559d2235178ccd213b19cfe5",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 56325d993f668f220a9b54f8d6a3db2be333c4c6 Azure: [FAILURE](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6115)
* 30e9575e4e8a0d86559d2235178ccd213b19cfe5 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on PR #13408:
URL: https://github.com/apache/hudi/pull/13408#issuecomment-2964682463
Azure CI passed: https://dev.azure.com/apachehudi/hudi-oss-ci/_build/results?buildId=6127&view=results
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "cshuo (via GitHub)" <gi...@apache.org>.
cshuo commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2139026381
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/BufferedRecordSerializer.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.common.table.read;
+
+import org.apache.hudi.common.serialization.CustomSerializer;
+import org.apache.hudi.common.serialization.RecordSerializer;
+import org.apache.hudi.common.util.SerializationUtils;
+
+import com.esotericsoftware.kryo.Kryo;
+import com.esotericsoftware.kryo.io.Input;
+import com.esotericsoftware.kryo.io.Output;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+
+/**
+ * An implementation of {@link CustomSerializer} for {@link BufferedRecord}.
+ *
+ */
+public class BufferedRecordSerializer<T> implements CustomSerializer<BufferedRecord<T>> {
+ public static final int KRYO_SERIALIZER_INITIAL_BUFFER_SIZE = 1048576;
+ private final Kryo kryo;
+ // Caching ByteArrayOutputStream to avoid recreating it for every operation
+ private final ByteArrayOutputStream baos;
+ private final RecordSerializer<T> recordSerializer;
+
+ public BufferedRecordSerializer(RecordSerializer<T> recordSerializer) {
+ SerializationUtils.KryoInstantiator kryoInstantiator = new SerializationUtils.KryoInstantiator();
+ this.kryo = kryoInstantiator.newKryo();
+ this.baos = new ByteArrayOutputStream(KRYO_SERIALIZER_INITIAL_BUFFER_SIZE);
+ this.kryo.setRegistrationRequired(false);
+ this.recordSerializer = recordSerializer;
+ }
+
+ @Override
+ public byte[] serialize(BufferedRecord<T> record) throws IOException {
+ kryo.reset();
+ baos.reset();
+ try (Output output = new Output(baos)) {
+ this.kryo.writeClassAndObject(output, record.getRecordKey());
+ this.kryo.writeClassAndObject(output, record.getOrderingValue());
+ this.kryo.writeClassAndObject(output, record.getSchemaId());
Review Comment:
solved
##########
hudi-common/src/test/java/org/apache/hudi/avro/TestSizeEstimator.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.avro;
+
+import org.apache.hudi.avro.model.HoodieMetadataRecord;
+import org.apache.hudi.common.table.read.BufferedRecord;
+import org.apache.hudi.common.util.SizeEstimator;
+
+import org.apache.avro.Schema;
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.avro.generic.IndexedRecord;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import java.util.HashMap;
+
+public class TestSizeEstimator {
Review Comment:
solved
##########
hudi-common/src/test/java/org/apache/hudi/avro/TestSizeEstimator.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.avro;
+
+import org.apache.hudi.avro.model.HoodieMetadataRecord;
+import org.apache.hudi.common.table.read.BufferedRecord;
+import org.apache.hudi.common.util.SizeEstimator;
+
+import org.apache.avro.Schema;
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.avro.generic.IndexedRecord;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import java.util.HashMap;
+
+public class TestSizeEstimator {
+ private Schema schema;
+
+ String jsonSchema = "{\n"
Review Comment:
solved
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Use customized IndexedRecord to decrease spilling probabi… [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #13408:
URL: https://github.com/apache/hudi/pull/13408#issuecomment-2954560541
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6081",
"triggerID" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* c3747e2e221dac7cb7ecc78f15619bbbad1a09a6 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6081)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #13408:
URL: https://github.com/apache/hudi/pull/13408#issuecomment-2960596225
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6081",
"triggerID" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"triggerType" : "PUSH"
}, {
"hash" : "939cadd2886028c09f13695862d88337333aaea2",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6088",
"triggerID" : "939cadd2886028c09f13695862d88337333aaea2",
"triggerType" : "PUSH"
}, {
"hash" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"status" : "CANCELED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6107",
"triggerID" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 90fffa4f58d1f646409af51f89e104e87cd78085 Azure: [CANCELED](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6107)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2138953591
##########
hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java:
##########
@@ -183,6 +187,14 @@ public Option<Predicate> getKeyFilterOpt() {
return keyFilterOpt;
}
+ public SizeEstimator<BufferedRecord<T>> getRecordSizeEstimator() {
+ return new HoodieRecordSizeEstimator<>(schemaHandler.getRequiredSchema());
+ }
+
+ public CustomSerializer<BufferedRecord<T>> getRecordSerializer() {
+ return new DefaultSerializer<>();
Review Comment:
No, we need to handle the avro record specially here.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "the-other-tim-brown (via GitHub)" <gi...@apache.org>.
the-other-tim-brown commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2139048152
##########
hudi-common/src/main/java/org/apache/hudi/common/serialization/RecordSerializer.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.common.serialization;
+
+import java.io.Serializable;
+
+/**
+ * Serializer for engine-specific record, e.g., {@code InternalRow} in Spark,
+ * {@code RowData} in Flink and {@code IndexedRecord} for Avro.
+ */
+public interface RecordSerializer<T> extends Serializable {
Review Comment:
This is very similar to the interface we already have, `CustomSerializer`. I'm guessing we can't consolidate due to the deserializer needing a schema? The records in the spillable map should all have the same schema since we read the log records with the projected schema from the log files so maybe we don't need that `schemaId`?
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Use customized IndexedRecord to decrease spilling probabi… [hudi]
Posted by "cshuo (via GitHub)" <gi...@apache.org>.
cshuo commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2136743186
##########
hudi-common/src/main/java/org/apache/hudi/avro/BinaryIndexedRecord.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.avro;
+
+import org.apache.hudi.exception.HoodieException;
+
+import org.apache.avro.Schema;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.avro.generic.IndexedRecord;
+
+import java.io.IOException;
+
+/**
+ * An implementation of {@link IndexedRecord} which contains serialized bytes of an {@link IndexedRecord}
+ * only, and it's used for buffering in {@code ExternalSpillableMap}, and should be deserialized to object
+ * based {@link IndexedRecord} for merging or iterating.
+ */
+public class BinaryIndexedRecord implements IndexedRecord {
Review Comment:
Fix at serializer level has been considered, but it cannot solve the problem that Avro record buffered in memoryMap of `ExternalSpillableMap` occupied much more memory than legacy log scanner (5x for MDT payload). So just as other engines, Avro record should be transformed into binary format before put into record buffer.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #13408:
URL: https://github.com/apache/hudi/pull/13408#issuecomment-2962114346
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6081",
"triggerID" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"triggerType" : "PUSH"
}, {
"hash" : "939cadd2886028c09f13695862d88337333aaea2",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6088",
"triggerID" : "939cadd2886028c09f13695862d88337333aaea2",
"triggerType" : "PUSH"
}, {
"hash" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6107",
"triggerID" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"triggerType" : "PUSH"
}, {
"hash" : "56325d993f668f220a9b54f8d6a3db2be333c4c6",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6115",
"triggerID" : "56325d993f668f220a9b54f8d6a3db2be333c4c6",
"triggerType" : "PUSH"
}, {
"hash" : "30e9575e4e8a0d86559d2235178ccd213b19cfe5",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6127",
"triggerID" : "30e9575e4e8a0d86559d2235178ccd213b19cfe5",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 30e9575e4e8a0d86559d2235178ccd213b19cfe5 Azure: [FAILURE](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6127)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Use customized IndexedRecord to decrease spilling probabi… [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #13408:
URL: https://github.com/apache/hudi/pull/13408#issuecomment-2955773707
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6081",
"triggerID" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"triggerType" : "PUSH"
}, {
"hash" : "939cadd2886028c09f13695862d88337333aaea2",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "939cadd2886028c09f13695862d88337333aaea2",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* c3747e2e221dac7cb7ecc78f15619bbbad1a09a6 Azure: [FAILURE](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6081)
* 939cadd2886028c09f13695862d88337333aaea2 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Use customized IndexedRecord to decrease spilling probabi… [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #13408:
URL: https://github.com/apache/hudi/pull/13408#issuecomment-2954632821
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6081",
"triggerID" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* c3747e2e221dac7cb7ecc78f15619bbbad1a09a6 Azure: [FAILURE](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6081)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #13408:
URL: https://github.com/apache/hudi/pull/13408#issuecomment-2960996663
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6081",
"triggerID" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"triggerType" : "PUSH"
}, {
"hash" : "939cadd2886028c09f13695862d88337333aaea2",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6088",
"triggerID" : "939cadd2886028c09f13695862d88337333aaea2",
"triggerType" : "PUSH"
}, {
"hash" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"status" : "CANCELED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6107",
"triggerID" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"triggerType" : "PUSH"
}, {
"hash" : "56325d993f668f220a9b54f8d6a3db2be333c4c6",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6115",
"triggerID" : "56325d993f668f220a9b54f8d6a3db2be333c4c6",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 90fffa4f58d1f646409af51f89e104e87cd78085 Azure: [CANCELED](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6107)
* 56325d993f668f220a9b54f8d6a3db2be333c4c6 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6115)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "cshuo (via GitHub)" <gi...@apache.org>.
cshuo commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2139076809
##########
hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java:
##########
@@ -183,6 +187,14 @@ public Option<Predicate> getKeyFilterOpt() {
return keyFilterOpt;
}
+ public SizeEstimator<BufferedRecord<T>> getRecordSizeEstimator() {
+ return new HoodieRecordSizeEstimator<>(schemaHandler.getRequiredSchema());
+ }
+
+ public CustomSerializer<BufferedRecord<T>> getRecordSerializer() {
+ return new DefaultSerializer<>();
Review Comment:
I've run a local micro benchmark comparing `DefaultSerializer` and `BufferedRecordSerializer(DefaultRecordSerializer)`, with 1 million flink `GenericRowData` records.
```
public class DefaultRecordSerializer<T> implements RecordSerializer<T>{
@Override
public byte[] serialize(T record) {
try {
return SerializationUtils.serialize(record);
} catch (IOException e) {
throw new RuntimeException(e);
}
}
@Override
public T deserialize(byte[] bytes, int schemaId) {
return SerializationUtils.deserialize(bytes);
}
}
```
records:
```
GenericRowData record = new GenericRowData(5);
record.setField(0, "lily");
record.setField(1, 23);
record.setField(2, "shanghai");
record.setField(3, 1000L);
record.setField(2, "20240101");
BufferedRecord<GenericRowData> bufferedRecord = new BufferedRecord<>("lily", 1000L, record, 1, false);
```
Results:
~~Legacy default: 1439s~~
Legacy default: 1082s
Legacy default: 982s
Legacy default: 969s
Legacy default: 958s
Avg: 997
~~New: 1164s~~
New: 1164s
New: 1144s
New: 1155s
New: 1165s
Avg: 1157
Seems the legacy `DefaultSerializer` performs little better, so we maybe keep `DefaultSerializer` as default until we implement efficient `RecordSerializer` for other engine-specific rows.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "the-other-tim-brown (via GitHub)" <gi...@apache.org>.
the-other-tim-brown commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2137967677
##########
hudi-common/src/test/java/org/apache/hudi/avro/TestSizeEstimator.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.avro;
+
+import org.apache.hudi.avro.model.HoodieMetadataRecord;
+import org.apache.hudi.common.table.read.BufferedRecord;
+import org.apache.hudi.common.util.SizeEstimator;
+
+import org.apache.avro.Schema;
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.avro.generic.IndexedRecord;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import java.util.HashMap;
+
+public class TestSizeEstimator {
+ private Schema schema;
+
+ String jsonSchema = "{\n"
Review Comment:
I see this is used in the other test class as well. Should we move it to some common test util?
##########
hudi-common/src/test/java/org/apache/hudi/avro/TestSizeEstimator.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.avro;
+
+import org.apache.hudi.avro.model.HoodieMetadataRecord;
+import org.apache.hudi.common.table.read.BufferedRecord;
+import org.apache.hudi.common.util.SizeEstimator;
+
+import org.apache.avro.Schema;
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.avro.generic.IndexedRecord;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import java.util.HashMap;
+
+public class TestSizeEstimator {
Review Comment:
nitpick: should this be renamed to `TestAvroRecordSizeEstimator`?
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/BufferedRecordSerializer.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.common.table.read;
+
+import org.apache.hudi.common.serialization.CustomSerializer;
+import org.apache.hudi.common.serialization.RecordSerializer;
+import org.apache.hudi.common.util.SerializationUtils;
+
+import com.esotericsoftware.kryo.Kryo;
+import com.esotericsoftware.kryo.io.Input;
+import com.esotericsoftware.kryo.io.Output;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+
+/**
+ * An implementation of {@link CustomSerializer} for {@link BufferedRecord}.
+ *
+ */
+public class BufferedRecordSerializer<T> implements CustomSerializer<BufferedRecord<T>> {
+ public static final int KRYO_SERIALIZER_INITIAL_BUFFER_SIZE = 1048576;
+ private final Kryo kryo;
+ // Caching ByteArrayOutputStream to avoid recreating it for every operation
+ private final ByteArrayOutputStream baos;
+ private final RecordSerializer<T> recordSerializer;
+
+ public BufferedRecordSerializer(RecordSerializer<T> recordSerializer) {
+ SerializationUtils.KryoInstantiator kryoInstantiator = new SerializationUtils.KryoInstantiator();
+ this.kryo = kryoInstantiator.newKryo();
+ this.baos = new ByteArrayOutputStream(KRYO_SERIALIZER_INITIAL_BUFFER_SIZE);
+ this.kryo.setRegistrationRequired(false);
+ this.recordSerializer = recordSerializer;
+ }
+
+ @Override
+ public byte[] serialize(BufferedRecord<T> record) throws IOException {
+ kryo.reset();
+ baos.reset();
+ try (Output output = new Output(baos)) {
+ this.kryo.writeClassAndObject(output, record.getRecordKey());
+ this.kryo.writeClassAndObject(output, record.getOrderingValue());
+ this.kryo.writeClassAndObject(output, record.getSchemaId());
Review Comment:
Could this be `writeInt` and the line below `writeBoolean`?
##########
hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java:
##########
@@ -183,6 +187,14 @@ public Option<Predicate> getKeyFilterOpt() {
return keyFilterOpt;
}
+ public SizeEstimator<BufferedRecord<T>> getRecordSizeEstimator() {
+ return new HoodieRecordSizeEstimator<>(schemaHandler.getRequiredSchema());
+ }
+
+ public CustomSerializer<BufferedRecord<T>> getRecordSerializer() {
+ return new DefaultSerializer<>();
Review Comment:
As the default, do we want to use `BufferedRecordSerializer` with the `DefaultSerializer` as the record serializer within it?
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "cshuo (via GitHub)" <gi...@apache.org>.
cshuo commented on PR #13408:
URL: https://github.com/apache/hudi/pull/13408#issuecomment-2959099851
PR updated, PTAL if you have time. cc @danny0405 @the-other-tim-brown
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "cshuo (via GitHub)" <gi...@apache.org>.
cshuo commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2139053908
##########
hudi-common/src/main/java/org/apache/hudi/common/serialization/RecordSerializer.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.common.serialization;
+
+import java.io.Serializable;
+
+/**
+ * Serializer for engine-specific record, e.g., {@code InternalRow} in Spark,
+ * {@code RowData} in Flink and {@code IndexedRecord} for Avro.
+ */
+public interface RecordSerializer<T> extends Serializable {
Review Comment:
> The records in the spillable map should all have the same schema
That's what I originally thought, but unfortunately not for MDT scenario. Currently, merging result of two MDT log records is always `HoodieMetadataRecord`, which has a definite schema `HoodieMetadataRecord#SCHEMA$`, which can be different with the required schema of FG reader. So we decode schema using `schemaId` in `BufferedRecord` to make sure the schema is matching the Avro record itself.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #13408:
URL: https://github.com/apache/hudi/pull/13408#issuecomment-2961125442
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6081",
"triggerID" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"triggerType" : "PUSH"
}, {
"hash" : "939cadd2886028c09f13695862d88337333aaea2",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6088",
"triggerID" : "939cadd2886028c09f13695862d88337333aaea2",
"triggerType" : "PUSH"
}, {
"hash" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6107",
"triggerID" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"triggerType" : "PUSH"
}, {
"hash" : "56325d993f668f220a9b54f8d6a3db2be333c4c6",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6115",
"triggerID" : "56325d993f668f220a9b54f8d6a3db2be333c4c6",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 56325d993f668f220a9b54f8d6a3db2be333c4c6 Azure: [FAILURE](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6115)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "the-other-tim-brown (via GitHub)" <gi...@apache.org>.
the-other-tim-brown commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2139136439
##########
hudi-common/src/main/java/org/apache/hudi/common/serialization/RecordSerializer.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hudi.common.serialization;
+
+import java.io.Serializable;
+
+/**
+ * Serializer for engine-specific record, e.g., {@code InternalRow} in Spark,
+ * {@code RowData} in Flink and {@code IndexedRecord} for Avro.
+ */
+public interface RecordSerializer<T> extends Serializable {
Review Comment:
I ran into this with the metadata compaction implementation as well. There can be runtime issues if we have mixed schemas with our records though so we need some way to standardize how to handle 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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 merged PR #13408:
URL: https://github.com/apache/hudi/pull/13408
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Use customized IndexedRecord to decrease spilling probabi… [hudi]
Posted by "cshuo (via GitHub)" <gi...@apache.org>.
cshuo commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2136752800
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java:
##########
@@ -376,14 +377,27 @@ public Option<IndexedRecord> combineAndGetUpdateValue(IndexedRecord oldRecord, S
}
@Override
- public Option<IndexedRecord> getInsertValue(Schema schemaIgnored, Properties propertiesIgnored) throws IOException {
+ public Option<IndexedRecord> getInsertValue(Schema schema, Properties propertiesIgnored) throws IOException {
if (key == null || this.isDeletedRecord) {
return Option.empty();
}
- HoodieMetadataRecord record = new HoodieMetadataRecord(key, type, filesystemMetadata, bloomFilterMetadata,
- columnStatMetadata, recordIndexMetadata, secondaryIndexMetadata);
- return Option.of(record);
+ if (schema != null && schema.getField(HoodieRecord.COMMIT_TIME_METADATA_FIELD) != null) {
+ GenericRecord result = new GenericData.Record(schema);
Review Comment:
* `HoodieMetadataPayload#getInsertValue` will be called as the merging result during FG reader merging metadata record.
* Currently `HoodieMetadataPayload#getInsertValue` always return fixed schema containing 7 data fields, which may break the logics in File Group reader, e.g., File Group reader has a required schema more than 7 data fields.
* The fix in this PR is using required schema to convert avro bytes back into GenericRecord.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Use customized IndexedRecord to decrease spilling probabi… [hudi]
Posted by "cshuo (via GitHub)" <gi...@apache.org>.
cshuo commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2136867791
##########
hudi-common/src/main/java/org/apache/hudi/avro/BinaryIndexedRecord.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.avro;
+
+import org.apache.hudi.exception.HoodieException;
+
+import org.apache.avro.Schema;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.avro.generic.IndexedRecord;
+
+import java.io.IOException;
+
+/**
+ * An implementation of {@link IndexedRecord} which contains serialized bytes of an {@link IndexedRecord}
+ * only, and it's used for buffering in {@code ExternalSpillableMap}, and should be deserialized to object
+ * based {@link IndexedRecord} for merging or iterating.
+ */
+public class BinaryIndexedRecord implements IndexedRecord {
Review Comment:
Yeah, extra costs will incurred if there is no spilling, but regression caused by spilling is more severe, that's a tradeoff.
Anyway, I've just checked again, for MDT reading, the legacy scanner uses `HoodieMetadataPayload` for buffering, which is not of binary format. So it seems binary format is not necessary considering the perf regression, and customized serializer/size estimator is enough. I'll give it a try toward 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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Use customized IndexedRecord to decrease spilling probabi… [hudi]
Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2136729018
##########
hudi-common/src/main/java/org/apache/hudi/common/table/read/HoodieFileGroupReader.java:
##########
@@ -334,6 +334,8 @@ public HoodieReadStats getStats() {
*/
T next() {
T nextVal = recordBuffer == null ? baseFileIterator.next() : recordBuffer.next();
+ Schema requiredSchema = readerContext.getSchemaHandler().getRequiredSchema();
+ nextVal = readerContext.fromBinaryRow(requiredSchema, nextVal);
Review Comment:
why we pass around the required schema here as long as the reader context already holds it.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #13408:
URL: https://github.com/apache/hudi/pull/13408#issuecomment-2960289033
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6081",
"triggerID" : "c3747e2e221dac7cb7ecc78f15619bbbad1a09a6",
"triggerType" : "PUSH"
}, {
"hash" : "939cadd2886028c09f13695862d88337333aaea2",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6088",
"triggerID" : "939cadd2886028c09f13695862d88337333aaea2",
"triggerType" : "PUSH"
}, {
"hash" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6107",
"triggerID" : "90fffa4f58d1f646409af51f89e104e87cd78085",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 939cadd2886028c09f13695862d88337333aaea2 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6088)
* 90fffa4f58d1f646409af51f89e104e87cd78085 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=6107)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [HUDI-9499] Add customized SizeEstimator and Serializer for Avro buffered record in FileGroup reader [hudi]
Posted by "the-other-tim-brown (via GitHub)" <gi...@apache.org>.
the-other-tim-brown commented on code in PR #13408:
URL: https://github.com/apache/hudi/pull/13408#discussion_r2138994279
##########
hudi-common/src/main/java/org/apache/hudi/common/engine/HoodieReaderContext.java:
##########
@@ -183,6 +187,14 @@ public Option<Predicate> getKeyFilterOpt() {
return keyFilterOpt;
}
+ public SizeEstimator<BufferedRecord<T>> getRecordSizeEstimator() {
+ return new HoodieRecordSizeEstimator<>(schemaHandler.getRequiredSchema());
+ }
+
+ public CustomSerializer<BufferedRecord<T>> getRecordSerializer() {
+ return new DefaultSerializer<>();
Review Comment:
This is the default though, the avro reader context will override this method. I'm curious if the specialized handler of the BufferedRecord will help with general performance.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org