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

JDBC adapter generates wrong SQL if UNION has more than two inputs

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 1.10.0
    • Fix Version/s: 1.12.0
    • Component/s: jdbc-adapter
    • Labels:
      None
    • Environment:

      NA

      Description

      JDBC adapter generates wrong SQL if set operation (UNION, INTERSECT or MINUS) has more than two inputs. In union example, after UnionMergeRule, the union input is convert to three input, and rel-to-sql convert result is wrong.

      input sql

       SELECT *
      FROM (SELECT \"product_id\"
      FROM \"foodmart\".\"product\"
      UNION ALL
      SELECT \"product_id\"
      FROM \"foodmart\".\"sales_fact_1997\")
      UNION ALL
      SELECT \"product_class_id\" AS \"PRODUCT_ID\"
      FROM \"foodmart\".\"product_class\"
       

      output sql

      SELECT \"product_id\"
      FROM \"foodmart\".\"product\"
      UNION ALL
      SELECT \"product_id\"
      FROM \"foodmart\".\"sales_fact_1997\"
       

      the last union query is lost.

        Activity

        Hide
        ransom Zhiqiang He added a comment -
        Show
        ransom Zhiqiang He added a comment - https://github.com/apache/calcite/pull/353 Julian Hyde please review it. thanks.
        Hide
        julianhyde Julian Hyde added a comment -

        Thanks for the PR; I reviewed it and the logic looks good, just a couple of "technical debt" items:

        • Is it necessary to call SqlPrettyWriter.setNeedWhitespace? The writer is supposed to add the necessary whitespace automatically. If it doesn't that is a bug in the writer.
        • Maybe SqlSetOperator should be a sub-class of SqlOperator rather than SqlBinaryOperator at present. Does that change have a big impact?
        • The okWithOptimizer method is really useful, but it would be even better if it could be invoked as a fluid API: rather than "sql(sql).okWithOptimizer(ruleSet, planner, expected)", people should be able to write "sql(sql).withOptimizer(ruleSet, planner).ok(expected)".
        Show
        julianhyde Julian Hyde added a comment - Thanks for the PR; I reviewed it and the logic looks good, just a couple of "technical debt" items: Is it necessary to call SqlPrettyWriter.setNeedWhitespace ? The writer is supposed to add the necessary whitespace automatically. If it doesn't that is a bug in the writer. Maybe SqlSetOperator should be a sub-class of SqlOperator rather than SqlBinaryOperator at present. Does that change have a big impact? The okWithOptimizer method is really useful, but it would be even better if it could be invoked as a fluid API: rather than "sql(sql).okWithOptimizer(ruleSet, planner, expected)", people should be able to write "sql(sql).withOptimizer(ruleSet, planner).ok(expected)".
        Hide
        julianhyde Julian Hyde added a comment -

        On second thoughts, SqlSetOperator should remain binary. SqlImplementor.setOpToSql should create a binary tree, (A union B) union C rather than a flat 3-way tree A union B union C. I think that means you can back out SqlSetOperator.unparse.

        Show
        julianhyde Julian Hyde added a comment - On second thoughts, SqlSetOperator should remain binary. SqlImplementor.setOpToSql should create a binary tree, (A union B) union C rather than a flat 3-way tree A union B union C . I think that means you can back out SqlSetOperator.unparse .
        Hide
        ransom Zhiqiang He added a comment -

        It was parse (A union B) union C befor Opimizer.
        but after UnionMergeRule optimizer rule executed. the rel expression was changed to A union B union C.

        Show
        ransom Zhiqiang He added a comment - It was parse (A union B) union C befor Opimizer. but after UnionMergeRule optimizer rule executed. the rel expression was changed to A union B union C.
        Hide
        julianhyde Julian Hyde added a comment -

        Yes. In the SqlNode AST, set-operators are binary operators; in RelNode land, set-operators are n-ary. (Same goes for AND and OR, by the way.)

        Let's not fight what's already working. Let's just fix the bug in SqlImplementor.setOpToSql.

        Show
        julianhyde Julian Hyde added a comment - Yes. In the SqlNode AST, set-operators are binary operators; in RelNode land, set-operators are n-ary. (Same goes for AND and OR, by the way.) Let's not fight what's already working. Let's just fix the bug in SqlImplementor.setOpToSql.
        Hide
        ransom Zhiqiang He added a comment -

        I'm already fix bug in SqlImplementor and changed reltosqltest to stream API. please review it, thanks.

        Show
        ransom Zhiqiang He added a comment - I'm already fix bug in SqlImplementor and changed reltosqltest to stream API. please review it, thanks.
        Hide
        ransom Zhiqiang He added a comment -

        Julian Hyde can you help me to review it ? thanks.

        Show
        ransom Zhiqiang He added a comment - Julian Hyde can you help me to review it ? thanks.
        Hide
        julianhyde Julian Hyde added a comment -
        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/89f11251 . Thanks for the PR, Zhiqiang He !
        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.12.0 (2017-03-24).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.12.0 (2017-03-24).

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            ransom Zhiqiang He
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development