Uploaded image for project: 'Phoenix'
  1. Phoenix
  2. PHOENIX-2120

Padding character is not inverted as required for DESC CHAR columns

    Details

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

      Description

      Looks like the padding character (a space char) is not inverted as it should be for DESC CHAR columns. This can lead to rows not being in the correct order (similar to PHOENIX-2067). If an application does not rely on auto padding, they will not be affected.

      1. PHOENIX-2120_v1.patch
        43 kB
        James Taylor
      2. PHOENIX-2120_v2.patch
        73 kB
        James Taylor
      3. PHOENIX-2120_v3.patch
        74 kB
        James Taylor
      4. PHOENIX-2120_addendum1-wip.patch
        11 kB
        James Taylor
      5. PHOENIX-2120_addendum_v1.patch
        66 kB
        James Taylor
      6. PHOENIX-2120_addendum_v2.patch
        78 kB
        James Taylor

        Activity

        Hide
        jamestaylor James Taylor added a comment -
        Show
        jamestaylor James Taylor added a comment - Please review, Dumindu Buddhika , ramkrishna.s.vasudevan , and/or Samarth Jain .
        Hide
        jamestaylor James Taylor added a comment -

        Slightly enhanced version to use correct padding character for BINARY ascending and descending columns.

        Show
        jamestaylor James Taylor added a comment - Slightly enhanced version to use correct padding character for BINARY ascending and descending columns.
        Hide
        samarthjain Samarth Jain added a comment -

        In PTableImpl.java, the method rowKeyOrderOptimizable() should be called instead.

        if (maxLength != null && type.isFixedWidth() && byteValue.length < maxLength) {
        +                    if (rowKeyOrderOptimizable) {
        +                        key.set(byteValue);
        +                        type.pad(key, maxLength, sortOrder);
        +                        byteValue = ByteUtil.copyKeyBytesIfNecessary(key);
        +                    } 
        

        In UpgradeUtil#upgradeDescVarLengthRowKeys, is it possible for the connection to have scn and tenant id properties set? I see that the only caller of this method is PhoenixRuntime and there we are getting the connection using:

        Properties props = new Properties();
                    conn = DriverManager.getConnection(jdbcUrl, props)
                            .unwrap(PhoenixConnection.class);
        
        public static void upgradeDescVarLengthRowKeys(PhoenixConnection conn, List<String> tablesToUpgrade) throws SQLException {
                if (conn.getClientInfo(PhoenixRuntime.CURRENT_SCN_ATTRIB) != null) {
                    throw new SQLException("May not specify the CURRENT_SCN property when upgrading");
                }
                if (conn.getClientInfo(PhoenixRuntime.TENANT_ID_ATTRIB) != null) {
                    throw new SQLException("May not specify the TENANT_ID_ATTRIB property when upgrading");
                }
        

        Extremely minor nits:

        remove TODO from @param bypassUpgrade TODO in

        private static boolean upgradeSharedIndex
        public static void upgradeDescVarLengthRowKeys
        

        Should the second comment be Upgrade local indexes?

                         // Upgrade view indexes
        -                wasUpgraded |= upgradeSharedIndex(upgradeConn, conn, sharedViewIndexName);
        +                wasUpgraded |= upgradeSharedIndex(upgradeConn, conn, sharedViewIndexName, bypassUpgrade);
                         String sharedLocalIndexName = Bytes.toString(MetaDataUtil.getLocalIndexPhysicalName(table.getName().getBytes()));
                         // Upgrade view indexes
        -                wasUpgraded |= upgradeSharedIndex(upgradeConn, conn, sharedLocalIndexName);
        +                wasUpgraded |= upgradeSharedIndex(upgradeConn, conn, sharedLocalIndexName, bypassUpgrade);
        

        A general note - considering this upgrade is user driven, it would be good to include the information in release notes along with examples. Especially for this case where the trailing spaces could possibly be legitimate:

        else if (field.getDataType() == PBinary.INSTANCE) {
        +                                        // Remove trailing space characters so that the setValues call below will replace them
        +                                        // with the correct zero byte character. Note this is somewhat dangerous as these
        +                                        // could be legit, but I don't know what the alternative is.
        +                                        int len = ptr.getLength();
        +                                        while (len > 0 && ptr.get()[ptr.getOffset() + len - 1] == StringUtil.SPACE_UTF8) {
        +                                            len--;
        +                                        }
        +                                        ptr.set(ptr.get(), ptr.getOffset(), len);                                        
                                             }
        
        Show
        samarthjain Samarth Jain added a comment - In PTableImpl.java, the method rowKeyOrderOptimizable() should be called instead. if (maxLength != null && type.isFixedWidth() && byteValue.length < maxLength) { + if (rowKeyOrderOptimizable) { + key.set(byteValue); + type.pad(key, maxLength, sortOrder); + byteValue = ByteUtil.copyKeyBytesIfNecessary(key); + } In UpgradeUtil#upgradeDescVarLengthRowKeys, is it possible for the connection to have scn and tenant id properties set? I see that the only caller of this method is PhoenixRuntime and there we are getting the connection using: Properties props = new Properties(); conn = DriverManager.getConnection(jdbcUrl, props) .unwrap(PhoenixConnection.class); public static void upgradeDescVarLengthRowKeys(PhoenixConnection conn, List< String > tablesToUpgrade) throws SQLException { if (conn.getClientInfo(PhoenixRuntime.CURRENT_SCN_ATTRIB) != null ) { throw new SQLException( "May not specify the CURRENT_SCN property when upgrading" ); } if (conn.getClientInfo(PhoenixRuntime.TENANT_ID_ATTRIB) != null ) { throw new SQLException( "May not specify the TENANT_ID_ATTRIB property when upgrading" ); } Extremely minor nits: remove TODO from @param bypassUpgrade TODO in private static boolean upgradeSharedIndex public static void upgradeDescVarLengthRowKeys Should the second comment be Upgrade local indexes? // Upgrade view indexes - wasUpgraded |= upgradeSharedIndex(upgradeConn, conn, sharedViewIndexName); + wasUpgraded |= upgradeSharedIndex(upgradeConn, conn, sharedViewIndexName, bypassUpgrade); String sharedLocalIndexName = Bytes.toString(MetaDataUtil.getLocalIndexPhysicalName(table.getName().getBytes())); // Upgrade view indexes - wasUpgraded |= upgradeSharedIndex(upgradeConn, conn, sharedLocalIndexName); + wasUpgraded |= upgradeSharedIndex(upgradeConn, conn, sharedLocalIndexName, bypassUpgrade); A general note - considering this upgrade is user driven, it would be good to include the information in release notes along with examples. Especially for this case where the trailing spaces could possibly be legitimate: else if (field.getDataType() == PBinary.INSTANCE) { + // Remove trailing space characters so that the setValues call below will replace them + // with the correct zero byte character. Note this is somewhat dangerous as these + // could be legit, but I don't know what the alternative is. + int len = ptr.getLength(); + while (len > 0 && ptr.get()[ptr.getOffset() + len - 1] == StringUtil.SPACE_UTF8) { + len--; + } + ptr.set(ptr.get(), ptr.getOffset(), len); }
        Hide
        jamestaylor James Taylor added a comment -

        Thanks for the review, Samarth Jain. I've updated the patch with your recommendations. I'll definitely write up a release note that describes how to find out if you need to upgrade tables and why you'd want to upgrade them.

        Show
        jamestaylor James Taylor added a comment - Thanks for the review, Samarth Jain . I've updated the patch with your recommendations. I'll definitely write up a release note that describes how to find out if you need to upgrade tables and why you'd want to upgrade them.
        Hide
        samarthjain Samarth Jain added a comment -

        LGTM, +1.

        Show
        samarthjain Samarth Jain added a comment - LGTM, +1.
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Phoenix-master #846 (See https://builds.apache.org/job/Phoenix-master/846/)
        PHOENIX-2120 Padding character is not inverted as required for DESC CHAR columns (jamestaylor: rev dcf845c25eb9bd619a999d16ce9e2f548ce7b491)

        • phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java
        • phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/LpadFunctionIT.java
        • phoenix-core/src/main/java/org/apache/phoenix/util/StringUtil.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/LiteralExpression.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/function/RTrimFunction.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PBinary.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PChar.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java
        • phoenix-core/src/main/java/org/apache/phoenix/util/PhoenixRuntime.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDataType.java
        • phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/KeyPart.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/function/PrefixFunction.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/function/InvertFunction.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundDateExpression.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundDecimalExpression.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Phoenix-master #846 (See https://builds.apache.org/job/Phoenix-master/846/ ) PHOENIX-2120 Padding character is not inverted as required for DESC CHAR columns (jamestaylor: rev dcf845c25eb9bd619a999d16ce9e2f548ce7b491) phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java phoenix-core/src/it/java/org/apache/phoenix/end2end/LpadFunctionIT.java phoenix-core/src/main/java/org/apache/phoenix/util/StringUtil.java phoenix-core/src/main/java/org/apache/phoenix/expression/LiteralExpression.java phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java phoenix-core/src/main/java/org/apache/phoenix/expression/function/RTrimFunction.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PBinary.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PChar.java phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java phoenix-core/src/main/java/org/apache/phoenix/util/PhoenixRuntime.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDataType.java phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java phoenix-core/src/main/java/org/apache/phoenix/compile/KeyPart.java phoenix-core/src/main/java/org/apache/phoenix/expression/function/PrefixFunction.java phoenix-core/src/main/java/org/apache/phoenix/expression/function/InvertFunction.java phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundDateExpression.java phoenix-core/src/main/java/org/apache/phoenix/expression/function/RoundDecimalExpression.java
        Hide
        jamestaylor James Taylor added a comment -

        Found a few issues wrt LIKE and SUBSTR with DESC columns which this patch addresses. Still needs a few more tweaks, though, so parking it here for now.

        Show
        jamestaylor James Taylor added a comment - Found a few issues wrt LIKE and SUBSTR with DESC columns which this patch addresses. Still needs a few more tweaks, though, so parking it here for now.
        Hide
        jamestaylor James Taylor added a comment -

        Patch that fixes SUBSTR and LIKE issues related to DESC columns having a trailing \xFF byte. Also, passes through rowKeyOrderOptimizable where necessary so that we continue to get the old behavior for existing tables.

        Thomas D'Silva - would you have time to review?

        Show
        jamestaylor James Taylor added a comment - Patch that fixes SUBSTR and LIKE issues related to DESC columns having a trailing \xFF byte. Also, passes through rowKeyOrderOptimizable where necessary so that we continue to get the old behavior for existing tables. Thomas D'Silva - would you have time to review?
        Hide
        tdsilva Thomas D'Silva added a comment -

        Sure I will review it.

        Show
        tdsilva Thomas D'Silva added a comment - Sure I will review it.
        Hide
        jamestaylor James Taylor added a comment -

        Final patch

        Show
        jamestaylor James Taylor added a comment - Final patch
        Hide
        tdsilva Thomas D'Silva added a comment -

        +1 LGTM

        Show
        tdsilva Thomas D'Silva added a comment - +1 LGTM
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Phoenix-master #851 (See https://builds.apache.org/job/Phoenix-master/851/)
        PHOENIX-2120 Padding character is not inverted as required for DESC CHAR columns (jtaylor: rev 498cc5524ff73c3947160ddfc4241f759c7044f2)

        • phoenix-core/src/it/java/org/apache/phoenix/end2end/BaseQueryIT.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/BaseExpression.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/RTrimFunctionIT.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/ExpressionCompiler.java
        • phoenix-core/src/main/java/org/apache/phoenix/execute/HashJoinPlan.java
        • phoenix-core/src/test/java/org/apache/phoenix/query/BaseTest.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/InListIT.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/function/InvertFunction.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/ComparisonExpression.java
        • phoenix-core/src/test/java/org/apache/phoenix/util/TestUtil.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/CoerceExpression.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/InListExpression.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/visitor/CloneExpressionVisitor.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PArrayDataType.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/function/PrefixFunction.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/LiteralExpression.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/ArrayConstructorExpression.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/function/RTrimFunction.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/SortOrderIT.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Phoenix-master #851 (See https://builds.apache.org/job/Phoenix-master/851/ ) PHOENIX-2120 Padding character is not inverted as required for DESC CHAR columns (jtaylor: rev 498cc5524ff73c3947160ddfc4241f759c7044f2) phoenix-core/src/it/java/org/apache/phoenix/end2end/BaseQueryIT.java phoenix-core/src/main/java/org/apache/phoenix/expression/BaseExpression.java phoenix-core/src/it/java/org/apache/phoenix/end2end/RTrimFunctionIT.java phoenix-core/src/main/java/org/apache/phoenix/compile/ExpressionCompiler.java phoenix-core/src/main/java/org/apache/phoenix/execute/HashJoinPlan.java phoenix-core/src/test/java/org/apache/phoenix/query/BaseTest.java phoenix-core/src/it/java/org/apache/phoenix/end2end/InListIT.java phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java phoenix-core/src/main/java/org/apache/phoenix/expression/function/InvertFunction.java phoenix-core/src/main/java/org/apache/phoenix/expression/ComparisonExpression.java phoenix-core/src/test/java/org/apache/phoenix/util/TestUtil.java phoenix-core/src/main/java/org/apache/phoenix/expression/CoerceExpression.java phoenix-core/src/main/java/org/apache/phoenix/expression/InListExpression.java phoenix-core/src/main/java/org/apache/phoenix/expression/visitor/CloneExpressionVisitor.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PArrayDataType.java phoenix-core/src/main/java/org/apache/phoenix/expression/function/PrefixFunction.java phoenix-core/src/main/java/org/apache/phoenix/expression/LiteralExpression.java phoenix-core/src/main/java/org/apache/phoenix/expression/ArrayConstructorExpression.java phoenix-core/src/main/java/org/apache/phoenix/expression/function/RTrimFunction.java phoenix-core/src/it/java/org/apache/phoenix/end2end/SortOrderIT.java
        Hide
        enis Enis Soztutar added a comment -

        Bulk close of all issues that has been resolved in a released version.

        Show
        enis Enis Soztutar added a comment - Bulk close of all issues that has been resolved in a released version.

          People

          • Assignee:
            jamestaylor James Taylor
            Reporter:
            jamestaylor James Taylor
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development