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 > > >
