[DISCUSS] Convert main Table API classes into traits

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[DISCUSS] Convert main Table API classes into traits

Timo Walther-2
Hi everyone,

I'm currently thinking about how to implement FLINK-8606. The reason
behind it is that Java users are able to see all variables and methods
that are declared 'private[flink]' or even 'protected' in Scala. Classes
such as TableEnvironment look very messy from the outside in Java. Since
we cannot change the visibility of Scala protected members, I was
thinking about a bigger change to solve this issue once and for all. My
idea is to convert all TableEnvironment classes and maybe the Table
class into traits. The actual implementation would end up in some
internal classes such as "InternalTableEnvironment" that implement the
public traits. The goal would be to stay source code compatible.

What do you think?

Regards,
Timo

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Convert main Table API classes into traits

Rong Rong
Hi Timo,

Thanks for looking into this Timo. It's becoming increasingly messy for my
trying to locate the correct functions in IDE :-/

This is probably due to the fact that Scala and Java access modifiers /
qualifiers are subtly and fundamentally different. Using Trait might be the
best solution here. Another way I can think of is to move the all
TableEnvironment classes to Java side, but that would probably introduce a
lot of issue we need to resolve on the Scala side though. "protected" is
less restrictive in Java but there's really no equivalent of package
private modifier on Java.

I was wondering is there any better way to provide backward-compatible
support though. I played around with it and seems like every "protected"
field will create a private Java member and a public getter, should we add
them all and annotate with "@Deprecated" ?
--
Rong

On Thu, Mar 1, 2018 at 10:58 AM, Timo Walther <[hidden email]> wrote:

> Hi everyone,
>
> I'm currently thinking about how to implement FLINK-8606. The reason
> behind it is that Java users are able to see all variables and methods that
> are declared 'private[flink]' or even 'protected' in Scala. Classes such as
> TableEnvironment look very messy from the outside in Java. Since we cannot
> change the visibility of Scala protected members, I was thinking about a
> bigger change to solve this issue once and for all. My idea is to convert
> all TableEnvironment classes and maybe the Table class into traits. The
> actual implementation would end up in some internal classes such as
> "InternalTableEnvironment" that implement the public traits. The goal would
> be to stay source code compatible.
>
> What do you think?
>
> Regards,
> Timo
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Convert main Table API classes into traits

Shuyi Chen
Hi Timo,

I am throwing some second thoughts here, as I don't quite see what trait
provides over abstract class here for TableEnvironment case. Trait in scala
can also have implementation and you can have 'private[flink]' or
'protected'  type and method in trait as well.

AFAIK, the differences between Scala trait and abstract class are:
1) you can have constructor for abstract class, but not in trait
2) Abstract classes are fully interoperable with Java. You can call them
from Java code without any wrappers. Traits are fully interoperable only if
they do not contain any implementation code for scala 2.11.
3) you can do multiple inheritance or mixin composition with trait.

In the TableEnvironment case,
1) I don't see a need for mixin, and class hierarchy seems fit better here
by design.
2) to better interoperate with Java from scala 2.11, it's better to use
abstract class. (But AFAIK, scala 2.12 and java 8 would be compatible,
though)
3) you might pay a bit performance overhead with trait (compiled to
interface) compared to abstract class, but it's not a big deal here.

But in other cases, trait might be a better one if it might be reused and
mixined in multiple, unrelated classes.

So another option would be to refactor TableEnvironment to clean up or move
the 'private[flink]' or 'protected' stuff to the actual implementor
(e.g. 'InternalTableEnvironment') as
you would do for your trait approach for TableEnvironment. I think this
option might help with backward compatibility as well. Thanks.

Shuyi

On Fri, Mar 2, 2018 at 10:25 AM, Rong Rong <[hidden email]> wrote:

> Hi Timo,
>
> Thanks for looking into this Timo. It's becoming increasingly messy for my
> trying to locate the correct functions in IDE :-/
>
> This is probably due to the fact that Scala and Java access modifiers /
> qualifiers are subtly and fundamentally different. Using Trait might be the
> best solution here. Another way I can think of is to move the all
> TableEnvironment classes to Java side, but that would probably introduce a
> lot of issue we need to resolve on the Scala side though. "protected" is
> less restrictive in Java but there's really no equivalent of package
> private modifier on Java.
>
> I was wondering is there any better way to provide backward-compatible
> support though. I played around with it and seems like every "protected"
> field will create a private Java member and a public getter, should we add
> them all and annotate with "@Deprecated" ?
> --
> Rong
>
> On Thu, Mar 1, 2018 at 10:58 AM, Timo Walther <[hidden email]> wrote:
>
> > Hi everyone,
> >
> > I'm currently thinking about how to implement FLINK-8606. The reason
> > behind it is that Java users are able to see all variables and methods
> that
> > are declared 'private[flink]' or even 'protected' in Scala. Classes such
> as
> > TableEnvironment look very messy from the outside in Java. Since we
> cannot
> > change the visibility of Scala protected members, I was thinking about a
> > bigger change to solve this issue once and for all. My idea is to convert
> > all TableEnvironment classes and maybe the Table class into traits. The
> > actual implementation would end up in some internal classes such as
> > "InternalTableEnvironment" that implement the public traits. The goal
> would
> > be to stay source code compatible.
> >
> > What do you think?
> >
> > Regards,
> > Timo
> >
> >
>



