Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-1321

In-list to join optimization should have configurable in-list size

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.9.0
    • Component/s: None
    • Labels:
      None

      Description

      We current have a default in-list size of 20. Instead of the magic number 20, we should make this configurable.

      select count(*) from table where col in (1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1);
      

        Issue Links

          Activity

          Hide
          gparai Gautam Kumar Parai added a comment -

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

          Show
          gparai Gautam Kumar Parai added a comment - I have created a pull request ( https://github.com/apache/calcite/pull/257 ) Jinfeng Ni Julian Hyde can you please take a look? Thanks!
          Hide
          julianhyde Julian Hyde added a comment -

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

          Show
          julianhyde Julian Hyde added a comment - First reaction to the PR: that's a lot of code. Is there a way of fixing this without writing so much code?
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment -

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

          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - I wonder if this transformation should be expressed as a planner rule, not a SqlToRelConverter property.
          Hide
          jni Jinfeng Ni added a comment -

          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.

          Show
          jni Jinfeng Ni added a comment - 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.
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment -

          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.

          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - 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.
          Hide
          julianhyde Julian Hyde added a comment -

          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?

          Show
          julianhyde Julian Hyde added a comment - 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?
          Hide
          gparai Gautam Kumar Parai added a comment -

          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!

          Show
          gparai Gautam Kumar Parai added a comment - 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!
          Hide
          jni Jinfeng Ni added a comment -

          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?

          Show
          jni Jinfeng Ni added a comment - 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?
          Hide
          julianhyde Julian Hyde added a comment - - edited

          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.

          Show
          julianhyde Julian Hyde added a comment - - edited 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.
          Hide
          gparai Gautam Kumar Parai added a comment -

          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.

          Show
          gparai Gautam Kumar Parai added a comment - 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.
          Hide
          julianhyde Julian Hyde added a comment -

          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.

          Show
          julianhyde Julian Hyde added a comment - 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.
          Hide
          gparai Gautam Kumar Parai added a comment -

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

          Show
          gparai Gautam Kumar Parai added a comment - Julian Hyde I have updated the pull request. ConfigImpl is immutable now. Can you please take a look? Thanks!
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/c933c79f . Thanks for the PR, Gautam Kumar Parai !
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Resolved in release 1.9.0 (2016-09-22)

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.9.0 (2016-09-22)

            People

            • Assignee:
              gparai Gautam Kumar Parai
              Reporter:
              gparai Gautam Kumar Parai
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development