[jira] [Commented] (PHOENIX-2120) Padding character is not inverted as required for DESC CHAR columns

["Samarth Jain (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/PHOENIX-2120?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14634257#comment-14634257 ] 

Samarth Jain commented on PHOENIX-2120:
---------------------------------------

In PTableImpl.java, the method rowKeyOrderOptimizable() should be called instead.
{code}
if (maxLength != null && type.isFixedWidth() && byteValue.length < maxLength) {
+                    if (rowKeyOrderOptimizable) {
+                        key.set(byteValue);
+                        type.pad(key, maxLength, sortOrder);
+                        byteValue = ByteUtil.copyKeyBytesIfNecessary(key);
+                    } 
{code}

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:
{code}
Properties props = new Properties();
            conn = DriverManager.getConnection(jdbcUrl, props)
                    .unwrap(PhoenixConnection.class);
{code}
{code}
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");
        }
{code}


Extremely minor nits:

remove TODO from @param bypassUpgrade TODO in 
{code}
private static boolean upgradeSharedIndex
public static void upgradeDescVarLengthRowKeys
{code}

Should the second comment be Upgrade local indexes?
{code}
                 // 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);
{code}

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:
{code}
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);                                        
                                     }
{code}



> Padding character is not inverted as required for DESC CHAR columns
> -------------------------------------------------------------------
>
>                 Key: PHOENIX-2120
>                 URL: https://issues.apache.org/jira/browse/PHOENIX-2120
>             Project: Phoenix
>          Issue Type: Improvement
>            Reporter: James Taylor
>         Attachments: PHOENIX-2120_v1.patch, PHOENIX-2120_v2.patch
>
>
> 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.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)