calcite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vladimir Sitnikov <sitnikov.vladi...@gmail.com>
Subject Re: Review calcite-445
Date Sun, 22 Feb 2015 12:11:44 GMT
Hi,

> This change lays the foundations for streaming queries, such as being able
> to query both the current contents of a table and future deltas to it

I am sorry, I cannot yet understand how BINDABLE projects/filters are
the foundation for chi.

>  to generate a different expression
> for the two forms of the same table,

Can you pin-point the part of the code?
As far as I can find, you always call StreamableTable.stream() before
usage, so I cannot understand why do you need several expressions.

> more of the complex runtime logic (e.g. negotiating which filters and
> projects to push down) into BindableTableScan.

You mean TableScanNode, don't you? (its create method is a bit too long)

It is interesting that TableScanNode can rescan _multiple_ times in
case projects are too aggressive.
It tuns out to be:
1) Functional issue: enumerable1 is not closed in case of rescan. This
might lead to resource leak.
2) Documentation/design flaw. It probably would be nice if PFT could
return more projections than asked.

In general change in calcite-445 looks good, however as you put in
javadoc of CalcSplitRule: "Not enabled by default, as it works against
the usual flow". It is bad cost does not accomodate the number of
pushed filters, etc.

Comments regarding the code:
c1) TableScanNode seems to pretend it knows how to map Class to field
list. I'm not sure if it is in line with table.rowType.
https://github.com/julianhyde/incubator-calcite/blob/bd0b606178252c90805aff49851f4c9d5eea1a0a/core/src/main/java/org/apache/calcite/interpreter/TableScanNode.java#L180
c2) ImmutableIntList#identity might probably be better expressed in
terms of ImmutableIntList#range (that is avoid creation of int[])

Vladimir

Mime
View raw message