Responding to Jesus Camacho Rodriguez's review comments against https://github.com/julianhyde/calcite/commit/0a1273c25a01d9678277ae6157823db665e46cc9#commitcomment-17193625.
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.
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.
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 supports a "filter" attribute. So yes, we should not add an artificial aggregation.