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

Relbuilder .union(...) should use parentheses after .sortLimit(...) is called

    Details

    • Type: Bug
    • Status: Open
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: 1.13.0
    • Fix Version/s: None
    • Component/s: jdbc-adapter
    • Labels:
    • Environment:

      Java 7
      H2 version 1.4.195
      Calcite version 1.13.0

      Description

      I'm trying to build up multiple queries separately as RelNode , then call

      builder.pushAll(unionParts).union(true);

      to union the results in SQL, but I end up with
      org.h2.jdbc.JdbcSQLException: Syntax error in SQL statement

      It turns out to be an issue with lack of parentheses between calls to UNION ALL

      I've done some digging and according to the mysql docs the union call needs to have these RelNode's (or their equivalent sql) wrapped in parentheses when using ORDER BY and LIMIT which is
      .sortLimit(offset, fetch, builder.field("TEST_VALUE"))
      with the RelBuilder

      I've been able to resolve this by converting each RelNode to SQL and manually wrapping it in parentheses between calls to UNION ALL before executing the statement against H2
      Like so: (<insert query>) UNION ALL (<insert query>)

      Here's an example I made to show exactly what's happening kevinhinterlong/CalciteUnionError

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        Sounds valid. It looks to be a problem with the MySQL-specific SQL generation, i.e. RelToSqlConverter and SqlNode.unparse, rather than RelBuilder. RelBuilder is doing its job correctly, and giving the correct RelNode tree. I saw your example, but it should be possible to reproduce this with a fairly concise test case in RelToSqlConverterTest.

        Do other SQL dialects (e.g. PostgreSQL) require parentheses too?

        Show
        julianhyde Julian Hyde added a comment - Sounds valid. It looks to be a problem with the MySQL-specific SQL generation, i.e. RelToSqlConverter and SqlNode.unparse , rather than RelBuilder . RelBuilder is doing its job correctly, and giving the correct RelNode tree. I saw your example, but it should be possible to reproduce this with a fairly concise test case in RelToSqlConverterTest . Do other SQL dialects (e.g. PostgreSQL) require parentheses too?
        Hide
        kevinhinterlong Kevin Hinterlong added a comment -

        Ok so it looks like this is something that is supported by MySQL (and PostgreSQL, not sure what else), but Standard SQL doesn’t allow this. So for example SQLite doesn’t support it.

        Here’s a relevant stackoverflow

        So it looks like the best portable way to accomplish this would be to make multiple queries.

        Show
        kevinhinterlong Kevin Hinterlong added a comment - Ok so it looks like this is something that is supported by MySQL (and PostgreSQL, not sure what else), but Standard SQL doesn’t allow this. So for example SQLite doesn’t support it. Here’s a relevant stackoverflow So it looks like the best portable way to accomplish this would be to make multiple queries.
        Hide
        julianhyde Julian Hyde added a comment -

        I don't agree with your interpretation of that Stack Overflow post. Parentheses around SELECT expressions are very useful to override the usual precedence of UNION, INTERSECT and EXCEPT, support for them is required by the SQL standard (even if SQLLite doesn't support them), and many databases do support them.

        Calcite's parser also supports them. See SqlParserTest.testOrderInternal, for instance.

        So, I think the JDBC adapter should generate SQL with parentheses if by default. If there are particular dialects that don't support them, let's cross that bridge when we get to it.

        Show
        julianhyde Julian Hyde added a comment - I don't agree with your interpretation of that Stack Overflow post. Parentheses around SELECT expressions are very useful to override the usual precedence of UNION, INTERSECT and EXCEPT, support for them is required by the SQL standard (even if SQLLite doesn't support them), and many databases do support them. Calcite's parser also supports them. See SqlParserTest.testOrderInternal, for instance. So, I think the JDBC adapter should generate SQL with parentheses if by default. If there are particular dialects that don't support them, let's cross that bridge when we get to it.
        Hide
        kevinhinterlong Kevin Hinterlong added a comment -

        I agree that support for them is required by the SQL standard, the "this" I was referring to in my comment mean having an ORDER BY/LIMIT on each of the subqueries being unioned together.

        I was referring to this part of the answer from the Stack Overflow post.

        standard SQL only allows a ORDER BY for the final result

        Show
        kevinhinterlong Kevin Hinterlong added a comment - I agree that support for them is required by the SQL standard, the "this" I was referring to in my comment mean having an ORDER BY / LIMIT on each of the subqueries being unioned together. I was referring to this part of the answer from the Stack Overflow post. standard SQL only allows a ORDER BY for the final result
        Hide
        julianhyde Julian Hyde added a comment -

        Ah, thanks for clarifying. Could we change the builder to generate your "[Success] Manual" query,

        (SELECT *
        FROM "PUBLIC"."TEST"
        ORDER BY "TEST_VALUE" NULLS LAST
        OFFSET 10 ROWS
        FETCH NEXT 20 ROWS ONLY)
        UNION ALL
        (SELECT *
        FROM "PUBLIC"."TEST"
        ORDER BY "TEST_VALUE" NULLS LAST
        FETCH NEXT 10 ROWS ONLY)
        

        I think that will work for MySQL, PostgreSQL, H2, Oracle and several others.

        Show
        julianhyde Julian Hyde added a comment - Ah, thanks for clarifying. Could we change the builder to generate your " [Success] Manual" query, (SELECT * FROM "PUBLIC" . "TEST" ORDER BY "TEST_VALUE" NULLS LAST OFFSET 10 ROWS FETCH NEXT 20 ROWS ONLY) UNION ALL (SELECT * FROM "PUBLIC" . "TEST" ORDER BY "TEST_VALUE" NULLS LAST FETCH NEXT 10 ROWS ONLY) I think that will work for MySQL, PostgreSQL, H2, Oracle and several others.
        Hide
        kevinhinterlong Kevin Hinterlong added a comment - - edited

        I think that would work for most cases. The only one I know which wouldn't work is SQLite which needs something like below (taken from one of the answers in the Stack Overflow post, I don't use or need support for SQLite)

            SELECT * FROM (SELECT *
            FROM "PUBLIC"."TEST"
            ORDER BY "TEST_VALUE"
            LIMIT 1
            OFFSET 0)
            UNION ALL
            SELECT * FROM (SELECT *
            FROM "PUBLIC"."TEST"
            ORDER BY "TEST_VALUE"
            LIMIT 2
            OFFSET 1)
        
        Show
        kevinhinterlong Kevin Hinterlong added a comment - - edited I think that would work for most cases. The only one I know which wouldn't work is SQLite which needs something like below (taken from one of the answers in the Stack Overflow post, I don't use or need support for SQLite) SELECT * FROM ( SELECT * FROM "PUBLIC" . "TEST" ORDER BY "TEST_VALUE" LIMIT 1 OFFSET 0) UNION ALL SELECT * FROM ( SELECT * FROM "PUBLIC" . "TEST" ORDER BY "TEST_VALUE" LIMIT 2 OFFSET 1)

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            kevinhinterlong Kevin Hinterlong
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development