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

Give error if ORDER BY clause references ambiguous column name

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.11.0
    • Fix Version/s: 1.12.0
    • Component/s: None
    • Labels:
      None

      Description

      When I use calcite, I found follow sql can convert to a RelNode correctly.

       
      SELECT id AS t1, name AS t1 FROM T ORDER BY t1
      

      The column name t1 for ORDER BY is ambiguous, should check at validation?

        Activity

        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).
        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/c3288a14 . Thanks for the PR, zhen wang !
        Hide
        julianhyde Julian Hyde added a comment -

        Latest revision, with changes to SqlValidatorTest, here: https://github.com/julianhyde/calcite/commit/cd23bf06b5aadcb7afed52015e45ea49188f4c52

        Show
        julianhyde Julian Hyde added a comment - Latest revision, with changes to SqlValidatorTest, here: https://github.com/julianhyde/calcite/commit/cd23bf06b5aadcb7afed52015e45ea49188f4c52
        Hide
        julianhyde Julian Hyde added a comment -

        it's just when are the two earlier unit tests supposed to be fixed?

        The rule is that if there is one column in the SELECT clause whose alias is "foo" and one or more of the tables in the FROM clause have a column called "foo", we reference the alias, not the columns. (I'm not 100% sure what the SQL standard says, but this policy seems right to me.) So I think the plans are valid. I'll remove the TODOs.

        Show
        julianhyde Julian Hyde added a comment - it's just when are the two earlier unit tests supposed to be fixed? The rule is that if there is one column in the SELECT clause whose alias is "foo" and one or more of the tables in the FROM clause have a column called "foo", we reference the alias, not the columns. (I'm not 100% sure what the SQL standard says, but this policy seems right to me.) So I think the plans are valid. I'll remove the TODOs.
        Hide
        zhenw zhen wang added a comment -

        I'm good. not that familiar with sql spec.

        it's just when are the two earlier unit tests supposed to be fixed?
        if we treat 'explicit' alias the same as `implicit` alias, then feels the two will never be fixed. or rather should they be fixed as comments suggests in the first place.

        Show
        zhenw zhen wang added a comment - I'm good. not that familiar with sql spec. it's just when are the two earlier unit tests supposed to be fixed? if we treat 'explicit' alias the same as `implicit` alias, then feels the two will never be fixed. or rather should they be fixed as comments suggests in the first place.
        Hide
        julianhyde Julian Hyde added a comment -

        Looks good. I added some extra tests.

        I disagree with you on a couple of minor points:

        • We should use nameMatcher, so that case-insensitive sessions get case-insensitive matches;
        • I don't think we should treat columns with explicit aliases ("AS name") differently than ones with implicit aliases ("t.name").

        But I do agree that columns in the SELECT clause override this that are in tables in the FROM clause. For example, SELECT dept.deptno FROM dept, emp ORDER BY deptno is valid, because though both emp and dept have a column called deptno, there is only one deptno column in the SELECT clause.

        Please review my fix up in https://github.com/julianhyde/calcite/tree/1535-ambiguous-order-by.

        Show
        julianhyde Julian Hyde added a comment - Looks good. I added some extra tests. I disagree with you on a couple of minor points: We should use nameMatcher, so that case-insensitive sessions get case-insensitive matches; I don't think we should treat columns with explicit aliases ("AS name") differently than ones with implicit aliases ("t.name"). But I do agree that columns in the SELECT clause override this that are in tables in the FROM clause. For example, SELECT dept.deptno FROM dept, emp ORDER BY deptno is valid, because though both emp and dept have a column called deptno , there is only one deptno column in the SELECT clause. Please review my fix up in https://github.com/julianhyde/calcite/tree/1535-ambiguous-order-by .
        Hide
        zhenw zhen wang added a comment -
        Show
        zhenw zhen wang added a comment - https://github.com/apache/calcite/pull/359 Julian Hyde please review
        Hide
        julianhyde Julian Hyde added a comment -

        It's OK for column aliases to be non-unique (for a top-level query) but if you reference a non-unique column name in an ORDER BY clause I agree it should give an error, say "Ambiguous column name 'T1' in ORDER BY".

        Show
        julianhyde Julian Hyde added a comment - It's OK for column aliases to be non-unique (for a top-level query) but if you reference a non-unique column name in an ORDER BY clause I agree it should give an error, say "Ambiguous column name 'T1' in ORDER BY".

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            dongming Dongming Liu
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development