You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "reuvenlax (via GitHub)" <gi...@apache.org> on 2024/10/11 18:44:02 UTC
[PR] Schema inference parameterized types [beam]
reuvenlax opened a new pull request, #32757:
URL: https://github.com/apache/beam/pull/32757
=
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema inference parameterized types [beam]
Posted by "reuvenlax (via GitHub)" <gi...@apache.org>.
reuvenlax commented on code in PR #32757:
URL: https://github.com/apache/beam/pull/32757#discussion_r1799844830
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/StaticSchemaInference.java:
##########
@@ -85,14 +86,17 @@ enum MethodType {
* public getter methods, or special annotations on the class.
*/
public static Schema schemaFromClass(
- TypeDescriptor<?> typeDescriptor, FieldValueTypeSupplier fieldValueTypeSupplier) {
- return schemaFromClass(typeDescriptor, fieldValueTypeSupplier, new HashMap<>());
+ TypeDescriptor<?> typeDescriptor,
+ FieldValueTypeSupplier fieldValueTypeSupplier,
+ Map<Type, Type> boundTypes) {
Review Comment:
Added Internal annotation. the fact that it wasn't so marked before was an oversight.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ReflectUtils.java:
##########
@@ -243,4 +255,45 @@ public static TypeDescriptor boxIfPrimitive(TypeDescriptor typeDescriptor) {
? TypeDescriptor.of(Primitives.wrap(typeDescriptor.getRawType()))
: typeDescriptor;
}
+
+ public static <T> Map<Type, Type> getAllBoundTypes(TypeDescriptor<T> typeDescriptor) {
+ Map<Type, Type> boundParameters = Maps.newHashMap();
+ TypeDescriptor<?> currentType = typeDescriptor;
+ do {
+ if (currentType.getType() instanceof ParameterizedType) {
+ ParameterizedType parameterizedType = (ParameterizedType) currentType.getType();
+ TypeVariable<?>[] typeVariables = currentType.getRawType().getTypeParameters();
+ Type[] typeArguments = parameterizedType.getActualTypeArguments();
+ ;
+ if (typeArguments.length != typeVariables.length) {
+ throw new RuntimeException("Unmatching arguments lengths");
+ }
+ for (int i = 0; i < typeVariables.length; ++i) {
+ boundParameters.put(typeVariables[i], typeArguments[i]);
+ }
+ }
+ Type superClass = currentType.getRawType().getGenericSuperclass();
+ if (superClass == null || superClass.equals(Object.class)) {
+ break;
+ }
+ currentType = TypeDescriptor.of(superClass);
+ } while (true);
+ return boundParameters;
+ }
+
+ public static Type resolveType(Type type, Map<Type, Type> boundTypes) {
+ TypeDescriptor<?> typeDescriptor = TypeDescriptor.of(type);
+ if (typeDescriptor.isSubtypeOf(TypeDescriptor.of(Iterable.class))
+ || typeDescriptor.isSubtypeOf(TypeDescriptor.of(Map.class))) {
+ // Don't resolve these as we special case map and interable.
+ return type;
+ }
+
+ if (type instanceof TypeVariable) {
+ TypeVariable<?> typeVariable = (TypeVariable<?>) type;
+ return Preconditions.checkArgumentNotNull(boundTypes.get(typeVariable));
Review Comment:
I'd prefer not to, as that would simply be hiding a bug which might cause other problems (often caused by a call to getType() instead of getGenericType()).
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ReflectUtils.java:
##########
@@ -243,4 +255,45 @@ public static TypeDescriptor boxIfPrimitive(TypeDescriptor typeDescriptor) {
? TypeDescriptor.of(Primitives.wrap(typeDescriptor.getRawType()))
: typeDescriptor;
}
+
+ public static <T> Map<Type, Type> getAllBoundTypes(TypeDescriptor<T> typeDescriptor) {
+ Map<Type, Type> boundParameters = Maps.newHashMap();
+ TypeDescriptor<?> currentType = typeDescriptor;
+ do {
+ if (currentType.getType() instanceof ParameterizedType) {
+ ParameterizedType parameterizedType = (ParameterizedType) currentType.getType();
+ TypeVariable<?>[] typeVariables = currentType.getRawType().getTypeParameters();
+ Type[] typeArguments = parameterizedType.getActualTypeArguments();
+ ;
+ if (typeArguments.length != typeVariables.length) {
+ throw new RuntimeException("Unmatching arguments lengths");
Review Comment:
Done
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ReflectUtils.java:
##########
@@ -243,4 +255,45 @@ public static TypeDescriptor boxIfPrimitive(TypeDescriptor typeDescriptor) {
? TypeDescriptor.of(Primitives.wrap(typeDescriptor.getRawType()))
: typeDescriptor;
}
+
+ public static <T> Map<Type, Type> getAllBoundTypes(TypeDescriptor<T> typeDescriptor) {
Review Comment:
Done
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema inference parameterized types [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #32757:
URL: https://github.com/apache/beam/pull/32757#issuecomment-2416355867
I believe this change introduced a regression (see #32795)
Iceberg [unit](https://github.com/apache/beam/actions/workflows/IO_Iceberg_Unit_Tests.yml) and [integration](https://github.com/apache/beam/actions/workflows/IO_Iceberg_Integration_Tests.yml) tests are failing with the following error:
```
java.lang.IllegalArgumentException: unable to serialize SchemaCoder<Schema: Fields:
Field{...
Field{...
```
Tests are failing at HEAD, but passing when reverting this PR: #32802
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema inference parameterized types [beam]
Posted by "reuvenlax (via GitHub)" <gi...@apache.org>.
reuvenlax commented on PR #32757:
URL: https://github.com/apache/beam/pull/32757#issuecomment-2414724698
@ahmedabu98 Any more comments? Are you ok with marking these classes as Internal, or do you prefer adding overloads?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema inference parameterized types [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32757:
URL: https://github.com/apache/beam/pull/32757#issuecomment-2408015787
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment `assign set of reviewers`
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema inference parameterized types [beam]
Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on PR #32757:
URL: https://github.com/apache/beam/pull/32757#issuecomment-2419524340
> The fix has been submitted - is that Yaml test still failing? Separately we should figure out why none of those tests triggered on my initial PR :(
> […](#)
> On Wed, Oct 16, 2024 at 6:00 PM Kenn Knowles ***@***.***> wrote: Suspected culprit of breaking this one too https://github.com/apache/beam/actions/workflows/beam_PreCommit_Yaml_Xlang_Direct.yml?query=branch%3Amaster+is%3Acompleted — Reply to this email directly, view it on GitHub <[#32757 (comment)](https://github.com/apache/beam/pull/32757#issuecomment-2418271581)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFAYJVJWYRWOHOV74QY6PS3Z34D4HAVCNFSM6AAAAABPZOTUP2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJYGI3TCNJYGE> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>
Yea we could see if there is a natural precommit trigger that would make sense. It may be that these are far enough apart in the codebase that postcommit is where we expect to catch it anyhow. Then we would roll back and roll forward touching a trigger file in `.github/trigger_files`
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema inference parameterized types [beam]
Posted by "reuvenlax (via GitHub)" <gi...@apache.org>.
reuvenlax commented on code in PR #32757:
URL: https://github.com/apache/beam/pull/32757#discussion_r1799847569
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/AutoValueUtils.java:
##########
@@ -161,7 +162,8 @@ private static boolean matchConstructor(
// Verify that constructor parameters match (name and type) the inferred schema.
for (Parameter parameter : constructor.getParameters()) {
FieldValueTypeInformation type = typeMap.get(parameter.getName());
- if (type == null || type.getRawType() != parameter.getType()) {
+ ;
Review Comment:
Done
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema inference parameterized types [beam]
Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on PR #32757:
URL: https://github.com/apache/beam/pull/32757#issuecomment-2418271581
Suspected culprit of breaking this one too https://github.com/apache/beam/actions/workflows/beam_PreCommit_Yaml_Xlang_Direct.yml?query=branch%3Amaster+is%3Acompleted
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema inference parameterized types [beam]
Posted by "reuvenlax (via GitHub)" <gi...@apache.org>.
reuvenlax commented on code in PR #32757:
URL: https://github.com/apache/beam/pull/32757#discussion_r1799816927
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ReflectUtils.java:
##########
@@ -88,14 +89,23 @@ public static List<Method> getMethods(Class<?> clazz) {
return DECLARED_METHODS.computeIfAbsent(
clazz,
c -> {
- return Arrays.stream(c.getDeclaredMethods())
- .filter(
- m -> !m.isBridge()) // Covariant overloads insert bridge functions, which we must
- // ignore.
- .filter(m -> !Modifier.isPrivate(m.getModifiers()))
- .filter(m -> !Modifier.isProtected(m.getModifiers()))
- .filter(m -> !Modifier.isStatic(m.getModifiers()))
- .collect(Collectors.toList());
+ List<Method> methods = Lists.newArrayList();
+ do {
+ if (c.getPackage() != null && c.getPackage().getName().startsWith("java.")) {
+ break; // skip java built-in classes
+ }
+ Arrays.stream(c.getDeclaredMethods())
+ .filter(
+ m ->
+ !m.isBridge()) // Covariant overloads insert bridge functions, which we must
+ // ignore.
+ .filter(m -> !Modifier.isPrivate(m.getModifiers()))
+ .filter(m -> !Modifier.isProtected(m.getModifiers()))
+ .filter(m -> !Modifier.isStatic(m.getModifiers()))
+ .forEach(methods::add);
+ c = c.getSuperclass();
Review Comment:
This was an existing bug that was uncovered by the new unit tests I added in this PR. JavaFieldSchema (for POJOs) finds all fields in superclasses and adds them to the schema, but we didn't do that for JavaBeans. I changed this to make things consistent and fix the broken tests.
BTW related to your other comments - we should probably mark all of these. helper classes as Internal. They were never meant for external users to use them, and this change (among others) is at least a functionality change
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema inference parameterized types [beam]
Posted by "reuvenlax (via GitHub)" <gi...@apache.org>.
reuvenlax commented on code in PR #32757:
URL: https://github.com/apache/beam/pull/32757#discussion_r1799811464
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/FieldValueTypeInformation.java:
##########
@@ -125,18 +126,20 @@ public static FieldValueTypeInformation forOneOf(
.build();
}
- public static FieldValueTypeInformation forField(Field field, int index) {
- TypeDescriptor<?> type = TypeDescriptor.of(field.getGenericType());
+ public static FieldValueTypeInformation forField(
+ Field field, int index, Map<Type, Type> boundTypes) {
Review Comment:
In this case, I think so. This is a logically an internal class, which was only marked public because we needed to access it from other packages (e.g. the protobuf package). The fact that it was not marked @Internal was likely a mistake, and I think it's fine to change it in this case.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema inference parameterized types [beam]
Posted by "reuvenlax (via GitHub)" <gi...@apache.org>.
reuvenlax commented on code in PR #32757:
URL: https://github.com/apache/beam/pull/32757#discussion_r1799845653
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/POJOUtils.java:
##########
@@ -73,8 +73,11 @@
public class POJOUtils {
public static Schema schemaFromPojoClass(
- TypeDescriptor<?> typeDescriptor, FieldValueTypeSupplier fieldValueTypeSupplier) {
- return StaticSchemaInference.schemaFromClass(typeDescriptor, fieldValueTypeSupplier);
+ TypeDescriptor<?> typeDescriptor,
+ FieldValueTypeSupplier fieldValueTypeSupplier,
+ Map<Type, Type> boundTypes) {
Review Comment:
Added Internal annotation. This was never meant to be used outside of Beam internals.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/JavaBeanUtils.java:
##########
@@ -64,8 +65,11 @@
public class JavaBeanUtils {
/** Create a {@link Schema} for a Java Bean class. */
public static Schema schemaFromJavaBeanClass(
- TypeDescriptor<?> typeDescriptor, FieldValueTypeSupplier fieldValueTypeSupplier) {
- return StaticSchemaInference.schemaFromClass(typeDescriptor, fieldValueTypeSupplier);
+ TypeDescriptor<?> typeDescriptor,
+ FieldValueTypeSupplier fieldValueTypeSupplier,
+ Map<Type, Type> boundTypes) {
Review Comment:
Ditto - added Internal
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema inference parameterized types [beam]
Posted by "stankiewicz (via GitHub)" <gi...@apache.org>.
stankiewicz commented on PR #32757:
URL: https://github.com/apache/beam/pull/32757#issuecomment-2419070258
I'm curious if prior work from #32081 had same issues. Can @tilgalas take a look as he explored this feature already and his PR was waiting in review queue for extended amount of time.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema inference parameterized types [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #32757:
URL: https://github.com/apache/beam/pull/32757#discussion_r1799082439
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ReflectUtils.java:
##########
@@ -243,4 +255,45 @@ public static TypeDescriptor boxIfPrimitive(TypeDescriptor typeDescriptor) {
? TypeDescriptor.of(Primitives.wrap(typeDescriptor.getRawType()))
: typeDescriptor;
}
+
+ public static <T> Map<Type, Type> getAllBoundTypes(TypeDescriptor<T> typeDescriptor) {
+ Map<Type, Type> boundParameters = Maps.newHashMap();
+ TypeDescriptor<?> currentType = typeDescriptor;
+ do {
+ if (currentType.getType() instanceof ParameterizedType) {
+ ParameterizedType parameterizedType = (ParameterizedType) currentType.getType();
+ TypeVariable<?>[] typeVariables = currentType.getRawType().getTypeParameters();
+ Type[] typeArguments = parameterizedType.getActualTypeArguments();
+ ;
+ if (typeArguments.length != typeVariables.length) {
+ throw new RuntimeException("Unmatching arguments lengths");
+ }
+ for (int i = 0; i < typeVariables.length; ++i) {
+ boundParameters.put(typeVariables[i], typeArguments[i]);
+ }
+ }
+ Type superClass = currentType.getRawType().getGenericSuperclass();
+ if (superClass == null || superClass.equals(Object.class)) {
+ break;
+ }
+ currentType = TypeDescriptor.of(superClass);
+ } while (true);
+ return boundParameters;
+ }
+
+ public static Type resolveType(Type type, Map<Type, Type> boundTypes) {
+ TypeDescriptor<?> typeDescriptor = TypeDescriptor.of(type);
+ if (typeDescriptor.isSubtypeOf(TypeDescriptor.of(Iterable.class))
+ || typeDescriptor.isSubtypeOf(TypeDescriptor.of(Map.class))) {
+ // Don't resolve these as we special case map and interable.
+ return type;
+ }
+
+ if (type instanceof TypeVariable) {
+ TypeVariable<?> typeVariable = (TypeVariable<?>) type;
+ return Preconditions.checkArgumentNotNull(boundTypes.get(typeVariable));
Review Comment:
Does it make sense to fall back on `type` if `boundTypes` doesn't contain it for whatever reason? Instead of failing here
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ReflectUtils.java:
##########
@@ -243,4 +255,45 @@ public static TypeDescriptor boxIfPrimitive(TypeDescriptor typeDescriptor) {
? TypeDescriptor.of(Primitives.wrap(typeDescriptor.getRawType()))
: typeDescriptor;
}
+
+ public static <T> Map<Type, Type> getAllBoundTypes(TypeDescriptor<T> typeDescriptor) {
+ Map<Type, Type> boundParameters = Maps.newHashMap();
+ TypeDescriptor<?> currentType = typeDescriptor;
+ do {
+ if (currentType.getType() instanceof ParameterizedType) {
+ ParameterizedType parameterizedType = (ParameterizedType) currentType.getType();
+ TypeVariable<?>[] typeVariables = currentType.getRawType().getTypeParameters();
+ Type[] typeArguments = parameterizedType.getActualTypeArguments();
+ ;
+ if (typeArguments.length != typeVariables.length) {
+ throw new RuntimeException("Unmatching arguments lengths");
Review Comment:
nit: include the typeDescriptor in the error message for easier debugging?
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ReflectUtils.java:
##########
@@ -243,4 +255,45 @@ public static TypeDescriptor boxIfPrimitive(TypeDescriptor typeDescriptor) {
? TypeDescriptor.of(Primitives.wrap(typeDescriptor.getRawType()))
: typeDescriptor;
}
+
+ public static <T> Map<Type, Type> getAllBoundTypes(TypeDescriptor<T> typeDescriptor) {
Review Comment:
nit: add a comment/javadoc for this method?
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/AutoValueUtils.java:
##########
@@ -161,7 +162,8 @@ private static boolean matchConstructor(
// Verify that constructor parameters match (name and type) the inferred schema.
for (Parameter parameter : constructor.getParameters()) {
FieldValueTypeInformation type = typeMap.get(parameter.getName());
- if (type == null || type.getRawType() != parameter.getType()) {
+ ;
Review Comment:
nit: cleanup
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ByteBuddyUtils.java:
##########
@@ -745,8 +751,8 @@ protected StackManipulation convertList(TypeDescriptor<?> type) {
@Override
protected StackManipulation convertMap(TypeDescriptor<?> type) {
- final TypeDescriptor keyType = ReflectUtils.getMapType(type, 0);
- final TypeDescriptor valueType = ReflectUtils.getMapType(type, 1);
+ final TypeDescriptor keyType = ReflectUtils.getMapType(type, 0, Collections.emptyMap());
Review Comment:
nit: might be cleaner to just overload `ReflectUtils.get____Type()` methods
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/StaticSchemaInference.java:
##########
@@ -85,14 +86,17 @@ enum MethodType {
* public getter methods, or special annotations on the class.
*/
public static Schema schemaFromClass(
- TypeDescriptor<?> typeDescriptor, FieldValueTypeSupplier fieldValueTypeSupplier) {
- return schemaFromClass(typeDescriptor, fieldValueTypeSupplier, new HashMap<>());
+ TypeDescriptor<?> typeDescriptor,
+ FieldValueTypeSupplier fieldValueTypeSupplier,
+ Map<Type, Type> boundTypes) {
Review Comment:
Same with `fieldFromType()` method below
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ReflectUtils.java:
##########
@@ -88,14 +89,23 @@ public static List<Method> getMethods(Class<?> clazz) {
return DECLARED_METHODS.computeIfAbsent(
clazz,
c -> {
- return Arrays.stream(c.getDeclaredMethods())
- .filter(
- m -> !m.isBridge()) // Covariant overloads insert bridge functions, which we must
- // ignore.
- .filter(m -> !Modifier.isPrivate(m.getModifiers()))
- .filter(m -> !Modifier.isProtected(m.getModifiers()))
- .filter(m -> !Modifier.isStatic(m.getModifiers()))
- .collect(Collectors.toList());
+ List<Method> methods = Lists.newArrayList();
+ do {
+ if (c.getPackage() != null && c.getPackage().getName().startsWith("java.")) {
+ break; // skip java built-in classes
+ }
+ Arrays.stream(c.getDeclaredMethods())
+ .filter(
+ m ->
+ !m.isBridge()) // Covariant overloads insert bridge functions, which we must
+ // ignore.
+ .filter(m -> !Modifier.isPrivate(m.getModifiers()))
+ .filter(m -> !Modifier.isProtected(m.getModifiers()))
+ .filter(m -> !Modifier.isStatic(m.getModifiers()))
+ .forEach(methods::add);
+ c = c.getSuperclass();
Review Comment:
Why this functionality change to get all methods in the superclass chain?
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/JavaBeanSchemaTest.java:
##########
@@ -625,4 +626,41 @@ public void testSetterConstructionWithRenamedFields() throws NoSuchSchemaExcepti
assertEquals(
registry.getFromRowFunction(BeanWithCaseFormat.class).apply(row), beanWithCaseFormat);
}
+
+ @Test
+ public void testRegisterBeamWithTypeParameter() throws NoSuchSchemaException {
Review Comment:
nit:
```suggestion
public void testRegisterBeanWithTypeParameter() throws NoSuchSchemaException {
```
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/FieldValueTypeInformation.java:
##########
@@ -125,18 +126,20 @@ public static FieldValueTypeInformation forOneOf(
.build();
}
- public static FieldValueTypeInformation forField(Field field, int index) {
- TypeDescriptor<?> type = TypeDescriptor.of(field.getGenericType());
+ public static FieldValueTypeInformation forField(
+ Field field, int index, Map<Type, Type> boundTypes) {
Review Comment:
Same with some other changes to method signatures in this class: `forGetter()` and `forSetter()`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/FieldValueTypeInformation.java:
##########
@@ -125,18 +126,20 @@ public static FieldValueTypeInformation forOneOf(
.build();
}
- public static FieldValueTypeInformation forField(Field field, int index) {
- TypeDescriptor<?> type = TypeDescriptor.of(field.getGenericType());
+ public static FieldValueTypeInformation forField(
+ Field field, int index, Map<Type, Type> boundTypes) {
Review Comment:
Note that this is a breaking change to a public method (not marked with `@Internal`). Are we okay with that?
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/POJOUtils.java:
##########
@@ -73,8 +73,11 @@
public class POJOUtils {
public static Schema schemaFromPojoClass(
- TypeDescriptor<?> typeDescriptor, FieldValueTypeSupplier fieldValueTypeSupplier) {
- return StaticSchemaInference.schemaFromClass(typeDescriptor, fieldValueTypeSupplier);
+ TypeDescriptor<?> typeDescriptor,
+ FieldValueTypeSupplier fieldValueTypeSupplier,
+ Map<Type, Type> boundTypes) {
Review Comment:
Note: breaking change to a public method's signature
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/JavaBeanUtils.java:
##########
@@ -64,8 +65,11 @@
public class JavaBeanUtils {
/** Create a {@link Schema} for a Java Bean class. */
public static Schema schemaFromJavaBeanClass(
- TypeDescriptor<?> typeDescriptor, FieldValueTypeSupplier fieldValueTypeSupplier) {
- return StaticSchemaInference.schemaFromClass(typeDescriptor, fieldValueTypeSupplier);
+ TypeDescriptor<?> typeDescriptor,
+ FieldValueTypeSupplier fieldValueTypeSupplier,
+ Map<Type, Type> boundTypes) {
Review Comment:
Another breaking method signature change
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/StaticSchemaInference.java:
##########
@@ -85,14 +86,17 @@ enum MethodType {
* public getter methods, or special annotations on the class.
*/
public static Schema schemaFromClass(
- TypeDescriptor<?> typeDescriptor, FieldValueTypeSupplier fieldValueTypeSupplier) {
- return schemaFromClass(typeDescriptor, fieldValueTypeSupplier, new HashMap<>());
+ TypeDescriptor<?> typeDescriptor,
+ FieldValueTypeSupplier fieldValueTypeSupplier,
+ Map<Type, Type> boundTypes) {
Review Comment:
Another breaking method signature change
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/JavaBeanSchemaTest.java:
##########
@@ -625,4 +626,41 @@ public void testSetterConstructionWithRenamedFields() throws NoSuchSchemaExcepti
assertEquals(
registry.getFromRowFunction(BeanWithCaseFormat.class).apply(row), beanWithCaseFormat);
}
+
+ @Test
+ public void testRegisterBeamWithTypeParameter() throws NoSuchSchemaException {
+ SchemaRegistry registry = SchemaRegistry.createDefault();
+ TypeDescriptor<TestJavaBeans.SimpleParameterizedBean<String, Long, Boolean, SimpleBean>>
+ typeDescriptor =
+ new TypeDescriptor<
+ TestJavaBeans.SimpleParameterizedBean<String, Long, Boolean, SimpleBean>>() {};
+ Schema schema = registry.getSchema(typeDescriptor);
+
+ final Schema expectedSchema =
+ Schema.builder()
+ .addBooleanField("value1")
+ .addStringField("value2")
+ .addInt64Field("value3")
+ .addRowField("value4", SIMPLE_BEAN_SCHEMA)
+ .build();
+ assertTrue(expectedSchema.equivalent(schema));
+ }
+
+ @Test
+ public void testRegisterBeanWithInheritedTypeParameter() throws NoSuchSchemaException {
+ SchemaRegistry registry = SchemaRegistry.createDefault();
+ TypeDescriptor<TestJavaBeans.SimpleParameterizedBeanSubclass<Short>> typeDescriptor =
+ new TypeDescriptor<TestJavaBeans.SimpleParameterizedBeanSubclass<Short>>() {};
+ Schema schema = registry.getSchema(typeDescriptor);
+
+ final Schema expectedSchema =
+ Schema.builder()
+ .addBooleanField("value1")
+ .addStringField("value2")
+ .addInt64Field("value3")
+ .addRowField("value4", SIMPLE_BEAN_SCHEMA)
+ .addInt16Field("value5")
+ .build();
+ assertTrue(expectedSchema.equivalent(schema));
+ }
Review Comment:
add tests for collection, map, nested types?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema inference parameterized types [beam]
Posted by "reuvenlax (via GitHub)" <gi...@apache.org>.
reuvenlax merged PR #32757:
URL: https://github.com/apache/beam/pull/32757
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema inference parameterized types [beam]
Posted by "reuvenlax (via GitHub)" <gi...@apache.org>.
reuvenlax commented on code in PR #32757:
URL: https://github.com/apache/beam/pull/32757#discussion_r1799844572
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/JavaBeanSchemaTest.java:
##########
@@ -625,4 +626,41 @@ public void testSetterConstructionWithRenamedFields() throws NoSuchSchemaExcepti
assertEquals(
registry.getFromRowFunction(BeanWithCaseFormat.class).apply(row), beanWithCaseFormat);
}
+
+ @Test
+ public void testRegisterBeamWithTypeParameter() throws NoSuchSchemaException {
+ SchemaRegistry registry = SchemaRegistry.createDefault();
+ TypeDescriptor<TestJavaBeans.SimpleParameterizedBean<String, Long, Boolean, SimpleBean>>
+ typeDescriptor =
+ new TypeDescriptor<
+ TestJavaBeans.SimpleParameterizedBean<String, Long, Boolean, SimpleBean>>() {};
+ Schema schema = registry.getSchema(typeDescriptor);
+
+ final Schema expectedSchema =
+ Schema.builder()
+ .addBooleanField("value1")
+ .addStringField("value2")
+ .addInt64Field("value3")
+ .addRowField("value4", SIMPLE_BEAN_SCHEMA)
+ .build();
+ assertTrue(expectedSchema.equivalent(schema));
+ }
+
+ @Test
+ public void testRegisterBeanWithInheritedTypeParameter() throws NoSuchSchemaException {
+ SchemaRegistry registry = SchemaRegistry.createDefault();
+ TypeDescriptor<TestJavaBeans.SimpleParameterizedBeanSubclass<Short>> typeDescriptor =
+ new TypeDescriptor<TestJavaBeans.SimpleParameterizedBeanSubclass<Short>>() {};
+ Schema schema = registry.getSchema(typeDescriptor);
+
+ final Schema expectedSchema =
+ Schema.builder()
+ .addBooleanField("value1")
+ .addStringField("value2")
+ .addInt64Field("value3")
+ .addRowField("value4", SIMPLE_BEAN_SCHEMA)
+ .addInt16Field("value5")
+ .build();
+ assertTrue(expectedSchema.equivalent(schema));
+ }
Review Comment:
Added
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/JavaBeanSchemaTest.java:
##########
@@ -625,4 +626,41 @@ public void testSetterConstructionWithRenamedFields() throws NoSuchSchemaExcepti
assertEquals(
registry.getFromRowFunction(BeanWithCaseFormat.class).apply(row), beanWithCaseFormat);
}
+
+ @Test
+ public void testRegisterBeamWithTypeParameter() throws NoSuchSchemaException {
Review Comment:
Done
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema inference parameterized types [beam]
Posted by "reuvenlax (via GitHub)" <gi...@apache.org>.
reuvenlax commented on PR #32757:
URL: https://github.com/apache/beam/pull/32757#issuecomment-2418324025
The fix has been submitted - is that Yaml test still failing?
Separately we should figure out why none of those tests triggered on my
initial PR :(
On Wed, Oct 16, 2024 at 6:00 PM Kenn Knowles ***@***.***>
wrote:
> Suspected culprit of breaking this one too
> https://github.com/apache/beam/actions/workflows/beam_PreCommit_Yaml_Xlang_Direct.yml?query=branch%3Amaster+is%3Acompleted
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/beam/pull/32757#issuecomment-2418271581>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AFAYJVJWYRWOHOV74QY6PS3Z34D4HAVCNFSM6AAAAABPZOTUP2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJYGI3TCNJYGE>
> .
> You are receiving this because you modified the open/close state.Message
> ID: ***@***.***>
>
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema inference parameterized types [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #32757:
URL: https://github.com/apache/beam/pull/32757#issuecomment-2419090244
Created an issue for the failing YAML tests: #32832
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org