Skip to content

AVRO-3649: reorder union types to match default #1919

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Danny02 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

AVRO-3649: reorder union types to match default #1919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions lang/java/avro/src/main/java/org/apache/avro/Schema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1619,6 +1619,16 @@ private static JsonNode validateDefault(String fieldName, Schema schema, JsonNod
return defaultValue;
}

/**
* Checks if a JSON value matches the schema.
*
* @param jsonValue a value to check against the schema
* @return true if the value is valid according to this schema
*/
public boolean isValidValue(JsonNode jsonValue) {
return isValidDefault(this, jsonValue);
}

@clesaec clesaec Oct 20, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 use standard method instead of static (it may replace private static boolean isValidDefault in near future), and avoid this long switch/case on schema/type.
About default value on union type, I wonder if it couldn't be even better to change logic and get the first compatible Schema of union with the value (And throw exception if none) ?
I did this code in local (and partially copy yours), and it seems to work fine (at least for unit test). I will put more details in linked JIRA Ticket in few minutes.

@Danny02 Danny02 Oct 20, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx for the feedback, can u give me some points what standard method you are talking about?

@Danny02 Danny02 Oct 20, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just was able to take a look at your PR. It looks like a even better solution, because it "fixes" the problem on a lower level.

Would you say that this PR is now obsolete?

@clesaec clesaec Oct 20, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

standard method => i would say "non static overridable method" (better for object programming).
For 2 PR of 2 solution, I think we should discuss about that on the JIRA issue, i may missed some point (i want advice of other developer).

@RyanSkraba RyanSkraba Oct 20, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any possible way we can rewrite this without adding or exposing the isValidValue(JsonNode ...) method?

There was a reason the isValidDefault method is private: we really don't want to be exposing the jackson internals in public methods and (if I remember correctly). There was a fair amount of work deleting these methods in the past, in order to one day replace the jackson internals.

If anyone has a better memory than me, please feel to chime in!


private static boolean isValidDefault(Schema schema, JsonNode defaultValue) {
if (defaultValue == null)
return false;
Expand Down
23 changes: 23 additions & 0 deletions lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
*/
package org.apache.avro.reflect;

import com.fasterxml.jackson.databind.JsonNode;
import org.apache.avro.AvroRuntimeException;
import org.apache.avro.AvroTypeException;
import org.apache.avro.Conversion;
Expand All @@ -37,6 +38,7 @@
import org.apache.avro.specific.SpecificData;
import org.apache.avro.util.ClassUtils;
import org.apache.avro.util.MapUtil;
import org.apache.avro.util.internal.JacksonUtils;

import java.io.IOException;
import java.lang.annotation.Annotation;
Expand All @@ -62,9 +64,11 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.WeakHashMap;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.function.Predicate;

/** Utilities to use existing Java classes and interfaces via reflection. */
public class ReflectData extends SpecificData {
Expand Down Expand Up @@ -752,6 +756,25 @@ protected Schema createSchema(Type type, Map<String, Schema> names) {
if (STRING_OUTER_PARENT_REFERENCE.equals(fieldName)) {
throw new AvroTypeException("Class " + fullName + " must be a static inner class");
}

// We need to reorder the union types if the default value is not of the first schema.
// Avro requires that the default value is of the same type as the first schema in the union.
if (fieldSchema.getType() == Schema.Type.UNION && defaultValue != null) {
List<Schema> unionTypes = fieldSchema.getTypes();
JsonNode defJson = JacksonUtils.toJsonNode(defaultValue);
// we only need to reorder the types if the default value is not compatible with the first schema
for (int i = 1; i < unionTypes.size(); i++) {
Schema s = unionTypes.get(i);
if (s.isValidValue(defJson)) {
ArrayList<Schema> reordered = new ArrayList<>(unionTypes);
reordered.remove(s);
reordered.add(0, s);
fieldSchema = Schema.createUnion(reordered);
break;
}
}
}

Schema.Field recordField = new Schema.Field(fieldName, fieldSchema, doc, defaultValue);

AvroMeta[] metadata = field.getAnnotationsByType(AvroMeta.class); // add metadata
Expand Down
28 changes: 28 additions & 0 deletions lang/java/avro/src/test/java/org/apache/avro/reflect/TestReflect.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1281,6 +1281,34 @@ public void testMultipleFieldAliases() {
check(ClassWithMultipleAliasesOnField.class, expectedSchema.toString());
}

private static class NullableDefaultTest {
@Nullable
@AvroDefault("1")
int foo;
}

@Test
public void testAvroNullableDefault() {
check(NullableDefaultTest.class,
"{\"type\":\"record\",\"name\":\"NullableDefaultTest\","
+ "\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":["
+ "{\"name\":\"foo\",\"type\":[\"int\",\"null\"],\"default\":1}]}");
}

private static class UnionDefaultTest {
@Union({String.class, Integer.class})
@AvroDefault("1")
Object foo;
}

@Test
public void testAvroUnionDefault() {
check(UnionDefaultTest.class,
"{\"type\":\"record\",\"name\":\"UnionDefaultTest\","
+ "\"namespace\":\"org.apache.avro.reflect.TestReflect\",\"fields\":["
+ "{\"name\":\"foo\",\"type\":[\"int\",\"string\"],\"default\":1}]}");
}

private static class DefaultTest {
@AvroDefault("1")
int foo;
Expand Down