[jira] [Commented] (PHOENIX-3469) Once a column in primary key or index is DESC, the corresponding order by NULLS LAST/NULLS FIRST may work incorrectly
[ https://issues.apache.org/jira/browse/PHOENIX-3469?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15656833#comment-15656833 ] chenglei commented on PHOENIX-3469: --- [~jamestaylor], I uploaded my new patch, enhanced IT tests and unit tests following your suggestion. > 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_v2.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} > 90private 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) { > 232scanner = 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)
[jira] [Commented] (PHOENIX-3469) Once a column in primary key or index is DESC, the corresponding order by NULLS LAST/NULLS FIRST may work incorrectly
[ https://issues.apache.org/jira/browse/PHOENIX-3469?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15655768#comment-15655768 ] chenglei commented on PHOENIX-3469: --- Yes, [~jamestaylor],existing tests indeed all pass with my patch applied.It seems the https://builds.apache.org/job/PreCommit-PHOENIX-Build/ is pending,and I run the unit tests and IT tests in my local machine. I will continue my work following your suggestion. > 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} > 90private 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) { > 232scanner = 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)
[jira] [Commented] (PHOENIX-3469) Once a column in primary key or index is DESC, the corresponding order by NULLS LAST/NULLS FIRST may work incorrectly
[ 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} > 90private 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) { > 232scanner = 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)