flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Hogan <c...@greghogan.com>
Subject Re: Eclipse Problems
Date Thu, 28 Apr 2016 13:42:47 GMT
Matthias,

Won't this be a compile-time error as long as the user is parameterizing
the return type since .fromElements(OUT...) returns DataStreamSource<OUT>
and will bind to the nearest common superclass? The new
.fromElements(Class<OUT>, OUT...) does give the user the choice of common
superclass.

Greg


On Wed, Apr 27, 2016 at 10:25 AM, Matthias J. Sax <mjsax@apache.org> wrote:

> I guess, removing .fromElements(Object..) would fix the problem. Not
> sure so, if we can remove the method due to API stability...
>
> I don't see any other good solution (even if the current implementation
> gives a nice behavior by accident...):
>
> If you have a complex class hierarchy, it would be quite complex to find
> out the correct common sub-type. Using only .fromElemenst(Class<X>,
> X...) requires to specify the correct sub-type and has the additional
> advantage, the the compiler can check the type already (instead of a
> potential later runtime error).
>
>
> -Matthias
>
>
> On 04/27/2016 03:07 PM, Till Rohrmann wrote:
> > You’re completely right Mathias. The compiler shouldn’t allow something
> > like env.fromElements(SubClass.class, new ParentClass()) if it weren’t
> for
> > the overloaded method. Thus, the test case is somewhat bogus.
> >
> > I’m actually wondering why the initial problem
> > https://issues.apache.org/jira/browse/FLINK-3444 was solved this way. I
> > think it would be better to automatically infer the common super type of
> > all provided elements. Otherwise, you run into problems you’ve found out
> > about.
> >
> > Consequently, I think it is fine if you remove the
> fromElementsWithBaseType2
> > test case.
> >
> > Cheers,
> > Till
> > ​
> >
> > On Wed, Apr 27, 2016 at 1:22 PM, Matthias J. Sax <mjsax@apache.org>
> wrote:
> >
> >> Hi Till,
> >>
> >> but StreamExecutionEnvironmentTest.fromElementWithBaseTypeTest2 does not
> >> test was you describe -- even if it is intended to test it.
> >>
> >> It would test your describe scenario, if fromElements(Class<X>, X...)
> >> would be called, But this call is not possible because X is defined a
> >> type Subclass and thus the provided object of Parentclass cannot be
> >> handed over as type X. Therefore, fromElements(Object...) is called: of
> >> course, this fails too, because now the type is derived as
> >> Class<Subclass> (and not Subclass) and neither Subclass nor Parentclass
> >> inherit from Class<Subclass>.
> >>
> >> The scenario you describe will never work -- if you remove the overload
> >> fromElements(Object...) the code would not even compile as the compiler
> >> can figure out from the generics that the call
> >> fromElments(Subclass.class, new Parentclass()) is invalid.
> >>
> >> It is only possible to hand in "reverse inheritance types" for
> >> fromElemenst(Object...). In this case, the first given Object defines
> >> the type. Thus, if you call fromElements(new Subclass(), new
> >> Parentclass()), the call will fail, as Parentclass is no subtype of
> >> Subtype -- the call fromElements(new Parentclass() new Subclass()) would
> >> succeed.
> >>
> >> Makes sense?
> >>
> >> Still no idea how to make it compile in Eclipse...
> >>
> >> -Matthias
> >>
> >> On 04/27/2016 10:21 AM, Till Rohrmann wrote:
> >>> Thanks for looking into this problem Mathias. I think the Scala test
> >> should
> >>> be fixed as you've proposed.
> >>>
> >>> Concerning the
> >> StreamExecutionEnvironmentTest.fromElementWithBaseTypeTest2,
> >>> I think it shouldn't be changed. The reason is that the class defines
> the
> >>> common base class of the elements. And the test makes sure that the
> >>> fromElements call fails if you provide instances which are not of the
> >>> specified type or a subclass of it. Thus, we should find another way to
> >>> make it work with Eclipse.
> >>>
> >>> Cheers,
> >>> Till
> >>>
> >>> On Tue, Apr 26, 2016 at 9:41 PM, Matthias J. Sax <mjsax@apache.org>
> >> wrote:
> >>>
> >>>> Even if the fix works, I still have two issues in my Eclipse build...
> >>>>
> >>>> In
> >>>>
> >>>>
> >>>>
> >>
> flink-scala/src/test/scala/org/apache/flink/api/scala/extensions/base/AcceptPFTestBase.scala
> >>>>
> >>>> Eclipse cannot infer the integer type. It could be fixed if you make
> the
> >>>> type explicit (as this is only a test, it might be nice to fix this
--
> >>>> let me know if I can push this or not)
> >>>>
> >>>>> diff --git
> >>>>
> >>
> a/flink-scala/src/test/scala/org/apache/flink/api/scala/extensions/base/AcceptPFTestBase.scala
> >>>>
> >>
> b/flink-scala/src/test/scala/org/apache/flink/api/scala/extensions/base/AcceptPFTestBase.scala
> >>>>> index c2e13fe..f9ce3b8 100644
> >>>>> ---
> >>>>
> >>
> a/flink-scala/src/test/scala/org/apache/flink/api/scala/extensions/base/AcceptPFTestBase.scala
> >>>>> +++
> >>>>
> >>
> b/flink-scala/src/test/scala/org/apache/flink/api/scala/extensions/base/AcceptPFTestBase.scala
> >>>>> @@ -29,7 +29,7 @@ private[extensions] abstract class AcceptPFTestBase
> >>>> extends TestLogger with JUni
> >>>>>
> >>>>>    private val env = ExecutionEnvironment.getExecutionEnvironment
> >>>>>
> >>>>> -  protected val tuples = env.fromElements(1 -> "hello", 2 ->
> "world")
> >>>>> +  protected val tuples = env.fromElements(new Integer(1) ->
"hello",
> >>>> new Integer(2) -> "world")
> >>>>>    protected val caseObjects = env.fromElements(KeyValuePair(1,
> >>>> "hello"), KeyValuePair(2, "world"))
> >>>>>
> >>>>>    protected val groupedTuples = tuples.groupBy(_._1)
> >>>>
> >>>> Furthermore, in
> >>>>
> >>>>
> >>
> flink-java/src/test/java/org/apache/flink/api/java/io/FromElementsTest.java
> >>>>
> >>>>> @Test
> >>>>> public void fromElementsWithBaseTypeTest1() {
> >>>>>       ExecutionEnvironment executionEnvironment =
> >>>> ExecutionEnvironment.getExecutionEnvironment();
> >>>>>       executionEnvironment.fromElements(ParentType.class, new
> >> SubType(1,
> >>>> "Java"), new ParentType(1, "hello"));
> >>>>> }
> >>>>
> >>>> and in
> >>>>
> >>>>
> >>>>
> >>
> flink-streaming-java/src/test/java/org/apache/flink/streaming/api/StreamExecutionEnvironmentTest.java
> >>>>
> >>>>> @Test
> >>>>> public void fromElementsWithBaseTypeTest1() {
> >>>>>       StreamExecutionEnvironment env =
> >>>> StreamExecutionEnvironment.getExecutionEnvironment();
> >>>>>       env.fromElements(ParentClass.class, new SubClass(1, "Java"),
> new
> >>>> ParentClass(1, "hello"));
> >>>>> }
> >>>>
> >>>> In both cases, I get the error:
> >>>>
> >>>>   The method .fromElements(Object[]) is ambiguous
> >>>>
> >>>> No clue how to fix this, and why Eclipse does not bind to
> >>>> .fromElements(Class<X>, X). Any ideas?
> >>>>
> >>>> I also digger a little bit and for both test-classes there is a second
> >>>> test method called "fromElementsWithBaseTypeTest2". If I understand
> this
> >>>> test correctly, it also tries to bind to .fromElements(Class<X>,
X),
> but
> >>>> this does not happen and .fromElemenst(Object[]) is called. Even if
> >>>> there is still an exception, I got the impression that this test does
> >>>> not what the intention was.
> >>>>
> >>>> If might be good to change fromElementsWithBaseTypeTest2 to
> >>>>
> >>>>> env.fromElements(new SubClass(1, "Java"), new ParentClass(1,
> "hello"));
> >>>>
> >>>> (ie, remove the first Class parameter). Any comments on this?
> >>>>
> >>>> -Matthias
> >>>>
> >>>>
> >>>>
> >>>> On 04/25/2016 01:42 PM, Robert Metzger wrote:
> >>>>> Cool, thank you for working on this!
> >>>>>
> >>>>> On Mon, Apr 25, 2016 at 1:37 PM, Matthias J. Sax <mjsax@apache.org>
> >>>> wrote:
> >>>>>
> >>>>>> I can confirm that the SO answer works.
> >>>>>>
> >>>>>> I will add a note to the Eclipse setup guide at the web site.
> >>>>>>
> >>>>>> -Matthias
> >>>>>>
> >>>>>>
> >>>>>> On 04/25/2016 11:33 AM, Robert Metzger wrote:
> >>>>>>> It seems that the user resolved the issue on SO, right?
> >>>>>>>
> >>>>>>> On Mon, Apr 25, 2016 at 11:31 AM, Ufuk Celebi <uce@apache.org>
> >> wrote:
> >>>>>>>
> >>>>>>>> On Mon, Apr 25, 2016 at 12:14 AM, Matthias J. Sax <
> mjsax@apache.org
> >>>
> >>>>>>>> wrote:
> >>>>>>>>> What do you think about this?
> >>>>>>>>
> >>>>>>>> Hey Matthias!
> >>>>>>>>
> >>>>>>>> Thanks for bringing this up.
> >>>>>>>>
> >>>>>>>> I think it is very desirable to keep support for Eclipse.
It's
> >> quite a
> >>>>>>>> high barrier for new contributors to enforce a specific
IDE
> >> (although
> >>>>>>>> IntelliJ is gaining quite the user base I think :P).
> >>>>>>>>
> >>>>>>>> Do you have time to look into this?
> >>>>>>>>
> >>>>>>>> – Ufuk
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>>
> >>>
> >>
> >>
> >
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message