[FLINK-4673] [core] TypeInfoFactory for Either type #2545
Conversation
Removes from TypeExtractor the explicit parsing for Either and adds an EitherTypeInfoFactory.
|
I will review this PR soon. |
There was a problem hiding this comment.
Thanks for the PR @greghogan. There are some things that need to be fixed so that we have the same behavior again.
|
|
||
| @Override | ||
| public TypeInformation<Either<L, R>> createTypeInfo(Type t, Map<String, TypeInformation<?>> genericParameters) { | ||
| return new EitherTypeInfo(genericParameters.get("L"), genericParameters.get("R")); |
There was a problem hiding this comment.
Can check here if the key is not null? Btw. we should add a null check to the constructor of EitherTypeInfo.
| // type needs to be treated a pojo due to additional fields | ||
| if (subTypesInfo == null) { | ||
| if (t instanceof ParameterizedType) { | ||
| return (TypeInformation<OUT>) analyzePojo(typeToClass(t), new ArrayList<Type>(typeHierarchy), (ParameterizedType) t, in1Type, in2Type); |
There was a problem hiding this comment.
What happens if Either class is extended? The whole checking is missing when using a factory. It need to go into the createTypeInfo method. What I just recognized, actually the EitherTypeInfo needs a second constructor that takes the subclass similar to TupleTypeInfo. The t parameter of createTypeInfo can than passed to this constructor.
There was a problem hiding this comment.
Either is overridden in TypeExtractorTest without any test errors. Could you give an example that would break the current PR? @twalthr
There was a problem hiding this comment.
Sorry, for the delay. I will have a look at it again.
There was a problem hiding this comment.
Hi @twalthr just a gentle reminder for when you have some time to look at this.
There was a problem hiding this comment.
Sorry for not writing back earlier. You are right, all tests still work. The factories work better than I expected. We lose the input validation in this PR but I think this is ok. I will merge this.
There was a problem hiding this comment.
@twalthr thanks for checking this! Glad to hear that your factories implementation has exceeded expectations :)
| @Test(expected=InvalidTypesException.class) | ||
| public void testEitherFromObjectException() { | ||
| Either<String, Tuple1<Integer>> either = Either.Left("test"); | ||
| TypeExtractor.getForObject(either); |
There was a problem hiding this comment.
This test is necessary. The exception is not thrown anymore but the type information that is currently generated is invalid (the right type information is null, which should never happen).
greghogan
force-pushed
the
4673_typeinfofactory_for_either_type
branch
from
869245d
to
80e807d
Compare
Removes from TypeExtractor the explicit parsing for Either and adds an EitherTypeInfoFactory. This closes apache#2545.
Removes from TypeExtractor the explicit parsing for Either and adds an EitherTypeInfoFactory. This closes apache#2545.
Removes from TypeExtractor the explicit parsing for Either and adds an EitherTypeInfoFactory.
A test was removed that was expected to fail but now looks to succeed.