You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@calcite.apache.org by Haisheng Yuan <hy...@apache.org> on 2020/11/05 03:44:18 UTC
Re: [DISCUSS] Make SqlNodeList implement List<SqlNode>
The performance gain depends on the size of sqlnode list.
I am in favor of making SqlNodeList implement List<SqlNode>.
+1 to the proposal.
Haisheng Yuan
On 2020/11/05 02:32:01, Chunwei Lei <ch...@gmail.com> wrote:
> Thank you for raising this, Julian.
>
> How much performance improvement can we get?
>
>
>
> Best,
> Chunwei
>
>
> On Thu, Nov 5, 2020 at 5:32 AM Julian Hyde <jh...@apache.org> wrote:
>
> > Currently class SqlNodeList [1] implements Iterable<SqlNode> but it
> > does not implement List<SqlNode>. How do people feel about doing that?
> > (Or Collection<SqlNode>?)
> >
> > The main potential benefit is performance. Consider this code:
> >
> > SqlNodeList nodeList;
> > ImmutableList immutableList1 = ImmutableList.copyOf(nodeList);
> >
> > List<SqlNode> list = nodeList.toList();
> > ImmutableList immutableList2 = ImmutableList.copyOf(list);
> >
> > Today, the second form is faster, because ImmutableList.copyOf can
> > call size() and preallocate a list of the right size.
> >
> > The second benefit is that we can remove calls to '.toList()'.
> >
> > The downside is that a few locations are overloaded. For example,
> > class Span has overloaded methods
> >
> > static Span of(SqlNode node);
> > static Span of(Collection<? extends SqlNode> nodes)
> >
> > If SqlNodeList implements List<? extends SqlNode>, then the code
> >
> > SqlNodeList nodeList;
> > Span s = Span.of(nodeList);
> >
> > becomes ambiguous. We can disambiguate by adding Span.of(SqlNodeList).
> > But there may be other locations in client code that cannot be
> > disambiguated.
> >
> > Julian
> >
> > [1]
> > https://github.com/apache/calcite/blob/155276591288615c4d02d55fb7d77eceb2e24b2d/core/src/main/java/org/apache/calcite/sql/SqlNodeList.java#L38
> >
>
Re: [DISCUSS] Make SqlNodeList implement List<SqlNode>
Posted by Julian Hyde <jh...@gmail.com>.
I don’t think it’s necessary to quantify the performance improvements. If it’s the right thing to do, and the incompatibilities are not significant, let’s just do it.
I have logged https://issues.apache.org/jira/browse/CALCITE-4380 <https://issues.apache.org/jira/browse/CALCITE-4380>.
> On Nov 5, 2020, at 12:34 PM, Rui Wang <am...@apache.org> wrote:
>
> +1 on potential performance gain.
>
> If I have some time, I will try to research and understand how fast the
> space preallocation can offer.
>
>
> -Rui
>
> On Thu, Nov 5, 2020 at 12:34 AM Fan Liya <li...@gmail.com> wrote:
>
>> IMO, the major advantage of Iterable over List is that we do not need to
>> know all the elements before traversing the collection.
>> According to the current implementation, however, SqlNodeList is internally
>> backed by a List, so the advantage of Iterable no longer exists.
>> Therefore, I think it is more natural to make it implement List.
>>
>> Best,
>> Liya Fan
>>
>>
>> On Thu, Nov 5, 2020 at 11:44 AM Haisheng Yuan <hy...@apache.org> wrote:
>>
>>> The performance gain depends on the size of sqlnode list.
>>> I am in favor of making SqlNodeList implement List<SqlNode>.
>>> +1 to the proposal.
>>>
>>> Haisheng Yuan
>>>
>>> On 2020/11/05 02:32:01, Chunwei Lei <ch...@gmail.com> wrote:
>>>> Thank you for raising this, Julian.
>>>>
>>>> How much performance improvement can we get?
>>>>
>>>>
>>>>
>>>> Best,
>>>> Chunwei
>>>>
>>>>
>>>> On Thu, Nov 5, 2020 at 5:32 AM Julian Hyde <jh...@apache.org> wrote:
>>>>
>>>>> Currently class SqlNodeList [1] implements Iterable<SqlNode> but it
>>>>> does not implement List<SqlNode>. How do people feel about doing
>> that?
>>>>> (Or Collection<SqlNode>?)
>>>>>
>>>>> The main potential benefit is performance. Consider this code:
>>>>>
>>>>> SqlNodeList nodeList;
>>>>> ImmutableList immutableList1 = ImmutableList.copyOf(nodeList);
>>>>>
>>>>> List<SqlNode> list = nodeList.toList();
>>>>> ImmutableList immutableList2 = ImmutableList.copyOf(list);
>>>>>
>>>>> Today, the second form is faster, because ImmutableList.copyOf can
>>>>> call size() and preallocate a list of the right size.
>>>>>
>>>>> The second benefit is that we can remove calls to '.toList()'.
>>>>>
>>>>> The downside is that a few locations are overloaded. For example,
>>>>> class Span has overloaded methods
>>>>>
>>>>> static Span of(SqlNode node);
>>>>> static Span of(Collection<? extends SqlNode> nodes)
>>>>>
>>>>> If SqlNodeList implements List<? extends SqlNode>, then the code
>>>>>
>>>>> SqlNodeList nodeList;
>>>>> Span s = Span.of(nodeList);
>>>>>
>>>>> becomes ambiguous. We can disambiguate by adding
>> Span.of(SqlNodeList).
>>>>> But there may be other locations in client code that cannot be
>>>>> disambiguated.
>>>>>
>>>>> Julian
>>>>>
>>>>> [1]
>>>>>
>>>
>> https://github.com/apache/calcite/blob/155276591288615c4d02d55fb7d77eceb2e24b2d/core/src/main/java/org/apache/calcite/sql/SqlNodeList.java#L38
>>>>>
>>>>
>>>
>>
Re: [DISCUSS] Make SqlNodeList implement List<SqlNode>
Posted by Rui Wang <am...@apache.org>.
+1 on potential performance gain.
If I have some time, I will try to research and understand how fast the
space preallocation can offer.
-Rui
On Thu, Nov 5, 2020 at 12:34 AM Fan Liya <li...@gmail.com> wrote:
> IMO, the major advantage of Iterable over List is that we do not need to
> know all the elements before traversing the collection.
> According to the current implementation, however, SqlNodeList is internally
> backed by a List, so the advantage of Iterable no longer exists.
> Therefore, I think it is more natural to make it implement List.
>
> Best,
> Liya Fan
>
>
> On Thu, Nov 5, 2020 at 11:44 AM Haisheng Yuan <hy...@apache.org> wrote:
>
> > The performance gain depends on the size of sqlnode list.
> > I am in favor of making SqlNodeList implement List<SqlNode>.
> > +1 to the proposal.
> >
> > Haisheng Yuan
> >
> > On 2020/11/05 02:32:01, Chunwei Lei <ch...@gmail.com> wrote:
> > > Thank you for raising this, Julian.
> > >
> > > How much performance improvement can we get?
> > >
> > >
> > >
> > > Best,
> > > Chunwei
> > >
> > >
> > > On Thu, Nov 5, 2020 at 5:32 AM Julian Hyde <jh...@apache.org> wrote:
> > >
> > > > Currently class SqlNodeList [1] implements Iterable<SqlNode> but it
> > > > does not implement List<SqlNode>. How do people feel about doing
> that?
> > > > (Or Collection<SqlNode>?)
> > > >
> > > > The main potential benefit is performance. Consider this code:
> > > >
> > > > SqlNodeList nodeList;
> > > > ImmutableList immutableList1 = ImmutableList.copyOf(nodeList);
> > > >
> > > > List<SqlNode> list = nodeList.toList();
> > > > ImmutableList immutableList2 = ImmutableList.copyOf(list);
> > > >
> > > > Today, the second form is faster, because ImmutableList.copyOf can
> > > > call size() and preallocate a list of the right size.
> > > >
> > > > The second benefit is that we can remove calls to '.toList()'.
> > > >
> > > > The downside is that a few locations are overloaded. For example,
> > > > class Span has overloaded methods
> > > >
> > > > static Span of(SqlNode node);
> > > > static Span of(Collection<? extends SqlNode> nodes)
> > > >
> > > > If SqlNodeList implements List<? extends SqlNode>, then the code
> > > >
> > > > SqlNodeList nodeList;
> > > > Span s = Span.of(nodeList);
> > > >
> > > > becomes ambiguous. We can disambiguate by adding
> Span.of(SqlNodeList).
> > > > But there may be other locations in client code that cannot be
> > > > disambiguated.
> > > >
> > > > Julian
> > > >
> > > > [1]
> > > >
> >
> https://github.com/apache/calcite/blob/155276591288615c4d02d55fb7d77eceb2e24b2d/core/src/main/java/org/apache/calcite/sql/SqlNodeList.java#L38
> > > >
> > >
> >
>
Re: [DISCUSS] Make SqlNodeList implement List<SqlNode>
Posted by Fan Liya <li...@gmail.com>.
IMO, the major advantage of Iterable over List is that we do not need to
know all the elements before traversing the collection.
According to the current implementation, however, SqlNodeList is internally
backed by a List, so the advantage of Iterable no longer exists.
Therefore, I think it is more natural to make it implement List.
Best,
Liya Fan
On Thu, Nov 5, 2020 at 11:44 AM Haisheng Yuan <hy...@apache.org> wrote:
> The performance gain depends on the size of sqlnode list.
> I am in favor of making SqlNodeList implement List<SqlNode>.
> +1 to the proposal.
>
> Haisheng Yuan
>
> On 2020/11/05 02:32:01, Chunwei Lei <ch...@gmail.com> wrote:
> > Thank you for raising this, Julian.
> >
> > How much performance improvement can we get?
> >
> >
> >
> > Best,
> > Chunwei
> >
> >
> > On Thu, Nov 5, 2020 at 5:32 AM Julian Hyde <jh...@apache.org> wrote:
> >
> > > Currently class SqlNodeList [1] implements Iterable<SqlNode> but it
> > > does not implement List<SqlNode>. How do people feel about doing that?
> > > (Or Collection<SqlNode>?)
> > >
> > > The main potential benefit is performance. Consider this code:
> > >
> > > SqlNodeList nodeList;
> > > ImmutableList immutableList1 = ImmutableList.copyOf(nodeList);
> > >
> > > List<SqlNode> list = nodeList.toList();
> > > ImmutableList immutableList2 = ImmutableList.copyOf(list);
> > >
> > > Today, the second form is faster, because ImmutableList.copyOf can
> > > call size() and preallocate a list of the right size.
> > >
> > > The second benefit is that we can remove calls to '.toList()'.
> > >
> > > The downside is that a few locations are overloaded. For example,
> > > class Span has overloaded methods
> > >
> > > static Span of(SqlNode node);
> > > static Span of(Collection<? extends SqlNode> nodes)
> > >
> > > If SqlNodeList implements List<? extends SqlNode>, then the code
> > >
> > > SqlNodeList nodeList;
> > > Span s = Span.of(nodeList);
> > >
> > > becomes ambiguous. We can disambiguate by adding Span.of(SqlNodeList).
> > > But there may be other locations in client code that cannot be
> > > disambiguated.
> > >
> > > Julian
> > >
> > > [1]
> > >
> https://github.com/apache/calcite/blob/155276591288615c4d02d55fb7d77eceb2e24b2d/core/src/main/java/org/apache/calcite/sql/SqlNodeList.java#L38
> > >
> >
>