[
https://issues.apache.org/jira/browse/AVRO-2840?focusedWorklogId=652274&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-652274
]
ASF GitHub Bot logged work on AVRO-2840:
----------------------------------------
Author: ASF GitHub Bot
Created on: 17/Sep/21 11:27
Start Date: 17/Sep/21 11:27
Worklog Time Spent: 10m
Work Description: martin-g commented on a change in pull request #885:
URL: https://github.com/apache/avro/pull/885#discussion_r710947377
##########
File path: lang/java/avro/src/main/java/org/apache/avro/LogicalTypes.java
##########
@@ -51,9 +65,27 @@ default String getTypeName() {
public static void register(String logicalTypeName, LogicalTypeFactory
factory) {
Objects.requireNonNull(logicalTypeName, "Logical type name cannot be
null");
Objects.requireNonNull(factory, "Logical type factory cannot be null");
+
+ try {
+ String factoryTypeName = factory.getTypeName();
+ if (!logicalTypeName.equals(factoryTypeName)) {
+ throw new IllegalArgumentException(String.format(
Review comment:
This might break some old users of this API.
##########
File path: lang/java/avro/src/main/java/org/apache/avro/LogicalTypes.java
##########
@@ -51,9 +65,27 @@ default String getTypeName() {
public static void register(String logicalTypeName, LogicalTypeFactory
factory) {
Objects.requireNonNull(logicalTypeName, "Logical type name cannot be
null");
Objects.requireNonNull(factory, "Logical type factory cannot be null");
+
+ try {
+ String factoryTypeName = factory.getTypeName();
+ if (!logicalTypeName.equals(factoryTypeName)) {
+ throw new IllegalArgumentException(String.format(
+ "Provided logicalTypeName '%s' does not match factory typeName
'%s'", logicalTypeName, factoryTypeName));
+ }
+ } catch (UnsupportedOperationException ignore) {
+ // ignore exception, as by default this value has not been provided
Review comment:
Which value is not provided ? It is not very clear.
It seems only `factory.getTypeName();` can throw it. And it looks like a
hack to avoid the new protection.
##########
File path: lang/java/avro/src/test/java/org/apache/avro/TestLogicalType.java
##########
@@ -226,6 +228,73 @@ public void testLogicalTypeInSchemaEquals() {
assertEqualsFalse("Different logical type", schema1, schema3);
}
+ @Test
+ public void testRegisterLogicalTypeThrowsIfTypeNameInconsistent() {
+ assertThrows("Should error if type name is inconsistent",
IllegalArgumentException.class,
+ "Provided logicalTypeName 'name' does not match factory typeName
'different'", () -> {
+ LogicalTypes.register("name", new LogicalTypes.LogicalTypeFactory() {
+ @Override
+ public LogicalType fromSchema(Schema schema) {
+ return LogicalTypes.date();
+ }
+
+ @Override
+ public String getTypeName() {
+ return "different";
+ }
+ });
+ return null;
+ });
+ }
+
+ @Test
+ public void testRegisterLogicalTypeWithName() {
+ final LogicalTypes.LogicalTypeFactory factory = new
LogicalTypes.LogicalTypeFactory() {
+ @Override
+ public LogicalType fromSchema(Schema schema) {
+ return LogicalTypes.date();
+ }
+
+ @Override
+ public String getTypeName() {
+ return "same";
+ }
+ };
+
+ LogicalTypes.register("same", factory);
+
+ MatcherAssert.assertThat(LogicalTypes.getCustomRegisteredTypes(),
IsMapContaining.hasEntry("same", factory));
+ }
+
+ @Test
+ public void testRegisterLogicalTypeWithFactoryName() {
+ final LogicalTypes.LogicalTypeFactory factory = new
LogicalTypes.LogicalTypeFactory() {
+ @Override
+ public LogicalType fromSchema(Schema schema) {
+ return LogicalTypes.date();
+ }
+
+ @Override
+ public String getTypeName() {
+ return "factory";
+ }
+ };
+
+ LogicalTypes.register(factory);
+
+ MatcherAssert.assertThat(LogicalTypes.getCustomRegisteredTypes(),
IsMapContaining.hasEntry("factory", factory));
+ }
+
+ @Test
+ public void testRegisterLogicalTypeWithFactoryNameNotProvidedWithoutError() {
+ final LogicalTypes.LogicalTypeFactory factory = schema ->
LogicalTypes.date();
+
+ LogicalTypes.register("logicalTypeName", factory);
+
+ MatcherAssert.assertThat(LogicalTypes.getCustomRegisteredTypes(),
+ IsMapContaining.hasEntry("logicalTypeName", factory));
+ }
+
Review comment:
It is clear to me now!
I'll let others decide whether the improvement (the names check) is allowed
now. I see its value but as I said it might break someone's app.
##########
File path:
lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java
##########
@@ -300,8 +305,8 @@ public void testSettingOutputCharacterEncoding() throws
Exception {
is.close(); // close input stream otherwise delete might fail
if (!this.outputFile.delete()) {
throw new IllegalStateException("unable to delete " + this.outputFile);
// delete otherwise compiler might not
-
// overwrite because src timestamp hasn't
-
// changed.
+ // overwrite because src timestamp hasn't
Review comment:
Please restore the formatting or move the whole comment on the previous
line
##########
File path:
lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java
##########
@@ -516,6 +521,36 @@ public void testNullableTypesJavaUnbox() throws Exception {
"java.lang.Boolean");
}
+ @Test
+ public void testGetUsedCustomLogicalTypeFactories() throws Exception {
+ LogicalTypes.register("string-constant", new
StringCustomLogicalTypeFactory());
+
+ SpecificCompiler compiler = createCompiler();
+ compiler.setEnableDecimalLogicalType(true);
+
+ final Schema schema = new Schema.Parser().parse(
+
"{\"type\":\"record\",\"name\":\"NestedLogicalTypesRecord\",\"namespace\":\"org.apache.avro.codegentest.testdata\",\"doc\":\"Test
nested types with logical types in generated Java
classes\",\"fields\":[{\"name\":\"nestedRecord\",\"type\":{\"type\":\"record\",\"name\":\"NestedRecord\",\"fields\":[{\"name\":\"nullableDateField\",\"type\":[\"null\",{\"type\":\"int\",\"logicalType\":\"date\"}]}]}},{\"name\":\"myLogical\",\"type\":{\"type\":\"string\",\"logicalType\":\"string-constant\"}}]}");
Review comment:
let's split this line into several shorter concatenated ones
##########
File path:
lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java
##########
@@ -860,4 +895,11 @@ public void testAdditionalToolsAreInjectedIntoTemplate()
throws Exception {
assertEquals(1, itWorksFound);
}
+ public static class StringCustomLogicalTypeFactory implements
LogicalTypes.LogicalTypeFactory {
+ @Override
+ public LogicalType fromSchema(Schema schema) {
+ return new LogicalType("string-constant");
Review comment:
`"string-custom"` ?!
##########
File path:
lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java
##########
@@ -282,37 +287,51 @@ public void addCustomConversion(Class<?> conversionClass)
{
public Collection<String> getUsedConversionClasses(Schema schema) {
Collection<String> result = new HashSet<>();
- for (Conversion<?> conversion : getUsedConversions(schema, new
HashSet<>(), new HashSet<>())) {
+ for (Conversion<?> conversion : getUsedConversions(schema)) {
result.add(conversion.getClass().getCanonicalName());
}
return result;
}
- private Set<Conversion<?>> getUsedConversions(Schema schema,
Set<Conversion<?>> result, Set<Schema> seenSchemas) {
+ public Set<String> getUsedCustomLogicalTypeFactories(Schema schema) {
+ final Set<String> logicalTypeNames =
getUsedLogicalTypes(schema).stream().map(LogicalType::getName)
+ .collect(Collectors.toSet());
+
+ return LogicalTypes.getCustomRegisteredTypes().entrySet().stream()
+ .filter(entry -> logicalTypeNames.contains(entry.getKey()))
+ .map(entry ->
entry.getValue().getClass().getCanonicalName()).collect(Collectors.toSet());
+ }
+
+ private void getUsedTypes(Schema schema, Set<Conversion<?>>
conversionResults, Set<LogicalType> logicalTypeResults,
Review comment:
Since the method's return type is `void` I would suggest to rename it to
`collectUsedTypes`.
##########
File path:
lang/java/integration-test/codegen-test/src/test/resources/avro/custom_conversion_logical_types_enum.avsc
##########
@@ -0,0 +1,19 @@
+{"namespace": "org.apache.avro.codegentest.testdata",
Review comment:
nit: move the namespace on a new line
##########
File path:
lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java
##########
@@ -516,6 +521,36 @@ public void testNullableTypesJavaUnbox() throws Exception {
"java.lang.Boolean");
}
+ @Test
+ public void testGetUsedCustomLogicalTypeFactories() throws Exception {
+ LogicalTypes.register("string-constant", new
StringCustomLogicalTypeFactory());
+
+ SpecificCompiler compiler = createCompiler();
+ compiler.setEnableDecimalLogicalType(true);
+
+ final Schema schema = new Schema.Parser().parse(
+
"{\"type\":\"record\",\"name\":\"NestedLogicalTypesRecord\",\"namespace\":\"org.apache.avro.codegentest.testdata\",\"doc\":\"Test
nested types with logical types in generated Java
classes\",\"fields\":[{\"name\":\"nestedRecord\",\"type\":{\"type\":\"record\",\"name\":\"NestedRecord\",\"fields\":[{\"name\":\"nullableDateField\",\"type\":[\"null\",{\"type\":\"int\",\"logicalType\":\"date\"}]}]}},{\"name\":\"myLogical\",\"type\":{\"type\":\"string\",\"logicalType\":\"string-constant\"}}]}");
+
+ final Set<String> usedCustomLogicalTypeFactories =
compiler.getUsedCustomLogicalTypeFactories(schema);
+ Assert.assertEquals(1, usedCustomLogicalTypeFactories.size());
+
Assert.assertEquals("org.apache.avro.compiler.specific.TestSpecificCompiler.StringCustomLogicalTypeFactory",
+ usedCustomLogicalTypeFactories.iterator().next());
+ }
+
+ @Test
+ public void testEmptyGetUsedCustomLogicalTypeFactories() throws Exception {
+ LogicalTypes.register("string-constant", new
StringCustomLogicalTypeFactory());
+
+ SpecificCompiler compiler = createCompiler();
+ compiler.setEnableDecimalLogicalType(true);
+
+ final Schema schema = new Schema.Parser().parse(
+
"{\"type\":\"record\",\"name\":\"NestedLogicalTypesRecord\",\"namespace\":\"org.apache.avro.codegentest.testdata\",\"doc\":\"Test
nested types with logical types in generated Java
classes\",\"fields\":[{\"name\":\"nestedRecord\",\"type\":{\"type\":\"record\",\"name\":\"NestedRecord\",\"fields\":[{\"name\":\"nullableDateField\",\"type\":[\"null\",{\"type\":\"int\",\"logicalType\":\"date\"}]}]}}]}");
Review comment:
line too long
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 652274)
Time Spent: 50m (was: 40m)
> Maven allow Custom Logical Types for generated code
> ---------------------------------------------------
>
> Key: AVRO-2840
> URL: https://issues.apache.org/jira/browse/AVRO-2840
> Project: Apache Avro
> Issue Type: Improvement
> Components: java
> Reporter: Matthew McMahon
> Priority: Major
> Labels: maven, pull-request-available
> Attachments: AVRO-2840.patch
>
> Time Spent: 50m
> Remaining Estimate: 0h
>
> I was looking for a way to use my Custom Conversion in generated code, but as
> detailed, without the Custom Logical Type being registered in the static
> REGISTERED_TYPES of LogicalTypes, then it will not be considered.
> Therefore this issue is an attempt to add this ability into Avro Maven Plugin
> by adding a new configuration map type to the plugin, and will also register
> this logical type in the generated code.
>
> {panel:title=Cloned from AVRO-2621}
> So, as seen in AVRO-2498 we now have the ability in 1.9, at least using the
> Maven tools, to have generated classes with a field of type UUID. For some
> background, the secret sauce to get this working is:
> * Set the logicalType of the field to 'uuid' and type to 'string'
> * In the maven plugin configuration, add an item to <customConversions> with
> the value org.apache.avro.Conversions$UUIDConversion
> I attempted this with a custom type of my own (in this case, joda Money)
> first by implementing a CustomConversion<Money> class that created a new
> LogicalType called "money". This did not work, though, because although the
> maven mojo correctly added the CustomConversion to the SpecificCompiler, when
> the compiler parses the schema and sees the logicalType of type "money", it
> attempts to look up the LogicalType from LogicalTypes the LogicalType is not
> registered and it ignores it, which causes the field within the Schema to not
> have a logicalType. Without a logicalType, the compiler does not consider it
> a "UsedConversionClass" (see SpecificCompiler.getUsedConversionClass()) and
> they velocity template doesn't write the class with the custom field type.
> Trying to be clever, I attempted to work around this by putting a static
> initializer in my CustomConversion class which called
> LogicalTypes.register(). Unfortunately for me, the maven plugin doesn't load
> any of the classes specified in <customConversions> until AFTER it has parsed
> a schema.
> Obviously, putting a static initializer inside of a CustomConversion should
> not be part of a solution to this issue, but I propose a very simple solution:
> * Add a logicalTypes property to AbstractAvroMojo, with items of a new type,
> LocalTypeDefinition. That type will have a name and a logicalTypeFactory.
> * Upon startup of the mojo, iterate through each item and call
> LogicalTypes.register() on each, passing the indicated name and a new
> instance of the indicated logicalTypeFactory class.
> * At this point, the existing logic will treat the new custom type the same
> as "uuid" is treated and will generate SpecificRecord classes with fields of
> that type.
> I could implement this pretty trivially, if there's an appetite for it. This
> would seem to be 100% backwards compatible.
> {panel}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
