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 <amaliu...@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 <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 >>>>> >>>> >>> >>
