+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 <liya.fa...@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 <chunwei.l...@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 > > > > > > > > > >
