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

Add Utility to convert RelNode to SqlNode

    Details

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

      Description

      To track progress on Rel2Sql Converter.
      Following is the email conversation that lead to this JIRA:

      On Wed, Dec 2, 2015 at 3:13 AM, Julian Hyde <jhyde@apache.org> wrote:

      Yes, this would definitely be useful in Calcite. Thanks for offering.

      I would like to reduce the amount of code copy-pasted from the JdbcXxx relational expressions, but we can work on that after it is committed in and when there are some unit tests.

      Can you please create a JIRA case with an initial pull-request?

      I think the unit tests could be of the following form:

            @Test public void testScan() {
              final String sql = “select * from emp”;
              final String after = "SELECT *\n”
                  + "FROM \”EMP\””;
              check(sql, after);
            }
      

      where “after” is the result of the round trip SQL => SqlNode => RelNode => SqlNode => SQL, and your utility is performing the 3rd “=>”.

      Julian

      > On Dec 1, 2015, at 12:28 AM, Amogh Margoor <amoghm@qubole.com> wrote:
      >
      > Hi,
      > We have an usecase where we need to just send back optimized query as SQL,
      > without executing it. So we needed an utility to convert RelNode back to
      > SQL, and found most of the logic to be in JDBCRel. As we needed it for non
      > JDBC source, we created an utility to do so:
      > https://github.com/amoghmargoor/incubator-calcite/blob/NEZ-52/core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
      >
      > Utility is almost completely copy-pasted from JDBCRels with few fixes from
      > our side to make it work. If such utility will be useful in Calcite then
      > let us know, we can plan raising PRs with required UTs.
      >
      > Regards,
      > Amogh

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.6.0 (2016-01-22).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.6.0 (2016-01-22).
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/a8a8878d , with follow-up commits thru http://git-wip-us.apache.org/repos/asf/calcite/commit/47e0e7c9 . Thanks for the PR, Amogh Margoor !
          Hide
          julianhyde Julian Hyde added a comment -

          I've logged CALCITE-1010. I think I'll commit the work not including LIMIT/OFFSET/FETCH, and mark this issue fixed, then let's do LIMIT/OFFSET/FETCH properly, in the dialect.

          Show
          julianhyde Julian Hyde added a comment - I've logged CALCITE-1010 . I think I'll commit the work not including LIMIT/OFFSET/FETCH, and mark this issue fixed, then let's do LIMIT/OFFSET/FETCH properly, in the dialect.
          Hide
          amargoor Amogh Margoor added a comment -

          You can commit these changes if they are fine. I will log a JIRA for LIMIT issue and raise separate PR for it by the end of this weekend.
          I remember both "FETCH NEXT N ROWS ONLY" and "OFFSET N ROWS" were not supported for Redshift and Hive (checked on 0.13), whereas LIMIT N and OFFSET N were supported. I think even mysql had the same behavior (need to confirm though).

          Show
          amargoor Amogh Margoor added a comment - You can commit these changes if they are fine. I will log a JIRA for LIMIT issue and raise separate PR for it by the end of this weekend. I remember both "FETCH NEXT N ROWS ONLY" and "OFFSET N ROWS" were not supported for Redshift and Hive (checked on 0.13), whereas LIMIT N and OFFSET N were supported. I think even mysql had the same behavior (need to confirm though).
          Hide
          julianhyde Julian Hyde added a comment -

          Regarding LIMIT versus FETCH. Is that a change you think you'll make in the next day or so? If so I'll wait for it before committing. If not, please log a jira case.

          I agree that the support is patchy. LIMIT is more common but FETCH is the standard. We should support both, and the dialect should know what each database supports (not SqlSelectOperator as you have it now). To make this concrete, can you suggest a database that only supports LIMIT? Let's add a test where we generate SQL for that database.

          Show
          julianhyde Julian Hyde added a comment - Regarding LIMIT versus FETCH. Is that a change you think you'll make in the next day or so? If so I'll wait for it before committing. If not, please log a jira case. I agree that the support is patchy. LIMIT is more common but FETCH is the standard. We should support both, and the dialect should know what each database supports (not SqlSelectOperator as you have it now). To make this concrete, can you suggest a database that only supports LIMIT? Let's add a test where we generate SQL for that database.
          Hide
          amargoor Amogh Margoor added a comment - - edited

          Sorry for the delay, Julian Hyde. Code looks good to me and tests run just fine on it. However OFFSET and FETCH needed some changes. I have done those changes and added UTs for it in this commit: https://github.com/amoghmargoor/incubator-calcite/commit/d1ec52186d80f07d0b4dae21849b32d64d815dce
          PR has been updated by both this commit and your changes.

          One small enhancement is still pending for LIMIT. Most databases support LIMIT keyword instead of FETCH. So we have made change to SqlDialect which sets up a limit flag and outputs LIMIT keyword instead of FETCH during `unparse` or pretty-printing. I was not sure if I should club Dialect change with this PR so have not patched that in.

          Show
          amargoor Amogh Margoor added a comment - - edited Sorry for the delay, Julian Hyde . Code looks good to me and tests run just fine on it. However OFFSET and FETCH needed some changes. I have done those changes and added UTs for it in this commit: https://github.com/amoghmargoor/incubator-calcite/commit/d1ec52186d80f07d0b4dae21849b32d64d815dce PR has been updated by both this commit and your changes. One small enhancement is still pending for LIMIT. Most databases support LIMIT keyword instead of FETCH. So we have made change to SqlDialect which sets up a limit flag and outputs LIMIT keyword instead of FETCH during `unparse` or pretty-printing. I was not sure if I should club Dialect change with this PR so have not patched that in.
          Hide
          julianhyde Julian Hyde added a comment -

          Amogh Margoor, Thanks for the PR. I have done some work on it, eliminating duplicate code between JdbcRules and RelToSqlConverter by creating a new class SqlImplementor. I changed Context.ignoreCast to false, and found a different/better way to ignore trivial casts. I also incorporated fixes for CALCITE-970 and CALCITE-893 that had been missed in your copy of the code.

          See my https://github.com/julianhyde/calcite/commits/1003-rel-to-sql branch, in particular commit 69e20934. Can you please review, and check that your tests still run successfully on RelToSqlConverter. It would also be good to have some tests that use the OFFSET and FETCH clauses.

          Show
          julianhyde Julian Hyde added a comment - Amogh Margoor , Thanks for the PR. I have done some work on it, eliminating duplicate code between JdbcRules and RelToSqlConverter by creating a new class SqlImplementor. I changed Context.ignoreCast to false, and found a different/better way to ignore trivial casts. I also incorporated fixes for CALCITE-970 and CALCITE-893 that had been missed in your copy of the code. See my https://github.com/julianhyde/calcite/commits/1003-rel-to-sql branch, in particular commit 69e20934. Can you please review, and check that your tests still run successfully on RelToSqlConverter. It would also be good to have some tests that use the OFFSET and FETCH clauses.
          Hide
          amargoor Amogh Margoor added a comment -

          Hi Julian Hyde
          I have raised PR for this issue: https://github.com/apache/calcite/pull/174/files
          Current utility handles basic cases, but does not handle specialized Rel Nodes like EnumerableLimit, JdbcToEnumerable etc.
          I think we can add more utilities that extend this basic utility (such as EnumerableToSql, JdbcRelToSql etc) and handle such specialized cases.
          I can add them after this change gets in, if that is the right way to do it.
          Thanks.

          Show
          amargoor Amogh Margoor added a comment - Hi Julian Hyde I have raised PR for this issue: https://github.com/apache/calcite/pull/174/files Current utility handles basic cases, but does not handle specialized Rel Nodes like EnumerableLimit, JdbcToEnumerable etc. I think we can add more utilities that extend this basic utility (such as EnumerableToSql, JdbcRelToSql etc) and handle such specialized cases. I can add them after this change gets in, if that is the right way to do it. Thanks.

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              amargoor Amogh Margoor
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development