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

Incorrect trimming of CHAR when performing cast to VARCHAR

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.8.0
    • Component/s: core
    • Labels:
      None

      Description

      Assume the value ' ' with type CHAR(1) that needs to be cast to VARCHAR(10).

      The method makeCast (line 488 in RexBuilder.java) trims spaces on the right hand side of the CHAR value, but this seems incorrect as it could lead to incorrect results.

        Activity

        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment -

        Julian Hyde, what is your take? I think it is sufficient to add an additional check on the type to cast to (it should be CHAR) before trimming the value?

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - Julian Hyde , what is your take? I think it is sufficient to add an additional check on the type to cast to (it should be CHAR) before trimming the value?
        Hide
        julianhyde Julian Hyde added a comment -

        I quickly read the spec and it appears that CHAR is not to be trimmed when converting to VARCHAR. From 6.13 <cast specification>:

        c) If SD is fixed-length character string, variable-length character string, or large object character string, then
        Case:
        i) If the length in characters of SV is less than or equal to MLTD, then TV is SV.
        ii) If the length in characters of SV is larger than MLTD, then TV is the first MLTD characters of SV. If any of the remaining characters of SV are non-<space> characters, then a completion condition is raised: warning — string data, right truncation.

        So it looks if current behavior is incorrect. However, I do recall a bug with

        insert into t2 select ' ' as twoSpace from t union all select ' ' as threeSpace from t

        where the target column in t2 is a varchar(4). We should make sure not to break that again. The twoSpace value must not be padded to three spaces when it is implicitly converted from CHAR(2) to CHAR(3). IIRC it should be converted directly to a varchar(4).

        Show
        julianhyde Julian Hyde added a comment - I quickly read the spec and it appears that CHAR is not to be trimmed when converting to VARCHAR. From 6.13 <cast specification>: c) If SD is fixed-length character string, variable-length character string, or large object character string, then Case: i) If the length in characters of SV is less than or equal to MLTD, then TV is SV. ii) If the length in characters of SV is larger than MLTD, then TV is the first MLTD characters of SV. If any of the remaining characters of SV are non-<space> characters, then a completion condition is raised: warning — string data, right truncation. So it looks if current behavior is incorrect. However, I do recall a bug with insert into t2 select ' ' as twoSpace from t union all select ' ' as threeSpace from t where the target column in t2 is a varchar(4). We should make sure not to break that again. The twoSpace value must not be padded to three spaces when it is implicitly converted from CHAR(2) to CHAR(3). IIRC it should be converted directly to a varchar(4).
        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment -

        Julian Hyde, I have created a pull request in https://github.com/apache/calcite/pull/221 . Would you be able to review it? Thanks

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - Julian Hyde , I have created a pull request in https://github.com/apache/calcite/pull/221 . Would you be able to review it? Thanks
        Hide
        julianhyde Julian Hyde added a comment -

        I will try to get to it – swamped for the next day or two.

        Show
        julianhyde Julian Hyde added a comment - I will try to get to it – swamped for the next day or two.
        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment -

        No problem Julian. Thanks

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - No problem Julian. Thanks
        Hide
        julianhyde Julian Hyde added a comment -

        Jesus Camacho Rodriguez, I reviewed and added some tests. I concluded that your changes are OK, but we should be padding when converting to CHAR. You can see the tests and fixes in my branch https://github.com/julianhyde/calcite/tree/1199-padding. Can you review, and if it looks OK rebase, squash my commit into yours and push.

        Show
        julianhyde Julian Hyde added a comment - Jesus Camacho Rodriguez , I reviewed and added some tests. I concluded that your changes are OK, but we should be padding when converting to CHAR. You can see the tests and fixes in my branch https://github.com/julianhyde/calcite/tree/1199-padding . Can you review, and if it looks OK rebase, squash my commit into yours and push.
        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment -

        Julian Hyde, thanks a lot. I did not have time to check this yet; I will review, squash and commit by tomorrow.

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - Julian Hyde , thanks a lot. I did not have time to check this yet; I will review, squash and commit by tomorrow.
        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/ebb68da . Thanks Julian Hyde
        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.8.0 (2016-06-13).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.8.0 (2016-06-13).

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            jcamachorodriguez Jesus Camacho Rodriguez
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development