[
https://issues.apache.org/jira/browse/CALCITE-1121?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15252715#comment-15252715
]
Julian Hyde edited comment on CALCITE-1121 at 4/21/16 9:03 PM:
---------------------------------------------------------------
Responding to [~jcamachorodriguez]'s review comments against
https://github.com/julianhyde/calcite/commit/0a1273c25a01d9678277ae6157823db665e46cc9#commitcomment-17193625.
bq. Do we need the List as an input to the constructor? The tree is well-formed
so we could include directly the root RelNode of the tree. I do not think this
would add more complexity to the rest of the methods?
In other adapters we created sub-classes of each kind of {{RelNode}}. For
example, the MongoDB adapter has {{MongoFilter}}, {{MongoFilterRule}},
{{MongoProject}}, {{MongoProjectRule}} etc. But I found that it was a lot of
work to create all of this operators, with little return. In this adapter I am
trying a different strategy. I want to represent a table scan followed by a
linear sequence of operators (Druid does not support union or join), and thus
have {{DruidQuery}} as the only new {{RelNode}}. The {{RelNode}} sub-classes
conveniently represent those operations, and are immutable.
But we're not interested in the tree, or which particular sub-type of
{{RelNode}} each operator is, just the linear sequence. If the sequence is
{{Scan s, Filter f, Project p, Filter f2}}, we don't care whether the input to
F2 is P or something else, just that f2's input row type matches p's output row
type.
The linear sequence is important, so that we can recognize whether we can use a
Druid "select" query or a "groupBy" query, basically a regular expression.
Back to your question. Since the {{RelNode}} s in the list are not necessarily
wired up, we have to pass the whole list.
bq. Currently this case never gets exercised. Is it intentionally included to
cover a follow-up case? Would it be better to include a Unsupported error?
Yeah, I'll remove it. We can consider supporting "topN" in CALCITE-1206.
bq. Same idea as before. Though this is still WIP, would it be better to
include a Unsupported error than adding an artificial aggregation?
I see that Druid's "select" query
[now|http://druid.io/docs/latest/querying/select-query.html] supports a
"filter" attribute. So yes, we should not add an artificial aggregation.
was (Author: julianhyde):
Responding to [~jcamachorodriguez]'s review comments against
https://github.com/julianhyde/calcite/commit/0a1273c25a01d9678277ae6157823db665e46cc9#commitcomment-17193625.
bq. Do we need the List as an input to the constructor? The tree is well-formed
so we could include directly the root RelNode of the tree. I do not think this
would add more complexity to the rest of the methods?
In other adapters we created sub-classes of each kind of RelNode. For example,
the MongoDB adapter has MongoFilter, MongoFilterRule, MongoProject,
MongoProjectRule etc. But I found that it was a lot of work to create all of
this operators, with little return. In this adapter I am trying a different
strategy. I want to represent a table scan followed by a linear sequence of
operators (Druid does not support union or join), and thus have DruidQuery as
the only new RelNode. The RelNode sub-classes conveniently represent those
operations, and are immutable.
But we're not interested in the tree, or which particular sub-type of RelNode
each operator is, just the linear sequence. If the sequence is [Scan s, Filter
f, Project p, Filter f2], we don't care whether the input to F2 is P or
something else, just that f2's input row type matches p's output row type.
The linear sequence is important, so that we can recognize whether we can use a
Druid "select" query or a "groupBy" query, basically a regular expression.
Back to your question. Since the {{RelNode}}s in the list are not necessarily
wired up, we have to pass the whole list.
bq. Currently this case never gets exercised. Is it intentionally included to
cover a follow-up case? Would it be better to include a Unsupported error?
Yeah, I'll remove it. We can consider supporting "topN" in CALCITE-1206.
bq. Same idea as before. Though this is still WIP, would it be better to
include a Unsupported error than adding an artificial aggregation?
I see that "select"
[http://druid.io/docs/latest/querying/select-query.html|now] supports a
"filter" attribute. So yes, we should not add an artificial aggregation.
> Druid adapter
> -------------
>
> Key: CALCITE-1121
> URL: https://issues.apache.org/jira/browse/CALCITE-1121
> Project: Calcite
> Issue Type: Bug
> Reporter: Julian Hyde
> Assignee: Julian Hyde
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
