nsivabalan commented on a change in pull request #4203:
URL: https://github.com/apache/hudi/pull/4203#discussion_r778410194
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/keygen/TimestampBasedAvroKeyGenerator.java
##########
@@ -125,7 +126,7 @@ public TimestampBasedAvroKeyGenerator(TypedProperties
config) throws IOException
@Override
public String getPartitionPath(GenericRecord record) {
- Object partitionVal = HoodieAvroUtils.getNestedFieldVal(record,
getPartitionPathFields().get(0), true);
+ Object partitionVal = HoodieAvroUtils.getNestedFieldVal(record,
getPartitionPathFields().get(0), true, isConsistentLogicalTimestampEnabled());
Review comment:
can't we fetch the value of isConsistentLogicalTimestampEnabled in the
constructor from props?
```
TimestampBasedKeyGenerator(TypedProperties config)
```
We should be very cautious in changing public apis. KeyGenerator apis are
public and likely users have their custom key gen.
##########
File path:
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java
##########
@@ -889,6 +890,13 @@ public String getKeyGeneratorClass() {
return getString(KEYGENERATOR_CLASS_NAME);
}
+ public boolean isConsistentLogicalTimestampEnabled() {
+ if
(getBoolean(KeyGeneratorOptions.KEYGENERATOR_CONSISTENT_LOGICAL_TIMESTAMP_ENABLED)
== null) {
Review comment:
wouldn't getBoolean return the default if not found?
##########
File path:
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/SqlKeyGenerator.scala
##########
@@ -96,7 +96,7 @@ class SqlKeyGenerator(props: TypedProperties) extends
ComplexKeyGenerator(props)
val timeMs = if (rowType) { // In RowType, the
partitionPathValue is the time format string, convert to millis
SqlKeyGenerator.sqlTimestampFormat.parseMillis(_partitionValue)
} else {
- MILLISECONDS.convert(_partitionValue.toLong, MICROSECONDS)
+ Timestamp.valueOf(_partitionValue).getTime
Review comment:
CC: @xushiyan @YannByron
##########
File path:
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/keygen/TestTimestampBasedKeyGenerator.java
##########
@@ -238,6 +238,40 @@ public void testScalar() throws IOException {
assertEquals("2021-04-19", keyGen.getPartitionPath(baseRow));
}
+ @Test
+ public void testScalarWithLogicalType() throws IOException {
+ schema = SchemaTestUtil.getTimestampWithLogicalTypeSchema();
+ structType = AvroConversionUtils.convertAvroSchemaToStructType(schema);
+ baseRecord = SchemaTestUtil.generateAvroRecordFromJson(schema, 1, "001",
"f1");
+ baseRecord.put("createTime", 1638513806000000L);
+
+ properties = getBaseKeyConfig("SCALAR", "yyyy/MM/dd", "GMT",
"MICROSECONDS");
+
properties.setProperty(KeyGeneratorOptions.KEYGENERATOR_CONSISTENT_LOGICAL_TIMESTAMP_ENABLED.key(),
"true");
+ TimestampBasedKeyGenerator keyGen = new
TimestampBasedKeyGenerator(properties);
+ HoodieKey hk1 = keyGen.getKey(baseRecord);
+ assertEquals("2021/12/03", hk1.getPartitionPath());
+
+ // test w/ Row
+ baseRow = genericRecordToRow(baseRecord);
+ assertEquals("2021/12/03", keyGen.getPartitionPath(baseRow));
+ internalRow = KeyGeneratorTestUtilities.getInternalRow(baseRow);
+ assertEquals("2021/12/03", keyGen.getPartitionPath(internalRow,
baseRow.schema()));
Review comment:
were these returning diff values if the config is not 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-unsubscr...@hudi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
