Julian Hyde, I have created a PR with the new rule in : https://github.com/apache/calcite/pull/275

Could you review it? Thanks

I think count == 1 is wrong. What if there were two constant columns, select 1, 2 ... union all select 1, 2 .... When they are removed you have an empty union.

Can you add a test case SELECT ... FROM (SELECT 2 AS x ... UNION ALL SELECT 3 ...) WHERE x > 5? It's a negative test case, of course, but it there's a real danger the logic would get confused by two columns that are constant but are different constants, so it's worth testing.

Just curious, why did you pull up constants (creating a Project) rather than just pulling up predicates? Predicates are more powerful. For instance, in the above case you could easily pull pull up the predicate x = 2 or x = 3 in the above case. Then select ... from v where x > 5 would benefit.

You rely on RexNode.hashCode and .equals. RexInputRef has .hashCode and .equals but RexCall does not. You only use RexInputRef, so I think you're OK, but be careful. (Yes, we should probably add hashNode and equals for all RexNode sub-classes.)

Re "Can you add a test case". Oops, I see you have that already.

Thanks for the careful review Julian Hyde.

I have added a test case for select 1, 2 ... union all select 1, 2 ..... count==1 was an early bail out; in principle, I thought it could indeed be removed, but we need it so the rule is not triggered indefinitely. In the new commit, I also addressed your comment about the iterable on the BitSet.

Edit: nevermind, I have just rewritten the rule so we can bail out later, and hence we can pull up a single constant too.

Edit2: I thought I had figured out a way to detect this loop, but it does not seem straightforward, and we still end up in the firing the rule indefinitely. Thus, I think we need to leave the guard code for count=1 as it is.

Can you add this rule to RelOptUtil.registerAbstractRels? I think it would be useful to have it in the standard rule set (and it would get more testing).

Sure.

Wrt considering predicates instead of constants, I think it could be done indeed. I did not do it originally because the sample cases in Hive were only targeting constant values, thus no special reason to not implement it.

We could create a follow-up issue and implement it later on.

Julian Hyde, I have addressed the review comments, I think the patch is now ready to go in. Is it OK?

Looks good, but running final tests; I will commit when they pass.

Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/9b1624a8.

There was a wrong method call in the md provider : it should have been isAlwaysTrue, which is the case when we do not need to add the pred to the inferred list (it is that way in Hive, I think it ended up changed when I copied/pasted the code).

Fix up in : http://git-wip-us.apache.org/repos/asf/calcite/commit/317802d

Fix up now committed as part of CALCITE-1366.

Resolved in release 1.9.0 (2016-09-22)