Skip to content
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.

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

Jump to bottom

[FLINK-6909] [FLINK-7450] [types] Improve the handling of POJOs and clean-up type extraction #5097

Closed
wants to merge 1 commit into from
Closed

[FLINK-6909] [FLINK-7450] [types] Improve the handling of POJOs and clean-up type extraction #5097

wants to merge 1 commit into from

Conversation

twalthr
Copy link
Contributor

@twalthr twalthr commented Nov 29, 2017

Contribution Checklist

This PR solves a variety of issues that are related to the type extraction of POJOs and tuples.

What is the purpose of the change

Make the type extraction more stable and help users with more detailed exceptions.

Brief change log

  • Until now, subclasses of tuples where not properly checked for additional fields and serializability: Non-static subclasses of tuples or classes with no default constructor were valid types.
  • Even existing tests and Gelly classes where not implemented correctly.
  • I fixed bugs related to bounded generic fields in POJOs.
  • The type extractor has been refactored and simplified in order to have more consistent behavior. E.g., getForClass was unable to determine subclasses of tuples.
  • Type extraction tests have been refactored to remove all warnings that were present.
  • I tested generated Lombok POJOs.
  • Class cast execeptions in CSV reader have been fixed as well.
  • I added a utility method PojoTypeInfo.ensurePojo(MyPojo.class) that validates if a type is a POJO and throws an exception with reasons why the given field is no POJO. I think this can help users a lot to avoid common mistakes.

Verifying this change

Tests have been added to TypeExtractorTest, PojoTypeExtractionTest. Existing tests still run.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): yes
  • The public API, i.e., is any changed class annotated with @Public(Evolving): yes
  • The serializers: no
  • The runtime per-record code paths (performance sensitive): no
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable

aljoscha
aljoscha requested changes Dec 11, 2017
View reviewed changes
Copy link
Contributor

@aljoscha aljoscha left a comment

Choose a reason for hiding this comment

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

I think these changes are very good. They're a bit hard to tweeze apart, though, because the refactorings/fixups are mixed up with the actual bugs that are being fixed. I don't see in the changes what fixes the Lombock problem, for example.

Could you maybe pull this apart into a commit that is just cosmetics and cleanups and a commit that does the actual fixes and fixes/adds tests?

flink-core/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractionUtils.java
Modifier.isFinal(field.getModifiers())) {
continue; // we have no use for transient, static, or final fields
}
if (hasFieldWithSameName(field.getName(), result)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this simply result.stream().anyMatch(f -> f.getName().equals(field.getName()))?

flink-core/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractor.java
private boolean isValidPojoField(Field f, Class<?> clazz, ArrayList<Type> typeHierarchy) {
if(Modifier.isPublic(f.getModifiers())) {
return true;
private static Optional<String> analyzePojoField(Field f, Class<?> clazz, ArrayList<Type> typeHierarchy) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be an Either to be in line with other methods in this class. An Optional that actually contains something in case of failure seems a bit strange.

flink-core/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractor.java
* @param clazz class to be analyzed
* @param allowAbstract if true, allows abstract classes but prints an info to the log
*/
private static Optional<String> checkSerialization(Class<?> clazz, boolean allowAbstract) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@twalthr
Copy link
Contributor Author

twalthr commented May 17, 2018

Thanks for the review @aljoscha. Yes, I agree that this PR is a bit messy. I will close it for now and open PR with a finer granularity after rebasing.

@twalthr twalthr closed this May 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aljoscha aljoscha aljoscha requested changes

Assignees
No one assigned
Labels
component=API/TypeSerializationSystem
Projects
None yet
Milestone
No milestone
3 participants
@twalthr @aljoscha @rmetzger