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

Sort order incorrect for variable length DESC columns

    Details

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

      HBase 0.98.6-cdh5.3.0
      jdk1.7.0_67 x64
      CentOS release 6.4 (2.6.32-358.el6.x86_64)

      Description

      Steps to reproduce:
      1. Create a table:
      CREATE TABLE mytable (id BIGINT not null PRIMARY KEY, timestamp BIGINT, log_message varchar) IMMUTABLE_ROWS=true, SALT_BUCKETS=16;
      2. Create two indexes:
      CREATE INDEX mytable_index_search ON mytable(timestamp,id) INCLUDE (log_message) SALT_BUCKETS=16;
      CREATE INDEX mytable_index_search_desc ON mytable(timestamp DESC,id DESC) INCLUDE (log_message) SALT_BUCKETS=16;
      3. Upsert values:
      UPSERT INTO mytable VALUES(1, 1434983826018, 'message1');
      UPSERT INTO mytable VALUES(2, 1434983826100, 'message2');
      UPSERT INTO mytable VALUES(3, 1434983826101, 'message3');
      UPSERT INTO mytable VALUES(4, 1434983826202, 'message4');
      4. Sort DESC by timestamp:
      select timestamp,id,log_message from mytable ORDER BY timestamp DESC;

      Failure: data is sorted incorrectly. In case when we have two longs which are different only by last two digits (e.g. 1434983826155, 1434983826100) and one of the long ends with '00' we receive incorrect order.
      Sorting result:
      1434983826202
      1434983826100
      1434983826101
      1434983826018

      1. PHOENIX-2067_4.4.1.patch
        532 kB
        Rajeshbabu Chintaguntla
      2. PHOENIX-2067-tests2.patch
        28 kB
        Dumindu Buddhika
      3. PHOENIX-2067-tests.patch
        24 kB
        Dumindu Buddhika
      4. PHOENIX-2067_addendum.patch
        2 kB
        James Taylor
      5. PHOENIX-2067_array_addendum_v2.patch
        290 kB
        James Taylor
      6. PHOENIX-2067_array_addendum.patch
        11 kB
        James Taylor
      7. PHOENIX-2067_v3.patch
        262 kB
        James Taylor
      8. PHOENIX-2067_v2.patch
        259 kB
        James Taylor
      9. PHOENIX-2067_v1.patch
        261 kB
        James Taylor

        Activity

        Hide
        mykola.komarnytskyy Mykola Komarnytskyy added a comment -

        James Taylor, please take a look at this issue.

        Show
        mykola.komarnytskyy Mykola Komarnytskyy added a comment - James Taylor , please take a look at this issue.
        Hide
        jamestaylor James Taylor added a comment -

        This bug potentially affects any PK column of variable length that is declared as DESC sort order. The particular case is when one value is a subpart of another value. Simplest case would be this:

        CREATE TABLE t (k VARCHAR DESC PRIMARY KEY);
        UPSERT INTO t VALUES ('a');
        UPSERT INTO t VALUES('ab');
        SELECT * FROM t ORDER BY k;
        

        The 'ab' row should appear before the 'a' row. This is due to there being no terminator at the end of the row key as well as because we're not inverting the null/zero terminator/separator byte between parts of the row key.

        The fix is to:

        • use a 255 byte separator byte instead of a 0 byte separator between row key parts
        • include a 255 byte terminator at the end of the row key
        • include a 255 byte for any null value at the end of the row key (without this, row keys with null values in the middle of the row key might sort before a row key with null values at the end of the row key)
        • disallow a DESC pk column to be added to the PK in an ALTER TABLE <table> ADD <column> as it would require updating existing data.

        We need a script that users can run to fix their existing data (or at least identify that there's an issue).

        Show
        jamestaylor James Taylor added a comment - This bug potentially affects any PK column of variable length that is declared as DESC sort order. The particular case is when one value is a subpart of another value. Simplest case would be this: CREATE TABLE t (k VARCHAR DESC PRIMARY KEY); UPSERT INTO t VALUES ('a'); UPSERT INTO t VALUES('ab'); SELECT * FROM t ORDER BY k; The 'ab' row should appear before the 'a' row. This is due to there being no terminator at the end of the row key as well as because we're not inverting the null/zero terminator/separator byte between parts of the row key. The fix is to: use a 255 byte separator byte instead of a 0 byte separator between row key parts include a 255 byte terminator at the end of the row key include a 255 byte for any null value at the end of the row key (without this, row keys with null values in the middle of the row key might sort before a row key with null values at the end of the row key) disallow a DESC pk column to be added to the PK in an ALTER TABLE <table> ADD <column> as it would require updating existing data. We need a script that users can run to fix their existing data (or at least identify that there's an issue).
        Hide
        jamestaylor James Taylor added a comment -

        WIP patch. Unit tests pass along with new ones that demonstrate the issue.

        Show
        jamestaylor James Taylor added a comment - WIP patch. Unit tests pass along with new ones that demonstrate the issue.
        Hide
        jamestaylor James Taylor added a comment -

        Parking partial patch

        Show
        jamestaylor James Taylor added a comment - Parking partial patch
        Hide
        jamestaylor James Taylor added a comment -

        More WIP. With just a few test failures, but no upgrade or conditional optimization for existing data. This is with nulls last when DESC, but there's a problem with this - we'd need to include trailing nulls until the last DESC row key column and you wouldn't be able to add a new DESC row key column without mucking with the data (which is a showstopper).

        I'm going to instead use a null separator with DESC for null values and otherwise a 0xFF. That way, nulls will sort first for ASC and DESC, but DESC sort order will work for all values.

        Show
        jamestaylor James Taylor added a comment - More WIP. With just a few test failures, but no upgrade or conditional optimization for existing data. This is with nulls last when DESC, but there's a problem with this - we'd need to include trailing nulls until the last DESC row key column and you wouldn't be able to add a new DESC row key column without mucking with the data (which is a showstopper). I'm going to instead use a null separator with DESC for null values and otherwise a 0xFF. That way, nulls will sort first for ASC and DESC, but DESC sort order will work for all values.
        Hide
        jamestaylor James Taylor added a comment -

        Another WIP patch. All unit tests passing except one.

        Show
        jamestaylor James Taylor added a comment - Another WIP patch. All unit tests passing except one.
        Hide
        jamestaylor James Taylor added a comment -

        All unit tests passing.

        Show
        jamestaylor James Taylor added a comment - All unit tests passing.
        Hide
        jamestaylor James Taylor added a comment -

        Includes upgrade ability and warning message

        Show
        jamestaylor James Taylor added a comment - Includes upgrade ability and warning message
        Hide
        jamestaylor James Taylor added a comment -

        With upgrade

        Show
        jamestaylor James Taylor added a comment - With upgrade
        Hide
        jamestaylor James Taylor added a comment -

        Fix OrderedResultIterator for DESC variable length data

        Show
        jamestaylor James Taylor added a comment - Fix OrderedResultIterator for DESC variable length data
        Hide
        jamestaylor James Taylor added a comment -

        Working upgrade, but still needs some refinement.

        Show
        jamestaylor James Taylor added a comment - Working upgrade, but still needs some refinement.
        Hide
        jamestaylor James Taylor added a comment -

        Refined upgrade code

        Show
        jamestaylor James Taylor added a comment - Refined upgrade code
        Hide
        jamestaylor James Taylor added a comment -

        Working and tested patch

        Show
        jamestaylor James Taylor added a comment - Working and tested patch
        Hide
        jamestaylor James Taylor added a comment -

        Samarth Jain - please review. Here's an overview:

        • No change in behavior for existing tables. Queries that have an ORDER BY for a variable length, descending row key will now sort correctly, but at the expense of forcing an ORDER BY (since they aren't sorted correctly in their natural order, we can't optimize out the ORDER BY).
        • New tables (or indexes) will use the correct separator for DESC variable length row keys, so they won't be hit with the ORDER BY cost. See SchemaUtil.getSeparatorByte() for an overview of the logic to determine the separator byte.
        • A new utility (psql.py -u option) is available to 1) display the physical tables affected by this bug, and 2) to optionally re-write them so that they sort correctly.
        Show
        jamestaylor James Taylor added a comment - Samarth Jain - please review. Here's an overview: No change in behavior for existing tables. Queries that have an ORDER BY for a variable length, descending row key will now sort correctly, but at the expense of forcing an ORDER BY (since they aren't sorted correctly in their natural order, we can't optimize out the ORDER BY). New tables (or indexes) will use the correct separator for DESC variable length row keys, so they won't be hit with the ORDER BY cost. See SchemaUtil.getSeparatorByte() for an overview of the logic to determine the separator byte. A new utility (psql.py -u option) is available to 1) display the physical tables affected by this bug, and 2) to optionally re-write them so that they sort correctly.
        Hide
        jamestaylor James Taylor added a comment -

        As far as testing the upgrade, I did a bunch of manual testing:

        • Created table with descending, variable length row key, table without but with an index that has one, table without but with a view that has a local and shared index, multi-tenant tables against the same.
        • Verified correct physical tables identified that need upgrading.
        • Verified upgrade worked correctly for all of above
        Show
        jamestaylor James Taylor added a comment - As far as testing the upgrade, I did a bunch of manual testing: Created table with descending, variable length row key, table without but with an index that has one, table without but with a view that has a local and shared index, multi-tenant tables against the same. Verified correct physical tables identified that need upgrading. Verified upgrade worked correctly for all of above
        Hide
        jamestaylor James Taylor added a comment -

        Fixed test failure due to this change for adding column to base table. Also disallowing VARBINARY DESC which can't really work correctly since we can't control what bytes are used and thus cannot guarantee the sort order is correct. A workaround for users would be to upsert using INVERT which is the same as what would occur today. Shorter but equal byte values would sort above longer equal byte values, though.

        Show
        jamestaylor James Taylor added a comment - Fixed test failure due to this change for adding column to base table. Also disallowing VARBINARY DESC which can't really work correctly since we can't control what bytes are used and thus cannot guarantee the sort order is correct. A workaround for users would be to upsert using INVERT which is the same as what would occur today. Shorter but equal byte values would sort above longer equal byte values, though.
        Hide
        jamestaylor James Taylor added a comment -

        Addendum patch for handling DESC ARRAY. Still needs a few more tweaks, but parking here for now.

        Show
        jamestaylor James Taylor added a comment - Addendum patch for handling DESC ARRAY. Still needs a few more tweaks, but parking here for now.
        Hide
        jamestaylor James Taylor added a comment -

        Dumindu Buddhika and ramkrishna.s.vasudevan - would you guys mind reviewing this patch? This ensures that descending, variable length arrays sort correctly. The change is the use a 255 byte as the separator for non null values (including the terminators). See the couple of new tests in ArrayIT.

        Much of the type changes are just formatting and moving a couple of duplicated methods where they belong at the base type. The other changes are to ensure we keep the same separator - for example if a table has not been upgraded, we need to keep using the 0 byte separator. That's where most of the complication comes in.

        Much appreciated. Dumindu Buddhika - if you have time perhaps you can write a couple of lower level unit tests to confirm that my isRowKeyOrderOptimized works in all situations and that array_cat, prepend, and append work in that they maintain the same separator byte that the array is already using.

        Show
        jamestaylor James Taylor added a comment - Dumindu Buddhika and ramkrishna.s.vasudevan - would you guys mind reviewing this patch? This ensures that descending, variable length arrays sort correctly. The change is the use a 255 byte as the separator for non null values (including the terminators). See the couple of new tests in ArrayIT. Much of the type changes are just formatting and moving a couple of duplicated methods where they belong at the base type. The other changes are to ensure we keep the same separator - for example if a table has not been upgraded, we need to keep using the 0 byte separator. That's where most of the complication comes in. Much appreciated. Dumindu Buddhika - if you have time perhaps you can write a couple of lower level unit tests to confirm that my isRowKeyOrderOptimized works in all situations and that array_cat, prepend, and append work in that they maintain the same separator byte that the array is already using.
        Hide
        ram_krish ramkrishna.s.vasudevan added a comment -

        Went through the patch, on a high level
        ->We will rewrite the array bytes to use the new seperator byte if it is of type DESC.
        -> for the array_cat - if the existing array to which we will append a new array is of the old type we will coerce it to use the new sepeartor and the new array that we add should automatically use the new seperator (if the overall sort order is DESC) right?
        ->same with the prepend and append.
        But one question regarding the other operations where we try to use the SEPERATOR_BYTE to find if we have reached the end of the array - in all such places we should not blindly check with SEPERTOR_BYTE right - instead try to decide it based on the order of the current byte[]?

        Show
        ram_krish ramkrishna.s.vasudevan added a comment - Went through the patch, on a high level ->We will rewrite the array bytes to use the new seperator byte if it is of type DESC. -> for the array_cat - if the existing array to which we will append a new array is of the old type we will coerce it to use the new sepeartor and the new array that we add should automatically use the new seperator (if the overall sort order is DESC) right? ->same with the prepend and append. But one question regarding the other operations where we try to use the SEPERATOR_BYTE to find if we have reached the end of the array - in all such places we should not blindly check with SEPERTOR_BYTE right - instead try to decide it based on the order of the current byte[]?
        Hide
        jamestaylor James Taylor added a comment -

        Thanks for the review, ramkrishna.s.vasudevan.

        should not blindly check with SEPERTOR_BYTE right - instead try to decide it based on the order of the current byte[]?

        Luckily, for arrays we navigate using the offsets in the header, so this part didn't need to change. The separator bytes are purely to make sure variable length arrays sort correctly relative to each other.

        I'll check in shortly unless you have other feedback?

        Show
        jamestaylor James Taylor added a comment - Thanks for the review, ramkrishna.s.vasudevan . should not blindly check with SEPERTOR_BYTE right - instead try to decide it based on the order of the current byte[]? Luckily, for arrays we navigate using the offsets in the header, so this part didn't need to change. The separator bytes are purely to make sure variable length arrays sort correctly relative to each other. I'll check in shortly unless you have other feedback?
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Phoenix-master #834 (See https://builds.apache.org/job/Phoenix-master/834/)
        PHOENIX-2067 Sort order incorrect for variable length DESC columns (jtaylor: rev 2620a80c1e35c0d214f06a1b16e99da5415a1a2c)

        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedTinyintArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/exception/SQLExceptionCode.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PBooleanArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/DelegateTable.java
        • phoenix-core/src/main/java/org/apache/phoenix/util/ScanUtil.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedIntArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/OrderPreservingTracker.java
        • phoenix-core/src/main/java/org/apache/phoenix/index/IndexMaintainer.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/util/regex/JONIPattern.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/stats/StatisticsUtil.java
        • phoenix-core/src/main/java/org/apache/phoenix/iterate/OrderedResultIterator.java
        • phoenix-core/src/test/java/org/apache/phoenix/compile/QueryCompilerTest.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PLongArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/util/SchemaUtil.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PTimeArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/TupleProjectionCompiler.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/RowValueConstructorExpression.java
        • phoenix-core/src/main/java/org/apache/phoenix/iterate/BaseResultIterators.java
        • phoenix-core/src/main/java/org/apache/phoenix/coprocessor/generated/PTableProtos.java
        • phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PFloatArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/util/PhoenixRuntime.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/ScanRanges.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/LpadFunctionIT.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/ReverseScanIT.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/SortOrderIT.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/JoinCompiler.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/RowKeyValueAccessor.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PArrayDataType.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/RowValueConstructorIT.java
        • phoenix-core/src/main/java/org/apache/phoenix/util/TupleUtil.java
        • phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDoubleArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/PTable.java
        • phoenix-core/src/main/java/org/apache/phoenix/util/ByteUtil.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/FromCompiler.java
        • phoenix-core/src/main/java/org/apache/phoenix/filter/SkipScanFilter.java
        • phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/function/LpadFunction.java
        • phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/RowKeySchema.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/ArrayConstructorExpression.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedSmallintArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/UpsertCompiler.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/ArrayIT.java
        • phoenix-protocol/src/main/PTable.proto
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedDoubleArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PVarbinaryArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/function/ArrayModifierFunction.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedDateArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/UnionCompiler.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedTimestampArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PTimestampArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/function/ArrayConcatFunction.java
        • phoenix-core/src/main/java/org/apache/phoenix/query/KeyRange.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDecimalArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PIntegerArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/IsNullIT.java
        • phoenix-core/src/main/java/org/apache/phoenix/execute/DescVarLengthFastByteComparisons.java
        • phoenix-core/src/test/java/org/apache/phoenix/query/OrderByTest.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PCharArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDataType.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/OrderByCompiler.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedTimeArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PVarcharArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/coprocessor/BaseScannerRegionObserver.java
        • dev/eclipse_prefs_phoenix.epf
        • phoenix-core/src/main/java/org/apache/phoenix/schema/ValueSchema.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PBinaryArray.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/SortOrderFIT.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedLongArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDateArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/execute/BaseQueryPlan.java
        • phoenix-core/src/main/java/org/apache/phoenix/query/QueryConstants.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedFloatArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PTinyintArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PSmallintArray.java
        • phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java
        • phoenix-core/src/main/java/org/apache/phoenix/expression/OrderByExpression.java
        • phoenix-core/src/test/java/org/apache/phoenix/schema/types/PDataTypeTest.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Phoenix-master #834 (See https://builds.apache.org/job/Phoenix-master/834/ ) PHOENIX-2067 Sort order incorrect for variable length DESC columns (jtaylor: rev 2620a80c1e35c0d214f06a1b16e99da5415a1a2c) phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedTinyintArray.java phoenix-core/src/main/java/org/apache/phoenix/exception/SQLExceptionCode.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PBooleanArray.java phoenix-core/src/main/java/org/apache/phoenix/schema/DelegateTable.java phoenix-core/src/main/java/org/apache/phoenix/util/ScanUtil.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedIntArray.java phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java phoenix-core/src/main/java/org/apache/phoenix/compile/OrderPreservingTracker.java phoenix-core/src/main/java/org/apache/phoenix/index/IndexMaintainer.java phoenix-core/src/main/java/org/apache/phoenix/expression/util/regex/JONIPattern.java phoenix-core/src/main/java/org/apache/phoenix/schema/stats/StatisticsUtil.java phoenix-core/src/main/java/org/apache/phoenix/iterate/OrderedResultIterator.java phoenix-core/src/test/java/org/apache/phoenix/compile/QueryCompilerTest.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PLongArray.java phoenix-core/src/main/java/org/apache/phoenix/util/SchemaUtil.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PTimeArray.java phoenix-core/src/main/java/org/apache/phoenix/compile/TupleProjectionCompiler.java phoenix-core/src/main/java/org/apache/phoenix/expression/RowValueConstructorExpression.java phoenix-core/src/main/java/org/apache/phoenix/iterate/BaseResultIterators.java phoenix-core/src/main/java/org/apache/phoenix/coprocessor/generated/PTableProtos.java phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PFloatArray.java phoenix-core/src/main/java/org/apache/phoenix/util/PhoenixRuntime.java phoenix-core/src/main/java/org/apache/phoenix/compile/ScanRanges.java phoenix-core/src/it/java/org/apache/phoenix/end2end/LpadFunctionIT.java phoenix-core/src/it/java/org/apache/phoenix/end2end/ReverseScanIT.java phoenix-core/src/it/java/org/apache/phoenix/end2end/SortOrderIT.java phoenix-core/src/main/java/org/apache/phoenix/compile/JoinCompiler.java phoenix-core/src/main/java/org/apache/phoenix/schema/RowKeyValueAccessor.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PArrayDataType.java phoenix-core/src/it/java/org/apache/phoenix/end2end/RowValueConstructorIT.java phoenix-core/src/main/java/org/apache/phoenix/util/TupleUtil.java phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDoubleArray.java phoenix-core/src/main/java/org/apache/phoenix/schema/PTable.java phoenix-core/src/main/java/org/apache/phoenix/util/ByteUtil.java phoenix-core/src/main/java/org/apache/phoenix/compile/FromCompiler.java phoenix-core/src/main/java/org/apache/phoenix/filter/SkipScanFilter.java phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java phoenix-core/src/main/java/org/apache/phoenix/expression/function/LpadFunction.java phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java phoenix-core/src/main/java/org/apache/phoenix/schema/RowKeySchema.java phoenix-core/src/main/java/org/apache/phoenix/expression/ArrayConstructorExpression.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedSmallintArray.java phoenix-core/src/main/java/org/apache/phoenix/compile/UpsertCompiler.java phoenix-core/src/it/java/org/apache/phoenix/end2end/ArrayIT.java phoenix-protocol/src/main/PTable.proto phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedDoubleArray.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PVarbinaryArray.java phoenix-core/src/main/java/org/apache/phoenix/expression/function/ArrayModifierFunction.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedDateArray.java phoenix-core/src/main/java/org/apache/phoenix/compile/UnionCompiler.java phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedTimestampArray.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PTimestampArray.java phoenix-core/src/main/java/org/apache/phoenix/expression/function/ArrayConcatFunction.java phoenix-core/src/main/java/org/apache/phoenix/query/KeyRange.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDecimalArray.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PIntegerArray.java phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java phoenix-core/src/it/java/org/apache/phoenix/end2end/IsNullIT.java phoenix-core/src/main/java/org/apache/phoenix/execute/DescVarLengthFastByteComparisons.java phoenix-core/src/test/java/org/apache/phoenix/query/OrderByTest.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PCharArray.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDataType.java phoenix-core/src/main/java/org/apache/phoenix/compile/OrderByCompiler.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedTimeArray.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PVarcharArray.java phoenix-core/src/main/java/org/apache/phoenix/coprocessor/BaseScannerRegionObserver.java dev/eclipse_prefs_phoenix.epf phoenix-core/src/main/java/org/apache/phoenix/schema/ValueSchema.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PBinaryArray.java phoenix-core/src/it/java/org/apache/phoenix/end2end/SortOrderFIT.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedLongArray.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDateArray.java phoenix-core/src/main/java/org/apache/phoenix/execute/BaseQueryPlan.java phoenix-core/src/main/java/org/apache/phoenix/query/QueryConstants.java phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PUnsignedFloatArray.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PTinyintArray.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PSmallintArray.java phoenix-core/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java phoenix-core/src/main/java/org/apache/phoenix/expression/OrderByExpression.java phoenix-core/src/test/java/org/apache/phoenix/schema/types/PDataTypeTest.java
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Phoenix-master #835 (See https://builds.apache.org/job/Phoenix-master/835/)
        PHOENIX-2067 Sort order incorrect for variable length DESC columns (jtaylor: rev 4b99c632c5e40251451e69fbe6d108f51e549e9e)

        • phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Phoenix-master #835 (See https://builds.apache.org/job/Phoenix-master/835/ ) PHOENIX-2067 Sort order incorrect for variable length DESC columns (jtaylor: rev 4b99c632c5e40251451e69fbe6d108f51e549e9e) phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
        Hide
        Dumindux Dumindu Buddhika added a comment -

        James Taylor sorry for the late response. Here are some tests for the built-ins. I have found a small issue. As we are converting double separator bytes at the end also to new separator bytes for DESC arrays, there was a problem in counting trailing nulls in first array(in ARRAY_CAT). I changed the code to consider the new separator byte also.

        Show
        Dumindux Dumindu Buddhika added a comment - James Taylor sorry for the late response. Here are some tests for the built-ins. I have found a small issue. As we are converting double separator bytes at the end also to new separator bytes for DESC arrays, there was a problem in counting trailing nulls in first array(in ARRAY_CAT). I changed the code to consider the new separator byte also.
        Hide
        Dumindux Dumindu Buddhika added a comment -

        As ramkrishna.s.vasudevan suggested I looked at the other parts of the code also. The trailing null issue was in two other places. In array deserialization and positionAtArrayElement method. This patch has two tests which demonstrate the cases and the fix.

        Show
        Dumindux Dumindu Buddhika added a comment - As ramkrishna.s.vasudevan suggested I looked at the other parts of the code also. The trailing null issue was in two other places. In array deserialization and positionAtArrayElement method. This patch has two tests which demonstrate the cases and the fix.
        Hide
        jamestaylor James Taylor added a comment -

        +1. Please commit to 4.x and master branch, ramkrishna.s.vasudevan (and you're welcome to review as well too, of course).

        Show
        jamestaylor James Taylor added a comment - +1. Please commit to 4.x and master branch, ramkrishna.s.vasudevan (and you're welcome to review as well too, of course).
        Hide
        ram_krish ramkrishna.s.vasudevan added a comment -

        +1 . Nice tests. Thanks for covering up all the cases Dumindu Buddhika.
        James Taylor
        I was trying to suggest this point only in my review.

        Show
        ram_krish ramkrishna.s.vasudevan added a comment - +1 . Nice tests. Thanks for covering up all the cases Dumindu Buddhika . James Taylor I was trying to suggest this point only in my review.
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Phoenix-master #841 (See https://builds.apache.org/job/Phoenix-master/841/)
        PHOENIX-2067 Sort order incorrect for variable length DESC columns - ARRAY (ramkrishna: rev 33d60506c5f2d4408a1df79f278d7a45d3401a27)

        • phoenix-core/src/test/java/org/apache/phoenix/expression/ArrayAppendFunctionTest.java
        • phoenix-core/src/main/java/org/apache/phoenix/schema/types/PArrayDataType.java
        • phoenix-core/src/test/java/org/apache/phoenix/schema/types/PDataTypeForArraysTest.java
        • phoenix-core/src/test/java/org/apache/phoenix/expression/ArrayPrependFunctionTest.java
        • phoenix-core/src/test/java/org/apache/phoenix/expression/ArrayConcatFunctionTest.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Phoenix-master #841 (See https://builds.apache.org/job/Phoenix-master/841/ ) PHOENIX-2067 Sort order incorrect for variable length DESC columns - ARRAY (ramkrishna: rev 33d60506c5f2d4408a1df79f278d7a45d3401a27) phoenix-core/src/test/java/org/apache/phoenix/expression/ArrayAppendFunctionTest.java phoenix-core/src/main/java/org/apache/phoenix/schema/types/PArrayDataType.java phoenix-core/src/test/java/org/apache/phoenix/schema/types/PDataTypeForArraysTest.java phoenix-core/src/test/java/org/apache/phoenix/expression/ArrayPrependFunctionTest.java phoenix-core/src/test/java/org/apache/phoenix/expression/ArrayConcatFunctionTest.java
        Hide
        rajeshbabu Rajeshbabu Chintaguntla added a comment -

        James Taylor Here is the backport patch for 4.4.1. Most of the patch applied manually. I ran all the tests and they have passed.
        Can you please review it. Will commit if it's ok.

        Thanks.

        Show
        rajeshbabu Rajeshbabu Chintaguntla added a comment - James Taylor Here is the backport patch for 4.4.1. Most of the patch applied manually. I ran all the tests and they have passed. Can you please review it. Will commit if it's ok. Thanks.
        Hide
        jamestaylor James Taylor added a comment -

        Patch looks good, Rajeshbabu Chintaguntla. Thanks for back porting. One thing that needs to be manually tested is converting old tables to the correct row key using "psql.py -u localhost". You can wait to do this until after back porting PHOENIX-2171 so you only have to do it once. You'd want to create various tables with DESC row keys in pre 4.4.1 and then run the script to ensure that the conversion works correctly and you get the expected query plan post conversion.

        Show
        jamestaylor James Taylor added a comment - Patch looks good, Rajeshbabu Chintaguntla . Thanks for back porting. One thing that needs to be manually tested is converting old tables to the correct row key using "psql.py -u localhost". You can wait to do this until after back porting PHOENIX-2171 so you only have to do it once. You'd want to create various tables with DESC row keys in pre 4.4.1 and then run the script to ensure that the conversion works correctly and you get the expected query plan post conversion.
        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.
        Hide
        ndimiduk Nick Dimiduk added a comment -

        I think this patch broke PK constraints for NOT NULL, non-fixed-width VARCHAR columns.

                    // If some non null pk values aren't set, then throw
                    if (i < nColumns) {
                        PColumn column = columns.get(i);
                        if (column.getDataType().isFixedWidth() || !column.isNullable()) {
                            throw new ConstraintViolationException(name.getString() + "." + column.getName().getString() + " may not be null");
                        }
                    }
        

        The constraint we're managing is that of non-null for PK columns. For VARCHAR, fixed-width meets this criteria because there's no available representation for NULL. I assume variable-length columns can represent null (though I don't know the encoding details off the top of my head), so we must look for the `NOT NULL` constraint added in schema.

        I think the if condition should be

        if (!column.getDataType.isFixedWidth() || column.isNullable()) { throw... }
        

        Also, can this be folded into a single column.isNullable() – shouldn't that method check the datatype on the caller's behalf? Or is there a scenario where we want to know what additional constraints the schema defined vs. what the datatype offers?

        Show
        ndimiduk Nick Dimiduk added a comment - I think this patch broke PK constraints for NOT NULL, non-fixed-width VARCHAR columns. // If some non null pk values aren't set, then throw if (i < nColumns) { PColumn column = columns.get(i); if (column.getDataType().isFixedWidth() || !column.isNullable()) { throw new ConstraintViolationException(name.getString() + "." + column.getName().getString() + " may not be null"); } } The constraint we're managing is that of non-null for PK columns. For VARCHAR, fixed-width meets this criteria because there's no available representation for NULL. I assume variable-length columns can represent null (though I don't know the encoding details off the top of my head), so we must look for the `NOT NULL` constraint added in schema. I think the if condition should be if (!column.getDataType.isFixedWidth() || column.isNullable()) { throw... } Also, can this be folded into a single column.isNullable() – shouldn't that method check the datatype on the caller's behalf? Or is there a scenario where we want to know what additional constraints the schema defined vs. what the datatype offers?
        Show
        ndimiduk Nick Dimiduk added a comment - Nope, this patch didn't change the logic on that line: https://github.com/apache/phoenix/commit/2620a80c1e35c0d214f06a1b16e99da5415a1a2c#diff-d87ee86bba434603ba73b6a85a139529

          People

          • Assignee:
            jamestaylor James Taylor
            Reporter:
            mykola.komarnytskyy Mykola Komarnytskyy
          • Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development