--
"So you have to trust that the dots will somehow connect in your future."
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Convert main Table API classes into traits

Rong Rong
Hi Shuyi,

I am already assuming all package private and protected modifiers will be
cleaned up and  moved to internal implementation. Please correct me if I
were wrong, Timo.

Thanks,
Rong


On Fri, Mar 2, 2018, 5:02 PM Shuyi Chen <[hidden email]> wrote:

> Hi Timo,
>
> I am throwing some second thoughts here, as I don't quite see what trait
> provides over abstract class here for TableEnvironment case. Trait in scala
> can also have implementation and you can have 'private[flink]' or
> 'protected'  type and method in trait as well.
>
> AFAIK, the differences between Scala trait and abstract class are:
> 1) you can have constructor for abstract class, but not in trait
> 2) Abstract classes are fully interoperable with Java. You can call them
> from Java code without any wrappers. Traits are fully interoperable only if
> they do not contain any implementation code for scala 2.11.
> 3) you can do multiple inheritance or mixin composition with trait.
>
> In the TableEnvironment case,
> 1) I don't see a need for mixin, and class hierarchy seems fit better here
> by design.
> 2) to better interoperate with Java from scala 2.11, it's better to use
> abstract class. (But AFAIK, scala 2.12 and java 8 would be compatible,
> though)
> 3) you might pay a bit performance overhead with trait (compiled to
> interface) compared to abstract class, but it's not a big deal here.
>
> But in other cases, trait might be a better one if it might be reused and
> mixined in multiple, unrelated classes.
>
> So another option would be to refactor TableEnvironment to clean up or move
> the 'private[flink]' or 'protected' stuff to the actual implementor
> (e.g. 'InternalTableEnvironment') as
> you would do for your trait approach for TableEnvironment. I think this
> option might help with backward compatibility as well. Thanks.
>
> Shuyi
>
> On Fri, Mar 2, 2018 at 10:25 AM, Rong Rong <[hidden email]> wrote:
>
> > Hi Timo,
> >
> > Thanks for looking into this Timo. It's becoming increasingly messy for
> my
> > trying to locate the correct functions in IDE :-/
> >
> > This is probably due to the fact that Scala and Java access modifiers /
> > qualifiers are subtly and fundamentally different. Using Trait might be
> the
> > best solution here. Another way I can think of is to move the all
> > TableEnvironment classes to Java side, but that would probably introduce
> a
> > lot of issue we need to resolve on the Scala side though. "protected" is
> > less restrictive in Java but there's really no equivalent of package
> > private modifier on Java.
> >
> > I was wondering is there any better way to provide backward-compatible
> > support though. I played around with it and seems like every "protected"
> > field will create a private Java member and a public getter, should we
> add
> > them all and annotate with "@Deprecated" ?
> > --
> > Rong
> >
> > On Thu, Mar 1, 2018 at 10:58 AM, Timo Walther <[hidden email]>
> wrote:
> >
> > > Hi everyone,
> > >
> > > I'm currently thinking about how to implement FLINK-8606. The reason
> > > behind it is that Java users are able to see all variables and methods
> > that
> > > are declared 'private[flink]' or even 'protected' in Scala. Classes
> such
> > as
> > > TableEnvironment look very messy from the outside in Java. Since we
> > cannot
> > > change the visibility of Scala protected members, I was thinking about
> a
> > > bigger change to solve this issue once and for all. My idea is to
> convert
> > > all TableEnvironment classes and maybe the Table class into traits. The
> > > actual implementation would end up in some internal classes such as
> > > "InternalTableEnvironment" that implement the public traits. The goal
> > would
> > > be to stay source code compatible.
> > >
> > > What do you think?
> > >
> > > Regards,
> > > Timo
> > >
> > >
> >
>
>
>
> --
> "So you have to trust that the dots will somehow connect in your future."
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Convert main Table API classes into traits

Timo Walther-2
Hi everyone,

@Rong: Yes, that's what I meant. Package private and protected modifiers
and fields will be moved to internal implementation.

The hierachy would look like:

trait TableEnvironment {
   def fromTableSource(source: TableSource[_]): Table
   def registerExternalCatalog(name: String, externalCatalog:
ExternalCatalog): Unit
   ...
}

