[GitHub] flink pull request: [FLINK-1799][scala] Fix handling of generic ar...
Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/582 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1799][scala] Fix handling of generic ar...
Github user aljoscha commented on the pull request: https://github.com/apache/flink/pull/582#issuecomment-94799103 So, any thoughts about merging this? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1799][scala] Fix handling of generic ar...
Github user StephanEwen commented on the pull request: https://github.com/apache/flink/pull/582#issuecomment-94919453 Yo, looks fine now --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1799][scala] Fix handling of generic ar...
Github user aljoscha commented on the pull request: https://github.com/apache/flink/pull/582#issuecomment-94392757 Any more thoughts? Otherwise I would like to merge this. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1799][scala] Fix handling of generic ar...
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/582#discussion_r28691764
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/typeutils/ObjectArrayTypeInfo.java
---
@@ -143,6 +143,18 @@ else if (type instanceof Class? ((Class?)
type).isArray()
throw new InvalidTypesException(The given type is not a valid
object array.);
}
+ /**
+* Creates a new {@link
org.apache.flink.api.java.typeutils.ObjectArrayTypeInfo} from a
+* {@link TypeInformation} for the component type.
+*
+* p
+* This must be used in cases where the complete type of the array is
not available as a
+* {@link java.lang.reflect.Type} or {@link java.lang.Class}.
+*/
+ public static T, C ObjectArrayTypeInfoT, C
getInfoFor(TypeInformationC componentInfo) {
+ return new ObjectArrayTypeInfoT, C(Object[].class,
componentInfo.getTypeClass(), componentInfo);
--- End diff --
Is `Object[].class` actually correct? Why not give the class of the real
array type? There are a few ways to get that, for example
`Array.newInstance(clazz, 0).getClass();`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1799][scala] Fix handling of generic ar...
Github user aljoscha commented on a diff in the pull request:
https://github.com/apache/flink/pull/582#discussion_r28692599
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/typeutils/ObjectArrayTypeInfo.java
---
@@ -143,6 +143,18 @@ else if (type instanceof Class? ((Class?)
type).isArray()
throw new InvalidTypesException(The given type is not a valid
object array.);
}
+ /**
+* Creates a new {@link
org.apache.flink.api.java.typeutils.ObjectArrayTypeInfo} from a
+* {@link TypeInformation} for the component type.
+*
+* p
+* This must be used in cases where the complete type of the array is
not available as a
+* {@link java.lang.reflect.Type} or {@link java.lang.Class}.
+*/
+ public static T, C ObjectArrayTypeInfoT, C
getInfoFor(TypeInformationC componentInfo) {
+ return new ObjectArrayTypeInfoT, C(Object[].class,
componentInfo.getTypeClass(), componentInfo);
--- End diff --
That's right, I didn't think of that. Will change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1799][scala] Fix handling of generic ar...
Github user aljoscha commented on the pull request: https://github.com/apache/flink/pull/582#issuecomment-94483743 I fixed @StephanEwen's complaint. It was incorrect but the type class of TypeInformation does not seem to be used in any places where it matters. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1799][scala] Fix handling of generic ar...
Github user tillrohrmann commented on the pull request: https://github.com/apache/flink/pull/582#issuecomment-93929395 Maybe we could add in the test case the case where we have an ```Object``` array. Other than that LGTM. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1799][scala] Fix handling of generic ar...
Github user aljoscha commented on the pull request: https://github.com/apache/flink/pull/582#issuecomment-93980529 I added the test case @tillrohrmann suggested. Also helped me find a bug. :relieved: --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1799][scala] Fix handling of generic ar...
Github user aljoscha commented on the pull request: https://github.com/apache/flink/pull/582#issuecomment-92381429 Any more comments? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1799][scala] Fix handling of generic ar...
GitHub user aljoscha opened a pull request: https://github.com/apache/flink/pull/582 [FLINK-1799][scala] Fix handling of generic arrays You can merge this pull request into a Git repository by running: $ git pull https://github.com/aljoscha/flink fix-scala-generic-arrays Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/582.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #582 commit ac1b4111704933169f46515caffbe2607d263677 Author: Aljoscha Krettek aljoscha.kret...@gmail.com Date: 2015-04-09T12:05:08Z [FLINK-1799][scala] Fix handling of generic arrays --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-1799][scala] Fix handling of generic ar...
Github user StephanEwen commented on the pull request: https://github.com/apache/flink/pull/582#issuecomment-91327192 Instead of the switch/case statement, can't you use `PrimitiveArrayTypeInfo.getInfoFor(ClassX type)`? Seems simpler. Also, this is a good change to be backed by a Unit test case, rather than an Integration test case. I really think that such changes should be guarded by more fine grained tests (Unit Tests(, rather than the big hammer kind of test (have a distributed program that uses this). Out testing time goes through the roof otherwise. I fund that unit tests are a cycle or two more work to write, but typically also get you thinking better about what cases the tests should cover. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
