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

Dynamic parameters in OFFSET, FETCH and LIMIT clauses

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 1.15.0
    • Fix Version/s: 1.15.0
    • Component/s: core
    • Labels:
      None

      Description

      Fetch/Offset already support RexNode, it will be useful to support Dynamic parameters as well.

      This implementation is needed to be able to run Yahoo YCSB JDBC benchmarks which does large use of this syntax
      select .... LIMIT ?

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.15.0 (2017-12-11).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.15.0 (2017-12-11).
          Hide
          julianhyde Julian Hyde added a comment -

          Fixed in a5d520df; thanks for the PR, Enrico Olivelli!

          Show
          julianhyde Julian Hyde added a comment - Fixed in a5d520df ; thanks for the PR, Enrico Olivelli !
          Hide
          eolivelli Enrico Olivelli added a comment -

          Patch updated

          Show
          eolivelli Enrico Olivelli added a comment - Patch updated
          Hide
          eolivelli Enrico Olivelli added a comment -

          I think I have found the way. Will send updates soon

          Show
          eolivelli Enrico Olivelli added a comment - I think I have found the way. Will send updates soon
          Hide
          eolivelli Enrico Olivelli added a comment -

          I will fix checkstyle as last task

          Show
          eolivelli Enrico Olivelli added a comment - I will fix checkstyle as last task
          Hide
          eolivelli Enrico Olivelli added a comment -

          Yep, the error is clear.
          But I can't find any way to create an accessor to dynamic parameters as Expression.
          It seems that Expressions.dynamic() is not implemented and there is not used full method in RexToLixTranslator to achieve the goal, I am tring to write is but there is a lot of stuff to learn.
          Do I have to implement such support ? I see in RexToLixTranslator that some support for dynamic parameters is already present.

          For the scope of my urgent needs the patch is working, because I only use the planner.
          If it is a big change as there is no such support dyn parameters in Expressions and you need to close 1.15 next week I propose to split this issue in two parts, merge this part in 1.15 and I volounteer to finish the work as soon as possible but it will go to 1.16.

          The support for dynamic limits in planner for me is really important as it is used in YCSB for benchmarks.

          I will continue my search for the best implementation, maybe the impl is trivial but I am a newbie to Calcite codebase

          Show
          eolivelli Enrico Olivelli added a comment - Yep, the error is clear. But I can't find any way to create an accessor to dynamic parameters as Expression. It seems that Expressions.dynamic() is not implemented and there is not used full method in RexToLixTranslator to achieve the goal, I am tring to write is but there is a lot of stuff to learn. Do I have to implement such support ? I see in RexToLixTranslator that some support for dynamic parameters is already present. For the scope of my urgent needs the patch is working, because I only use the planner. If it is a big change as there is no such support dyn parameters in Expressions and you need to close 1.15 next week I propose to split this issue in two parts, merge this part in 1.15 and I volounteer to finish the work as soon as possible but it will go to 1.16. The support for dynamic limits in planner for me is really important as it is used in YCSB for benchmarks. I will continue my search for the best implementation, maybe the impl is trivial but I am a newbie to Calcite codebase
          Hide
          julianhyde Julian Hyde added a comment -

          I can't really show you how to do it without actually writing the code. That error is a pretty clear indication, I think.

          Show
          julianhyde Julian Hyde added a comment - I can't really show you how to do it without actually writing the code. That error is a pretty clear indication, I think.
          Hide
          eolivelli Enrico Olivelli added a comment - - edited

          I have integrated your changes Julian Hyde
          I can't find how to resolve the value for RexNode which is not a literal
          this is an example of error
          can you point me the correct way to do it ?

          java.lang.AssertionError: not a literal: ?0
          	at org.apache.calcite.rex.RexLiteral.findValue(RexLiteral.java:960)
          	at org.apache.calcite.rex.RexLiteral.intValue(RexLiteral.java:935)
          	at org.apache.calcite.adapter.enumerable.EnumerableLimit.implement(EnumerableLimit.java:120)
          	at org.apache.calcite.adapter.enumerable.EnumerableRelImplementor.visitChild(EnumerableRelImplementor.java:103)
          	at org.apache.calcite.adapter.enumerable.EnumerableCalc.implement(EnumerableCalc.java:124)
          	at org.apache.calcite.adapter.enumerable.EnumerableRelImplementor.implementRoot(EnumerableRelImplementor.java:108)
          	at org.apache.calcite.adapter.enumerable.EnumerableInterpretable.toBindable(EnumerableInterpretable.java:92)
          	at org.apache.calcite.prepare.CalcitePrepareImpl$CalcitePreparingStmt.implement(CalcitePrepareImpl.java:1261)
          	at org.apache.calcite.prepare.Prepare.prepareSql(Prepare.java:330)
          	at org.apache.calcite.prepare.Prepare.prepareSql(Prepare.java:229)
          
          Show
          eolivelli Enrico Olivelli added a comment - - edited I have integrated your changes Julian Hyde I can't find how to resolve the value for RexNode which is not a literal this is an example of error can you point me the correct way to do it ? java.lang.AssertionError: not a literal: ?0 at org.apache.calcite.rex.RexLiteral.findValue(RexLiteral.java:960) at org.apache.calcite.rex.RexLiteral.intValue(RexLiteral.java:935) at org.apache.calcite.adapter.enumerable.EnumerableLimit.implement(EnumerableLimit.java:120) at org.apache.calcite.adapter.enumerable.EnumerableRelImplementor.visitChild(EnumerableRelImplementor.java:103) at org.apache.calcite.adapter.enumerable.EnumerableCalc.implement(EnumerableCalc.java:124) at org.apache.calcite.adapter.enumerable.EnumerableRelImplementor.implementRoot(EnumerableRelImplementor.java:108) at org.apache.calcite.adapter.enumerable.EnumerableInterpretable.toBindable(EnumerableInterpretable.java:92) at org.apache.calcite.prepare.CalcitePrepareImpl$CalcitePreparingStmt.implement(CalcitePrepareImpl.java:1261) at org.apache.calcite.prepare.Prepare.prepareSql(Prepare.java:330) at org.apache.calcite.prepare.Prepare.prepareSql(Prepare.java:229)
          Hide
          julianhyde Julian Hyde added a comment -

          Reviewing https://github.com/apache/calcite/pull/569/commits/5f314a7b3c6e04966db02a53f38429dade02e738:

          • This is a good start - well done
          • I fixed the white-space after "?" in my branch https://github.com/julianhyde/calcite/tree/2061-offset-fetch-parameter
          • I also added a test to JdbcTest. It has errors currently - can you fix please? And write more tests if necessary
          • Also please make sure checkstyle and all other tests pass (checkstyle currently fails) and then let me know when you are ready for another review
          Show
          julianhyde Julian Hyde added a comment - Reviewing https://github.com/apache/calcite/pull/569/commits/5f314a7b3c6e04966db02a53f38429dade02e738: This is a good start - well done I fixed the white-space after "?" in my branch https://github.com/julianhyde/calcite/tree/2061-offset-fetch-parameter I also added a test to JdbcTest. It has errors currently - can you fix please? And write more tests if necessary Also please make sure checkstyle and all other tests pass (checkstyle currently fails) and then let me know when you are ready for another review
          Hide
          eolivelli Enrico Olivelli added a comment -
          Show
          eolivelli Enrico Olivelli added a comment - this is the patch https://github.com/apache/calcite/pull/569/files
          Hide
          eolivelli Enrico Olivelli added a comment -

          Michael Mior Julian Hyde I would like to pick this up. I am starting an implementation, for me is blocker

          Show
          eolivelli Enrico Olivelli added a comment - Michael Mior Julian Hyde I would like to pick this up. I am starting an implementation, for me is blocker

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              eolivelli Enrico Olivelli
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development