trait StreamTableEnvironment extends TableEnvironment {
   def fromDataStream[T](dataStream: DataStream[T]): Table
   ...
}

abstract class InternalTableEnvironment extends TableEnvironment {
   protected def getConversionMapper[OUT](...) = { implementation is here}
   ...
}

class InternalStreamTableEnvironment extends InternalTableEnvironment
with StreamTableEnvironment {
   ... implementation is here
}

I think having Interface and InterfaceImpl is a typical pattern that
could help in making the API more usable.

Yes, implementing everything in Java would help but this would fragement
the code base and would need a lot of work since the table environment
implementation is quite large.

Regards,
Timo



Am 3/3/18 um 8:07 AM schrieb Rong Rong:

> Hi Shuyi,
>
> I am already assuming all package private and protected modifiers will be
> cleaned up and  moved to internal implementation. Please correct me if I
> were wrong, Timo.
>
> Thanks,
> Rong
>
>
> On Fri, Mar 2, 2018, 5:02 PM Shuyi Chen <[hidden email]> wrote:
>
>> Hi Timo,
>>
>> I am throwing some second thoughts here, as I don't quite see what trait
>> provides over abstract class here for TableEnvironment case. Trait in scala
>> can also have implementation and you can have 'private[flink]' or
>> 'protected'  type and method in trait as well.
>>
>> AFAIK, the differences between Scala trait and abstract class are:
>> 1) you can have constructor for abstract class, but not in trait
>> 2) Abstract classes are fully interoperable with Java. You can call them
>> from Java code without any wrappers. Traits are fully interoperable only if
>> they do not contain any implementation code for scala 2.11.
>> 3) you can do multiple inheritance or mixin composition with trait.
>>
>> In the TableEnvironment case,
>> 1) I don't see a need for mixin, and class hierarchy seems fit better here
>> by design.
>> 2) to better interoperate with Java from scala 2.11, it's better to use
>> abstract class. (But AFAIK, scala 2.12 and java 8 would be compatible,
>> though)
>> 3) you might pay a bit performance overhead with trait (compiled to
>> interface) compared to abstract class, but it's not a big deal here.
>>
>> But in other cases, trait might be a better one if it might be reused and
>> mixined in multiple, unrelated classes.
>>
>> So another option would be to refactor TableEnvironment to clean up or move
>> the 'private[flink]' or 'protected' stuff to the actual implementor
>> (e.g. 'InternalTableEnvironment') as
>> you would do for your trait approach for TableEnvironment. I think this
>> option might help with backward compatibility as well. Thanks.
>>
>> Shuyi
>>
>> On Fri, Mar 2, 2018 at 10:25 AM, Rong Rong <[hidden email]> wrote:
>>
>>> Hi Timo,
>>>
>>> Thanks for looking into this Timo. It's becoming increasingly messy for
>> my
>>> trying to locate the correct functions in IDE :-/
>>>
>>> This is probably due to the fact that Scala and Java access modifiers /
>>> qualifiers are subtly and fundamentally different. Using Trait might be
>> the
>>> best solution here. Another way I can think of is to move the all
>>> TableEnvironment classes to Java side, but that would probably introduce
>> a
>>> lot of issue we need to resolve on the Scala side though. "protected" is
>>> less restrictive in Java but there's really no equivalent of package
>>> private modifier on Java.
>>>
>>> I was wondering is there any better way to provide backward-compatible
>>> support though. I played around with it and seems like every "protected"
>>> field will create a private Java member and a public getter, should we
>> add
>>> them all and annotate with "@Deprecated" ?
>>> --
>>> Rong
>>>
>>> On Thu, Mar 1, 2018 at 10:58 AM, Timo Walther <[hidden email]>
>> wrote:
>>>> Hi everyone,
>>>>
>>>> I'm currently thinking about how to implement FLINK-8606. The reason
>>>> behind it is that Java users are able to see all variables and methods
>>> that
>>>> are declared 'private[flink]' or even 'protected' in Scala. Classes
>> such
>>> as
>>>> TableEnvironment look very messy from the outside in Java. Since we
>>> cannot
>>>> change the visibility of Scala protected members, I was thinking about
>> a
>>>> bigger change to solve this issue once and for all. My idea is to
>> convert
>>>> all TableEnvironment classes and maybe the Table class into traits. The
>>>> actual implementation would end up in some internal classes such as
>>>> "InternalTableEnvironment" that implement the public traits. The goal
>>> would
>>>> be to stay source code compatible.
>>>>
>>>> What do you think?
>>>>
>>>> Regards,
>>>> Timo
>>>>
>>>>
>>
>>
>> --
>> "So you have to trust that the dots will somehow connect in your future."
>>