You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@calcite.apache.org by Vladimir Sitnikov <si...@gmail.com> on 2020/09/29 14:44:34 UTC
Re: [DISCUSS] Sarg (search argument) to generalize and replace IN in RexCall
Apparently, RangeSet is a @Beta API in Guava, and they stress that @Beta
APIs should not be used in libraries.
I suggest we do something with that, otherwise, it results in a case where
Calcite enforces all the consumers to depend on @Beta API which
might disappear or drift.
See https://github.com/google/guava/wiki/PhilosophyExplained#beta-apis
It is literally the very first "important warning":
https://github.com/google/guava#important-warnings
1, APIs marked with the @Beta annotation at the class or method level are
subject to change.
They can be modified in any way, or even removed, at any time. If your
code is a library itself
(i.e., it is used on the CLASSPATH of users outside your own control),
you should not use beta APIs unless you repackage them. If your code is a
library,
we strongly recommend using the Guava Beta Checker to ensure that you do
not use any @Beta APIs!
Should we revert RangeSets?
Should we ask Guava to promote it from @Beta?
Vladimir
Re: [DISCUSS] Sarg (search argument) to generalize and replace IN in
RexCall
Posted by Julian Hyde <jh...@gmail.com>.
There is already an issue requesting RangeSet taken out of beta: https://github.com/google/guava/issues/3376 <https://github.com/google/guava/issues/3376>
> On Sep 29, 2020, at 1:41 PM, Stamatis Zampetakis <za...@gmail.com> wrote:
>
> If it is a big concern can't we mark our own classes/fields relying on Beta
> APIs as experimental and subject to change?
>
> In [1] they also say the following about Beta APIs:
>
> "All this said, @Beta features tend to remain relatively stable. If we
> decide to delete a @Beta feature, we will typically deprecate it for one
> release before deleting it.
>
> On the other hand, if you want something taken out of @Beta, file an issue.
> We generally promote features out of @Beta only when it's specifically
> requested, so if you don't ask, it won't happen."
>
> So in the meantime let's also request them to promote the respective APIs
> from beta.
>
> Best,
> Stamatis
>
> [1] https://github.com/google/guava/wiki/PhilosophyExplained
>
> On Tue, Sep 29, 2020 at 9:31 PM Julian Hyde <jh...@gmail.com> wrote:
>
>> For the record, the Druid adapter has used RangeSet for a long while, and
>> it made sense, because Druid was doing tricky computations on date ranges.
>> Introducing Sargs brought that style to other parts of Calcite.
>>
>> If someone was to build an adapter similar to the Druid adapter, based on
>> 1.26, externally to Calcite, they probably would not have to depend on
>> RangeSet because Calcite’s Sarg class would do the rewrites that they need.
>>
>> Julian
>>
>>
>>> On Sep 29, 2020, at 12:24 PM, Vladimir Sitnikov <
>> sitnikov.vladimir@gmail.com> wrote:
>>>
>>> Julian>The vast majority of clients who use Sarg (or expressions that
>>> contain Sarg) will not reference RangeSet
>>> Julian> directly and therefore would not be impacted. So I think it’s an
>>> acceptable risk.
>>>
>>> Well, it is hard to tell, however, I know Druid is using Sargs, and, I
>>> guess, Druid is one among the very least tested adapters.
>>> See
>>>
>> https://github.com/apache/calcite/pull/2182/commits/edf57dce13d00d3f7c4035c323f5de2568dc8699
>>>
>>> In other words, Druid adapter proves Calcite forces clients to use
>>> Guava's @Beta API :(
>>>
>>> Vladimir
>>
>>
Re: [DISCUSS] Sarg (search argument) to generalize and replace IN in RexCall
Posted by Stamatis Zampetakis <za...@gmail.com>.
If it is a big concern can't we mark our own classes/fields relying on Beta
APIs as experimental and subject to change?
In [1] they also say the following about Beta APIs:
"All this said, @Beta features tend to remain relatively stable. If we
decide to delete a @Beta feature, we will typically deprecate it for one
release before deleting it.
On the other hand, if you want something taken out of @Beta, file an issue.
We generally promote features out of @Beta only when it's specifically
requested, so if you don't ask, it won't happen."
So in the meantime let's also request them to promote the respective APIs
from beta.
Best,
Stamatis
[1] https://github.com/google/guava/wiki/PhilosophyExplained
On Tue, Sep 29, 2020 at 9:31 PM Julian Hyde <jh...@gmail.com> wrote:
> For the record, the Druid adapter has used RangeSet for a long while, and
> it made sense, because Druid was doing tricky computations on date ranges.
> Introducing Sargs brought that style to other parts of Calcite.
>
> If someone was to build an adapter similar to the Druid adapter, based on
> 1.26, externally to Calcite, they probably would not have to depend on
> RangeSet because Calcite’s Sarg class would do the rewrites that they need.
>
> Julian
>
>
> > On Sep 29, 2020, at 12:24 PM, Vladimir Sitnikov <
> sitnikov.vladimir@gmail.com> wrote:
> >
> > Julian>The vast majority of clients who use Sarg (or expressions that
> > contain Sarg) will not reference RangeSet
> > Julian> directly and therefore would not be impacted. So I think it’s an
> > acceptable risk.
> >
> > Well, it is hard to tell, however, I know Druid is using Sargs, and, I
> > guess, Druid is one among the very least tested adapters.
> > See
> >
> https://github.com/apache/calcite/pull/2182/commits/edf57dce13d00d3f7c4035c323f5de2568dc8699
> >
> > In other words, Druid adapter proves Calcite forces clients to use
> > Guava's @Beta API :(
> >
> > Vladimir
>
>
Re: [DISCUSS] Sarg (search argument) to generalize and replace IN in
RexCall
Posted by Julian Hyde <jh...@gmail.com>.
For the record, the Druid adapter has used RangeSet for a long while, and it made sense, because Druid was doing tricky computations on date ranges. Introducing Sargs brought that style to other parts of Calcite.
If someone was to build an adapter similar to the Druid adapter, based on 1.26, externally to Calcite, they probably would not have to depend on RangeSet because Calcite’s Sarg class would do the rewrites that they need.
Julian
> On Sep 29, 2020, at 12:24 PM, Vladimir Sitnikov <si...@gmail.com> wrote:
>
> Julian>The vast majority of clients who use Sarg (or expressions that
> contain Sarg) will not reference RangeSet
> Julian> directly and therefore would not be impacted. So I think it’s an
> acceptable risk.
>
> Well, it is hard to tell, however, I know Druid is using Sargs, and, I
> guess, Druid is one among the very least tested adapters.
> See
> https://github.com/apache/calcite/pull/2182/commits/edf57dce13d00d3f7c4035c323f5de2568dc8699
>
> In other words, Druid adapter proves Calcite forces clients to use
> Guava's @Beta API :(
>
> Vladimir
Re: [DISCUSS] Sarg (search argument) to generalize and replace IN in RexCall
Posted by Vladimir Sitnikov <si...@gmail.com>.
Julian>The vast majority of clients who use Sarg (or expressions that
contain Sarg) will not reference RangeSet
Julian> directly and therefore would not be impacted. So I think it’s an
acceptable risk.
Well, it is hard to tell, however, I know Druid is using Sargs, and, I
guess, Druid is one among the very least tested adapters.
See
https://github.com/apache/calcite/pull/2182/commits/edf57dce13d00d3f7c4035c323f5de2568dc8699
In other words, Druid adapter proves Calcite forces clients to use
Guava's @Beta API :(
Vladimir
Re: [DISCUSS] Sarg (search argument) to generalize and replace IN in
RexCall
Posted by Julian Hyde <jh...@gmail.com>.
> On Sep 29, 2020, at 11:40 AM, Vladimir Sitnikov <si...@gmail.com> wrote:
>
> Julian>Suppose - worst case scenario - that Guava 30 removes RangeSet. The
> solution would be for us to copy RangeSet and enough dependent classes into
> Calcite to serve our needs
>
> Then all the clients would have to adjust packages because we can't copy
> Guava code and keep Guava's package names.
That is a valid concern. RangeSet used in a public field and a public method in class Sarg.
The vast majority of clients who use Sarg (or expressions that contain Sarg) will not reference RangeSet directly and therefore would not be impacted. So I think it’s an acceptable risk.
Julian
Re: [DISCUSS] Sarg (search argument) to generalize and replace IN in RexCall
Posted by Vladimir Sitnikov <si...@gmail.com>.
Julian>Suppose - worst case scenario - that Guava 30 removes RangeSet. The
solution would be for us to copy RangeSet and enough dependent classes into
Calcite to serve our needs
Then all the clients would have to adjust packages because we can't copy
Guava code and keep Guava's package names.
Julian>I knew the risks going in, and I’m not very concerned
I see Guava adjusted RangeSet#toString(). The change like that triggers
lots of plan representation changes :-(
Vladimir
Re: [DISCUSS] Sarg (search argument) to generalize and replace IN in
RexCall
Posted by Julian Hyde <jh...@gmail.com>.
I knew the risks going in, and I’m not very concerned. Suppose - worst case scenario - that Guava 30 removes RangeSet. The solution would be for us to copy RangeSet and enough dependent classes into Calcite to serve our needs. Our support for Guava 30 would be delayed by a month or two.
Sarg is a powerful abstraction for planning queries, and RangeSet is a good library to implement it on. If it hadn’t been in Guava, we would have had to implement something like it ourselves. (In fact, I recall one Calcite PR that basically had its own implementation of RangeSet.)
Julian
> On Sep 29, 2020, at 7:44 AM, Vladimir Sitnikov <si...@gmail.com> wrote:
>
> Apparently, RangeSet is a @Beta API in Guava, and they stress that @Beta
> APIs should not be used in libraries.
>
> I suggest we do something with that, otherwise, it results in a case where
> Calcite enforces all the consumers to depend on @Beta API which
> might disappear or drift.
>
> See https://github.com/google/guava/wiki/PhilosophyExplained#beta-apis
>
> It is literally the very first "important warning":
>
> https://github.com/google/guava#important-warnings
>
> 1, APIs marked with the @Beta annotation at the class or method level are
> subject to change.
> They can be modified in any way, or even removed, at any time. If your
> code is a library itself
> (i.e., it is used on the CLASSPATH of users outside your own control),
> you should not use beta APIs unless you repackage them. If your code is a
> library,
> we strongly recommend using the Guava Beta Checker to ensure that you do
> not use any @Beta APIs!
>
> Should we revert RangeSets?
> Should we ask Guava to promote it from @Beta?
>
> Vladimir