You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "nsivabalan (via GitHub)" <gi...@apache.org> on 2023/01/21 23:00:07 UTC
[GitHub] [hudi] nsivabalan opened a new pull request, #7726: [HUDI-5575] Adding auto generation of record keys w/ hudi. Added support to all key gen clases
nsivabalan opened a new pull request, #7726:
URL: https://github.com/apache/hudi/pull/7726
### Change Logs
As of now, record key generation and partition path generation are tightly coupled based on the key gen class used. But these could be de-coupled. Since recently we added [auto generation of record keys](https://github.com/apache/hudi/pull/7640), its not flexible to be used w/ any key gen class.
So, this patch fixes that so that users can enable auto generation of record keys along w/ any key gen class. Users have to enable a config called `hoodie.auto.generate.record.keys` to let hudi auto generate record keys for them. In such cases, users don't need to set any value for `hoodie.datasource.write.recordkey.field` which otherwise is a mandatory field. When this new config is enabled, the key gen class config (`hoodie.datasource.write.keygenerator.class`) will determine the partition path generation logic.
Added a new class called AutoRecordKeyGenerator which will be leveraged in all key gen classes for record key related methods.
### Impact
Enables users enable auto generation of record keys along w/ any key gen class.
### Risk level (write none, low medium or high below)
low.
### Documentation Update
_Describe any necessary documentation update if there is any new feature, config, or user-facing change_
- _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
[GitHub] [hudi] codope commented on pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "codope (via GitHub)" <gi...@apache.org>.
codope commented on PR #7726:
URL: https://github.com/apache/hudi/pull/7726#issuecomment-1402049115
Merging it as all tests are fixed and only `testUpsertsContinuousModeWithMultipleWritersForConflicts` is flaky, which is expected to be fixed by #7720
--
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
[GitHub] [hudi] nsivabalan commented on a diff in pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "nsivabalan (via GitHub)" <gi...@apache.org>.
nsivabalan commented on code in PR #7726:
URL: https://github.com/apache/hudi/pull/7726#discussion_r1084615222
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/keygen/TestAutoRecordKeyGenerator.java:
##########
@@ -112,8 +112,9 @@ protected GenericRecord createRecord(String partitionField, String stringValue,
protected TypedProperties getKeyGenProperties(String partitionPathField, int numFieldsInKeyGen) {
TypedProperties properties = new TypedProperties();
properties.put(KeyGeneratorOptions.PARTITIONPATH_FIELD_NAME.key(), partitionPathField);
- properties.put(KeyGeneratorOptions.NUM_FIELDS_IN_KEYLESS_GENERATOR.key(), numFieldsInKeyGen);
+ properties.put(KeyGeneratorOptions.NUM_FIELDS_IN_AUTO_RECORDKEY_GENERATION.key(), numFieldsInKeyGen);
properties.put(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key(), "");
Review Comment:
yes, we do need it. we will let users override if they wish to. but do have default value set.
--
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
[GitHub] [hudi] hudi-bot commented on pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #7726:
URL: https://github.com/apache/hudi/pull/7726#issuecomment-1399351386
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "25fbf375105d43c8a8707599c0eba58e01dea6f2",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "25fbf375105d43c8a8707599c0eba58e01dea6f2",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 25fbf375105d43c8a8707599c0eba58e01dea6f2 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
[GitHub] [hudi] nsivabalan commented on a diff in pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "nsivabalan (via GitHub)" <gi...@apache.org>.
nsivabalan commented on code in PR #7726:
URL: https://github.com/apache/hudi/pull/7726#discussion_r1084305752
##########
hudi-common/src/main/java/org/apache/hudi/keygen/BaseKeyGenerator.java:
##########
@@ -81,4 +88,10 @@ public List<String> getPartitionPathFields() {
public boolean isConsistentLogicalTimestampEnabled() {
return consistentLogicalTimestampEnabled;
}
+
+ protected void instantiateAutoRecordKeyGenerator() {
+ if (autoGenerateRecordKeys) {
+ autoRecordKeyGenerator = new AutoRecordKeyGenerator(config, getPartitionPathFields());
Review Comment:
issue is, partitionPath fields are set by each concrete class. so within BaseKeyGenerator constructor, partitionPaths are not set yet. even I tried and realised it later.
--
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
[GitHub] [hudi] nsivabalan commented on a diff in pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "nsivabalan (via GitHub)" <gi...@apache.org>.
nsivabalan commented on code in PR #7726:
URL: https://github.com/apache/hudi/pull/7726#discussion_r1084627543
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/keygen/ComplexKeyGenerator.java:
##########
@@ -64,14 +66,22 @@ public String getPartitionPath(GenericRecord record) {
@Override
public String getRecordKey(Row row) {
- tryInitRowAccessor(row.schema());
- return combineCompositeRecordKey(rowAccessor.getRecordKeyParts(row));
+ if (autoGenerateRecordKeys) {
+ return super.getRecordKey(row);
+ } else {
+ tryInitRowAccessor(row.schema());
Review Comment:
https://issues.apache.org/jira/browse/HUDI-5575
--
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
[GitHub] [hudi] hudi-bot commented on pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #7726:
URL: https://github.com/apache/hudi/pull/7726#issuecomment-1401823737
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "25fbf375105d43c8a8707599c0eba58e01dea6f2",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14539",
"triggerID" : "25fbf375105d43c8a8707599c0eba58e01dea6f2",
"triggerType" : "PUSH"
}, {
"hash" : "efe71f4ece3aeb318285d00b5de153aa182a35d4",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14569",
"triggerID" : "efe71f4ece3aeb318285d00b5de153aa182a35d4",
"triggerType" : "PUSH"
}, {
"hash" : "7de21a1d14ad52d84841a31bc850341fd66033e5",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "7de21a1d14ad52d84841a31bc850341fd66033e5",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* efe71f4ece3aeb318285d00b5de153aa182a35d4 Azure: [FAILURE](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14569)
* 7de21a1d14ad52d84841a31bc850341fd66033e5 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
[GitHub] [hudi] codope commented on a diff in pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "codope (via GitHub)" <gi...@apache.org>.
codope commented on code in PR #7726:
URL: https://github.com/apache/hudi/pull/7726#discussion_r1083729980
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/keygen/TestAutoRecordKeyGenerator.java:
##########
@@ -112,8 +112,9 @@ protected GenericRecord createRecord(String partitionField, String stringValue,
protected TypedProperties getKeyGenProperties(String partitionPathField, int numFieldsInKeyGen) {
TypedProperties properties = new TypedProperties();
properties.put(KeyGeneratorOptions.PARTITIONPATH_FIELD_NAME.key(), partitionPathField);
- properties.put(KeyGeneratorOptions.NUM_FIELDS_IN_KEYLESS_GENERATOR.key(), numFieldsInKeyGen);
+ properties.put(KeyGeneratorOptions.NUM_FIELDS_IN_AUTO_RECORDKEY_GENERATION.key(), numFieldsInKeyGen);
properties.put(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key(), "");
Review Comment:
Do we still need to set this prop?
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/TestAutoRecordKeyGeneration.scala:
##########
@@ -0,0 +1,119 @@
+/*
+ * 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
+
+
+import org.apache.hudi.common.fs.FSUtils
+import org.apache.hudi.common.testutils.HoodieTestDataGenerator
+import org.apache.hudi.common.testutils.RawTripTestPayload.recordsToStrings
+import org.apache.hudi.config.HoodieWriteConfig
+import org.apache.hudi.keygen.TimestampBasedKeyGenerator
+import org.apache.hudi.keygen.constant.KeyGeneratorOptions
+import org.apache.hudi.testutils.SparkClientFunctionalTestHarness
+import org.apache.spark.sql._
+import org.junit.jupiter.api.Assertions.{assertEquals, assertTrue}
+import org.junit.jupiter.params.ParameterizedTest
+import org.junit.jupiter.params.provider.CsvSource
+
+import scala.collection.JavaConversions._
+
+/**
+ * Tests auto generation of record keys.
+ */
+class TestAutoRecordKeyGeneration extends SparkClientFunctionalTestHarness {
Review Comment:
+1 for end to end test.
Can we write a similar test for spark-sql path as well?
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/keygen/TestAutoRecordKeyGenerator.java:
##########
@@ -29,45 +29,45 @@
import java.io.IOException;
-public class TestKeylessKeyGenerator {
+public class TestAutoRecordKeyGenerator {
Review Comment:
Should we also assert different partition paths in this test?
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/keygen/ComplexKeyGenerator.java:
##########
@@ -64,14 +66,22 @@ public String getPartitionPath(GenericRecord record) {
@Override
public String getRecordKey(Row row) {
- tryInitRowAccessor(row.schema());
- return combineCompositeRecordKey(rowAccessor.getRecordKeyParts(row));
+ if (autoGenerateRecordKeys) {
+ return super.getRecordKey(row);
+ } else {
+ tryInitRowAccessor(row.schema());
Review Comment:
let's file a ticket for handling row-writer path.
##########
hudi-common/src/main/java/org/apache/hudi/keygen/BaseKeyGenerator.java:
##########
@@ -81,4 +88,10 @@ public List<String> getPartitionPathFields() {
public boolean isConsistentLogicalTimestampEnabled() {
return consistentLogicalTimestampEnabled;
}
+
+ protected void instantiateAutoRecordKeyGenerator() {
+ if (autoGenerateRecordKeys) {
+ autoRecordKeyGenerator = new AutoRecordKeyGenerator(config, getPartitionPathFields());
Review Comment:
Why not move it to the constructor itself? Is there some keygen for which we don't want to instantiate `autoRecordKeyGenerator`? Moving it to the constructor ensures that no one can miss it in future keygens.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/keygen/ComplexAvroKeyGenerator.java:
##########
@@ -32,19 +33,25 @@ public class ComplexAvroKeyGenerator extends BaseKeyGenerator {
public ComplexAvroKeyGenerator(TypedProperties props) {
super(props);
- this.recordKeyFields = Arrays.stream(props.getString(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key()).split(","))
+ this.recordKeyFields = props.containsKey(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key())
+ ? Arrays.stream(props.getString(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key()).split(","))
.map(String::trim)
.filter(s -> !s.isEmpty())
- .collect(Collectors.toList());
+ .collect(Collectors.toList()) : Collections.EMPTY_LIST;
this.partitionPathFields = Arrays.stream(props.getString(KeyGeneratorOptions.PARTITIONPATH_FIELD_NAME.key()).split(","))
.map(String::trim)
.filter(s -> !s.isEmpty())
.collect(Collectors.toList());
+ instantiateAutoRecordKeyGenerator();
}
@Override
public String getRecordKey(GenericRecord record) {
- return KeyGenUtils.getRecordKey(record, getRecordKeyFieldNames(), isConsistentLogicalTimestampEnabled());
+ if (autoGenerateRecordKeys) {
Review Comment:
I think we should move this conditional inside `KeyGenUtils.getRecordKey` otherwise it's a burden on the developer to handle in each keygen class. Wdyt?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/keygen/TimestampBasedAvroKeyGenerator.java:
##########
@@ -65,6 +65,7 @@ public enum TimestampType implements Serializable {
private final DateTimeZone outputDateTimeZone;
protected final boolean encodePartitionPath;
+ private AutoRecordKeyGenerator autoRecordKeyGenerator;
Review Comment:
don't really need this.
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/keygen/CustomKeyGenerator.java:
##########
@@ -85,9 +85,13 @@ public String getPartitionPath(GenericRecord record) {
@Override
public String getRecordKey(Row row) {
- return getRecordKeyFieldNames().size() == 1
- ? new SimpleKeyGenerator(config, config.getString(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key()), null).getRecordKey(row)
- : new ComplexKeyGenerator(config).getRecordKey(row);
+ if (autoGenerateRecordKeys) {
Review Comment:
Also, need to handle empty/null recordkey field config in the constructor on line 63.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/keygen/ComplexAvroKeyGenerator.java:
##########
@@ -32,19 +33,25 @@ public class ComplexAvroKeyGenerator extends BaseKeyGenerator {
public ComplexAvroKeyGenerator(TypedProperties props) {
super(props);
- this.recordKeyFields = Arrays.stream(props.getString(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key()).split(","))
+ this.recordKeyFields = props.containsKey(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key())
Review Comment:
Let's make sure that this is handled in every keygen's constructor.
--
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
[GitHub] [hudi] nsivabalan commented on a diff in pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "nsivabalan (via GitHub)" <gi...@apache.org>.
nsivabalan commented on code in PR #7726:
URL: https://github.com/apache/hudi/pull/7726#discussion_r1084634345
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/keygen/ComplexAvroKeyGenerator.java:
##########
@@ -32,19 +33,25 @@ public class ComplexAvroKeyGenerator extends BaseKeyGenerator {
public ComplexAvroKeyGenerator(TypedProperties props) {
super(props);
- this.recordKeyFields = Arrays.stream(props.getString(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key()).split(","))
+ this.recordKeyFields = props.containsKey(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key())
+ ? Arrays.stream(props.getString(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key()).split(","))
.map(String::trim)
.filter(s -> !s.isEmpty())
- .collect(Collectors.toList());
+ .collect(Collectors.toList()) : Collections.EMPTY_LIST;
this.partitionPathFields = Arrays.stream(props.getString(KeyGeneratorOptions.PARTITIONPATH_FIELD_NAME.key()).split(","))
.map(String::trim)
.filter(s -> !s.isEmpty())
.collect(Collectors.toList());
+ instantiateAutoRecordKeyGenerator();
}
@Override
public String getRecordKey(GenericRecord record) {
- return KeyGenUtils.getRecordKey(record, getRecordKeyFieldNames(), isConsistentLogicalTimestampEnabled());
+ if (autoGenerateRecordKeys) {
Review Comment:
anyways, we need to overhaul the record key generation as you saw in one of the other patches (https://github.com/apache/hudi/pull/7699). that should handle this seamlessly. let me know.
--
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
[GitHub] [hudi] hudi-bot commented on pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #7726:
URL: https://github.com/apache/hudi/pull/7726#issuecomment-1401287378
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "25fbf375105d43c8a8707599c0eba58e01dea6f2",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14539",
"triggerID" : "25fbf375105d43c8a8707599c0eba58e01dea6f2",
"triggerType" : "PUSH"
}, {
"hash" : "efe71f4ece3aeb318285d00b5de153aa182a35d4",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14569",
"triggerID" : "efe71f4ece3aeb318285d00b5de153aa182a35d4",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* efe71f4ece3aeb318285d00b5de153aa182a35d4 Azure: [FAILURE](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14569)
<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
[GitHub] [hudi] codope commented on a diff in pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "codope (via GitHub)" <gi...@apache.org>.
codope commented on code in PR #7726:
URL: https://github.com/apache/hudi/pull/7726#discussion_r1084833766
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/keygen/ComplexAvroKeyGenerator.java:
##########
@@ -32,19 +33,25 @@ public class ComplexAvroKeyGenerator extends BaseKeyGenerator {
public ComplexAvroKeyGenerator(TypedProperties props) {
super(props);
- this.recordKeyFields = Arrays.stream(props.getString(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key()).split(","))
+ this.recordKeyFields = props.containsKey(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key())
+ ? Arrays.stream(props.getString(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key()).split(","))
.map(String::trim)
.filter(s -> !s.isEmpty())
- .collect(Collectors.toList());
+ .collect(Collectors.toList()) : Collections.EMPTY_LIST;
this.partitionPathFields = Arrays.stream(props.getString(KeyGeneratorOptions.PARTITIONPATH_FIELD_NAME.key()).split(","))
.map(String::trim)
.filter(s -> !s.isEmpty())
.collect(Collectors.toList());
+ instantiateAutoRecordKeyGenerator();
}
@Override
public String getRecordKey(GenericRecord record) {
- return KeyGenUtils.getRecordKey(record, getRecordKeyFieldNames(), isConsistentLogicalTimestampEnabled());
+ if (autoGenerateRecordKeys) {
Review Comment:
sounds good.
--
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
[GitHub] [hudi] hudi-bot commented on pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #7726:
URL: https://github.com/apache/hudi/pull/7726#issuecomment-1401112308
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "25fbf375105d43c8a8707599c0eba58e01dea6f2",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14539",
"triggerID" : "25fbf375105d43c8a8707599c0eba58e01dea6f2",
"triggerType" : "PUSH"
}, {
"hash" : "efe71f4ece3aeb318285d00b5de153aa182a35d4",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14569",
"triggerID" : "efe71f4ece3aeb318285d00b5de153aa182a35d4",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 25fbf375105d43c8a8707599c0eba58e01dea6f2 Azure: [FAILURE](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14539)
* efe71f4ece3aeb318285d00b5de153aa182a35d4 Azure: [PENDING](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14569)
<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
[GitHub] [hudi] codope merged pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "codope (via GitHub)" <gi...@apache.org>.
codope merged PR #7726:
URL: https://github.com/apache/hudi/pull/7726
--
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
[GitHub] [hudi] hudi-bot commented on pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #7726:
URL: https://github.com/apache/hudi/pull/7726#issuecomment-1399352263
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "25fbf375105d43c8a8707599c0eba58e01dea6f2",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14539",
"triggerID" : "25fbf375105d43c8a8707599c0eba58e01dea6f2",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 25fbf375105d43c8a8707599c0eba58e01dea6f2 Azure: [PENDING](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14539)
<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
[GitHub] [hudi] hudi-bot commented on pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #7726:
URL: https://github.com/apache/hudi/pull/7726#issuecomment-1399384895
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "25fbf375105d43c8a8707599c0eba58e01dea6f2",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14539",
"triggerID" : "25fbf375105d43c8a8707599c0eba58e01dea6f2",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 25fbf375105d43c8a8707599c0eba58e01dea6f2 Azure: [FAILURE](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14539)
<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
[GitHub] [hudi] vinothchandar commented on pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "vinothchandar (via GitHub)" <gi...@apache.org>.
vinothchandar commented on PR #7726:
URL: https://github.com/apache/hudi/pull/7726#issuecomment-1403090108
@nsivabalan are @alexeykudinkin 's review feedback addressed?
--
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
[GitHub] [hudi] alexeykudinkin commented on a diff in pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "alexeykudinkin (via GitHub)" <gi...@apache.org>.
alexeykudinkin commented on code in PR #7726:
URL: https://github.com/apache/hudi/pull/7726#discussion_r1085760200
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/keygen/NonpartitionedAvroKeyGenerator.java:
##########
@@ -39,6 +39,7 @@ public NonpartitionedAvroKeyGenerator(TypedProperties props) {
this.recordKeyFields = Arrays.stream(props.getString(KeyGeneratorOptions.RECORDKEY_FIELD_NAME.key())
.split(",")).map(String::trim).filter(s -> !s.isEmpty()).collect(Collectors.toList());
this.partitionPathFields = EMPTY_PARTITION_FIELD_LIST;
+ instantiateAutoRecordKeyGenerator();
Review Comment:
Please assign var inside the ctor, make it final
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/keygen/ComplexKeyGenerator.java:
##########
@@ -64,14 +67,22 @@ public String getPartitionPath(GenericRecord record) {
@Override
public String getRecordKey(Row row) {
- tryInitRowAccessor(row.schema());
- return combineCompositeRecordKey(rowAccessor.getRecordKeyParts(row));
+ if (autoGenerateRecordKeys) {
+ return super.getRecordKey(row);
+ } else {
+ tryInitRowAccessor(row.schema());
+ return combineCompositeRecordKey(rowAccessor.getRecordKeyParts(row));
+ }
}
@Override
public UTF8String getRecordKey(InternalRow internalRow, StructType schema) {
- tryInitRowAccessor(schema);
- return combineCompositeRecordKeyUnsafe(rowAccessor.getRecordKeyParts(internalRow));
+ if (autoGenerateRecordKeys) {
+ return super.getRecordKey(internalRow, schema);
Review Comment:
Why are we redirecting to BuiltinKeyGenerator here?
On top of that i don't see that we've changed it either, so how is this supposed to work?
##########
hudi-common/src/main/java/org/apache/hudi/keygen/BaseKeyGenerator.java:
##########
@@ -32,11 +32,16 @@
*/
public abstract class BaseKeyGenerator extends KeyGenerator {
+ protected static final String DEFAULT_PARTITION_PATH_SEPARATOR = "/";
+ public static final String SPLIT_REGEX = ":";
+
protected List<String> recordKeyFields;
protected List<String> partitionPathFields;
protected final boolean encodePartitionPath;
protected final boolean hiveStylePartitioning;
protected final boolean consistentLogicalTimestampEnabled;
+ protected final boolean autoGenerateRecordKeys;
+ protected AutoRecordKeyGenerator autoRecordKeyGenerator;
Review Comment:
Please wrap it into Option
##########
hudi-common/src/main/java/org/apache/hudi/keygen/constant/KeyGeneratorOptions.java:
##########
@@ -69,11 +69,21 @@ public class KeyGeneratorOptions extends HoodieConfig {
+ "`2016-12-29 09:54:00.0` in row-writer path, while it will be written as long value `1483023240000000` in non row-writer path. "
+ "If enabled, then the timestamp value will be written in both the cases.");
- public static final ConfigProperty<Integer> NUM_FIELDS_IN_KEYLESS_GENERATOR = ConfigProperty
- .key("hoodie.datasource.write.recordkey.keyless.field.count")
+ public static final ConfigProperty<String> AUTO_GENERATE_RECORD_KEYS = ConfigProperty
+ .key("hoodie.auto.generate.record.keys")
+ .defaultValue("false")
+ .sinceVersion("0.13.0")
+ .withDocumentation("When enabled, hudi will auto generate a deterministic key for a record based on the contents of the field. "
+ + "The keys are guaranteed to be deterministic but not unique, so they can only be used for insert workflows with deduplication disabled."
+ + "The class attempts to get sufficient uniqueness for keys to prevent frequent collisions by choosing the fields it uses in order of decreasing "
+ + "likelihood for uniqueness.");
+
+ public static final ConfigProperty<Integer> NUM_FIELDS_IN_AUTO_RECORDKEY_GENERATION = ConfigProperty
Review Comment:
I don't think we should expose this kind of configuration -- it's very confusing in terms of what exactly it affects, and how setting it will translate into desired outcome
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/keygen/TestAutoRecordKeyGenerator.java:
##########
@@ -29,66 +29,76 @@
import java.io.IOException;
-public class TestKeylessKeyGenerator {
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+public class TestAutoRecordKeyGenerator {
private static final long TIME = 1672265446090L;
private static final Schema SCHEMA;
+ private static final String PARTITION_PATH_STR = "partition1";
static {
try {
- SCHEMA = new Schema.Parser().parse(TestKeylessKeyGenerator.class.getClassLoader().getResourceAsStream("keyless_schema.avsc"));
+ SCHEMA = new Schema.Parser().parse(TestAutoRecordKeyGenerator.class.getClassLoader().getResourceAsStream("keyless_schema.avsc"));
} catch (IOException ex) {
throw new RuntimeException(ex);
}
}
@Test
public void createKeyWithoutPartitionColumn() {
- KeylessKeyGenerator keyGenerator = new KeylessKeyGenerator(getKeyGenProperties("", 3));
- GenericRecord record = createRecord("partition1", "value1", 123, 456L, TIME, null);
+ ComplexAvroKeyGenerator keyGenerator = new ComplexAvroKeyGenerator(getKeyGenProperties("partition_field", 3));
Review Comment:
Why are we testing `ComplexKeyGenerator` here?
We need to test 2 concerns:
- `AutoRecordKeyGenerator` itself
- All existing key-generators in auto-gen mode (just one parameterized test for `getRecordKey` should be fine)
##########
hudi-common/src/main/java/org/apache/hudi/keygen/BaseKeyGenerator.java:
##########
@@ -32,11 +32,16 @@
*/
public abstract class BaseKeyGenerator extends KeyGenerator {
+ protected static final String DEFAULT_PARTITION_PATH_SEPARATOR = "/";
+ public static final String SPLIT_REGEX = ":";
+
protected List<String> recordKeyFields;
protected List<String> partitionPathFields;
protected final boolean encodePartitionPath;
protected final boolean hiveStylePartitioning;
protected final boolean consistentLogicalTimestampEnabled;
+ protected final boolean autoGenerateRecordKeys;
Review Comment:
We don't need a separate boolean, let's hold auto-gen as an option
--
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
[GitHub] [hudi] nsivabalan commented on a diff in pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "nsivabalan (via GitHub)" <gi...@apache.org>.
nsivabalan commented on code in PR #7726:
URL: https://github.com/apache/hudi/pull/7726#discussion_r1084630916
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/TestAutoRecordKeyGeneration.scala:
##########
@@ -0,0 +1,119 @@
+/*
+ * 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
+
+
+import org.apache.hudi.common.fs.FSUtils
+import org.apache.hudi.common.testutils.HoodieTestDataGenerator
+import org.apache.hudi.common.testutils.RawTripTestPayload.recordsToStrings
+import org.apache.hudi.config.HoodieWriteConfig
+import org.apache.hudi.keygen.TimestampBasedKeyGenerator
+import org.apache.hudi.keygen.constant.KeyGeneratorOptions
+import org.apache.hudi.testutils.SparkClientFunctionalTestHarness
+import org.apache.spark.sql._
+import org.junit.jupiter.api.Assertions.{assertEquals, assertTrue}
+import org.junit.jupiter.params.ParameterizedTest
+import org.junit.jupiter.params.provider.CsvSource
+
+import scala.collection.JavaConversions._
+
+/**
+ * Tests auto generation of record keys.
+ */
+class TestAutoRecordKeyGeneration extends SparkClientFunctionalTestHarness {
Review Comment:
https://issues.apache.org/jira/browse/HUDI-5604
Will ask Lokesh or Jon to take it up. I am occupied w/ few other things to wrap up 0.13.0.
--
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
[GitHub] [hudi] hudi-bot commented on pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #7726:
URL: https://github.com/apache/hudi/pull/7726#issuecomment-1401106520
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "25fbf375105d43c8a8707599c0eba58e01dea6f2",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14539",
"triggerID" : "25fbf375105d43c8a8707599c0eba58e01dea6f2",
"triggerType" : "PUSH"
}, {
"hash" : "efe71f4ece3aeb318285d00b5de153aa182a35d4",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "efe71f4ece3aeb318285d00b5de153aa182a35d4",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 25fbf375105d43c8a8707599c0eba58e01dea6f2 Azure: [FAILURE](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14539)
* efe71f4ece3aeb318285d00b5de153aa182a35d4 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
[GitHub] [hudi] nsivabalan commented on pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "nsivabalan (via GitHub)" <gi...@apache.org>.
nsivabalan commented on PR #7726:
URL: https://github.com/apache/hudi/pull/7726#issuecomment-1401099309
@codope : addressed all feedback.
--
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
[GitHub] [hudi] hudi-bot commented on pull request #7726: [HUDI-5575] Adding/Fixing auto generation of record keys w/ hudi
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #7726:
URL: https://github.com/apache/hudi/pull/7726#issuecomment-1401834191
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "25fbf375105d43c8a8707599c0eba58e01dea6f2",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14539",
"triggerID" : "25fbf375105d43c8a8707599c0eba58e01dea6f2",
"triggerType" : "PUSH"
}, {
"hash" : "efe71f4ece3aeb318285d00b5de153aa182a35d4",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14569",
"triggerID" : "efe71f4ece3aeb318285d00b5de153aa182a35d4",
"triggerType" : "PUSH"
}, {
"hash" : "7de21a1d14ad52d84841a31bc850341fd66033e5",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14591",
"triggerID" : "7de21a1d14ad52d84841a31bc850341fd66033e5",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* efe71f4ece3aeb318285d00b5de153aa182a35d4 Azure: [FAILURE](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14569)
* 7de21a1d14ad52d84841a31bc850341fd66033e5 Azure: [PENDING](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=14591)
<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