[
https://issues.apache.org/jira/browse/PHOENIX-3469?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15655744#comment-15655744
]
James Taylor commented on PHOENIX-3469:
---------------------------------------
Nice work so far, [~comnetwork]. Here's some feedback:
- Let's also add testing for ORDER BY ORGANIZATION_ID ASC/DESC NULLS
FIRST/LAST. The code path is different if rows are ordered by the leading part
of the primary key.
- Would be good if the tests verified that the query plan is as expected for
these new tests. Namely that the plan.getOrderBy().isEmpty() is true when
expected and that it's using a reverse scan when DESC NULLS LAST is used, I
believe. You can check if a reverse scan is being done by (plan.getOrderBy() ==
OrderBy.REV_ROW_KEY_ORDER_BY). You can get the plan by doing a
statement.unwrap(PhoenixStatement.class).getQueryPlan().
Do existing tests all pass with your patch applied?
> Once a column in primary key or index is DESC, the corresponding order by
> NULLS LAST/NULLS FIRST may work incorrectly
> ----------------------------------------------------------------------------------------------------------------------
>
> Key: PHOENIX-3469
> URL: https://issues.apache.org/jira/browse/PHOENIX-3469
> Project: Phoenix
> Issue Type: Bug
> Affects Versions: 4.8.0
> Reporter: chenglei
> Assignee: chenglei
> Attachments: PHOENIX-3469_v1.patch
>
>
> This problem can be reproduced as following:
> {code:borderStyle=solid}
> CREATE TABLE DESC_TEST (
> ORGANIZATION_ID VARCHAR,
> CONTAINER_ID VARCHAR,
> ENTITY_ID VARCHAR NOT NULL,
> CONSTRAINT TEST_PK PRIMARY KEY (
> ORGANIZATION_ID DESC,
> CONTAINER_ID DESC,
> ENTITY_ID
> ))
> UPSERT INTO DESC_TEST VALUES ('a',null,'11')
> UPSERT INTO DESC_TEST VALUES (null,'2','22')
> UPSERT INTO DESC_TEST VALUES ('c','3','33')
> {code}
> For the following sql:
> {code:borderStyle=solid}
> SELECT CONTAINER_ID,ORGANIZATION_ID FROM DESC_TEST order by
> CONTAINER_ID ASC NULLS LAST
> {code}
> the expecting result is:
> {code:borderStyle=solid}
> 2, null
> 3, c
> null, a
> {code}
> but the actual result is:
> {code:borderStyle=solid}
> null, a
> 2, null
> 3, c
> {code}
> By debuging the source code,I found the ScanPlan passes the OrderByExpression
> to both the ScanRegionObserver and MergeSortTopNResultIterator in line 100
> and line 232,but the OrderByExpression 's "isNullsLast" property is false,
> while the sql is "order by CONTAINER_ID ASC NULLS LAST", the "isNullsLast"
> property should be true.
> {code:borderStyle=solid}
> 90 private ScanPlan(StatementContext context, FilterableStatement
> statement, TableRef table, RowProjector projector, Integer limit, Integer
> offset, OrderBy orderBy, ParallelIteratorFactory parallelIteratorFactory,
> boolean allowPageFilter, Expression dynamicFilter) throws SQLException {
> ......
> 95 boolean isOrdered = !orderBy.getOrderByExpressions().isEmpty();
> 96 if (isOrdered) { // TopN
> 97 int thresholdBytes =
> context.getConnection().getQueryServices().getProps().getInt(
> 98 QueryServices.SPOOL_THRESHOLD_BYTES_ATTRIB,
> QueryServicesOptions.DEFAULT_SPOOL_THRESHOLD_BYTES);
> 99 ScanRegionObserver.serializeIntoScan(context.getScan(),
> thresholdBytes,
> 100 limit == null ? -1 : QueryUtil.getOffsetLimit(limit,
> offset), orderBy.getOrderByExpressions(),
> 101 projector.getEstimatedRowByteSize());
> 102 }
> ......
> 231 } else if (isOrdered) {
> 232 scanner = new MergeSortTopNResultIterator(iterators, limit,
> offset, orderBy.getOrderByExpressions());
> {code}
> so the problem is caused by the OrderByCompiler, in line 144, it should not
> negative the "isNullsLast",because the "isNullsLast" should not be influenced
> by the SortOrder,no matter it is DESC or ASC:
> {code:borderStyle=solid}
> 142 if (expression.getSortOrder() == SortOrder.DESC) {
> 143 isAscending = !isAscending;
> 144 isNullsLast = !isNullsLast;
> 145 }
> {code}
> I include more IT test cases in my patch.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
