I have created a pull request (https://github.com/apache/calcite/pull/257) Jinfeng Ni Julian Hyde can you please take a look? Thanks!

First reaction to the PR: that's a lot of code. Is there a way of fixing this without writing so much code?

I wonder if this transformation should be expressed as a planner rule, not a SqlToRelConverter property.

The majority of code change seems to new unit test, which has better put the expected explain result in SqlToRelConveterTest.xml, in stead of in SqlToRelConverterTest.java.

I agree the transformation had better expressed as a planner rule, so that people has the control whether they want such transformation, and when they want to apply such transformation. Using SqlToRelConverter property is a step towards the first goal.

Using SqlToRelConverter property is a step towards the first goal.

SqlToRelConverter property will stay in public API of SqlToRelConverter, that is why I like "separate rule" idea better.

Until CALCITE-816, using a rule was not feasible, because there was no way to represent an IN expression in the RelNode/RexNode algebra. Now it is possible, and I agree, it's a good option. The in-list threshold would become a property of the rule instance.

I'm not that keen on adding a Config object to SqlToRelConverter, but I could be persuaded. Adding it and not dealing with existing properties (e.g. trimUnusedFields) is worse, because it incurs technical debt.

Gautam Kumar Parai, Can you change SqlToRelConverterTest to put the plans in SqlToRelConverterTest.xml as the other tests do?

I have updated the pull request (https://github.com/apache/calcite/pull/257) Jinfeng Ni Julian Hyde Can you please take a look at the updated pull request? Thanks!

Julian Hyde, not sure if you prefer a planner rule, or adding a Config object to SqlToRelConverter. Can you take another look at Gautam's new PR?

I'm not really sure whether I prefer a planner rule or a config. Planner rule is quite a lot more work, obviously. This patch looks like it's heading in the right direction, so let's go the config route for now.

The revised code looks better, because there's less code! You can remove a lot more by adding a method withConfig to Sql and removing the method convertsTo(String plan, SqlToRelConverter.Config config).

Please deprecate the old SqlToRelConverter constructor, and remove 2 of the 3 SqlToRelConverterTest.assertConvertsTo methods.

Remove all of the config fields in SqlToRelConverter (e.g. inSubqueryThreshold), and deprecate their accessor methods (e.g. getInSubqueryThreshold). Add a public final Config field.

Also, a couple of slips:

• Not "SQLToRelConverter", but "SqlToRelConverter".
• "Called by builder; all values are in private final fields." is a good idea. Make it so.

Continuation indent 4, not 8.

Jinfeng Ni Julian Hyde Thanks so much for the comments. I have updated the pull request to address them (https://github.com/apache/calcite/pull/257).

Regarding Julian Hyde]'s comment " "Called by builder; all values are in private final fields." is a good idea. Make it so." There are setter accessor methods in

SqlToRelConverter

which require

ConfigImpl

members to NOT be final.

If you look through how SqlToRelConverter is used, I believe you can remove the setter methods or make them deprecated and throw UnsupportedOperationException. And you can make ConfigImpl immutable.

Julian Hyde I have updated the pull request. ConfigImpl is immutable now. Can you please take a look? Thanks!

Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/c933c79f. Thanks for the PR, Gautam Kumar Parai!

Resolved in release 1.9.0 (2016-09-22)