You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by dbwong <gi...@git.apache.org> on 2018/10/10 09:46:24 UTC
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
GitHub user dbwong opened a pull request:
https://github.com/apache/phoenix/pull/362
[Phoenix-4841] Filters that uses RVC with pk columns where with DESC sort order don't work correctly
https://issues.apache.org/jira/browse/PHOENIX-4841
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dbwong/phoenix PHOENIX-4841
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/362.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #362
----
commit 64103759c0138b23995760e939b4e49a444ed52a
Author: Daniel Wong <da...@...>
Date: 2018-10-09T23:38:11Z
Initial work on 4841.
commit bd2eca081dd436ac2b35d26a5af7af1b1c8f82e5
Author: Daniel Wong <da...@...>
Date: 2018-10-10T00:31:13Z
Update to handle InList optimizations.
----
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by dbwong <gi...@git.apache.org>.
Github user dbwong commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r225743248
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java ---
@@ -231,6 +231,24 @@ public static Expression pushKeyExpressionsToScan(StatementContext context, Filt
if (prevSortOrder == null) {
prevSortOrder = sortOrder;
} else if (prevSortOrder != sortOrder) {
+ //Consider the Universe of keys to be [0,7]+ on the leading column A
--- End diff --
That regular expression based that is keys are a collection of values between 0 and 7777...
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r230509377
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java ---
@@ -242,6 +260,12 @@ public static Expression pushKeyExpressionsToScan(StatementContext context, Filt
cnf.add(leftRanges);
clipLeftSpan = 0;
prevSortOrder = sortOrder;
+ // since we have to clip the portion with the same sort order, we can no longer
+ // extract the nodes from the where clause
+ // for eg. for the schema A VARCHAR DESC, B VARCHAR ASC and query WHERE (A,B) < ('a','b')
+ // the range (* - a\xFFb) is converted to (~a-*)(*-b)
+ // so we still need to filter on A,B
--- End diff --
ok cool
---
[GitHub] phoenix issue #362: [Phoenix-4841] Filters that uses RVC with pk columns whe...
Posted by xcangCRM <gi...@git.apache.org>.
Github user xcangCRM commented on the issue:
https://github.com/apache/phoenix/pull/362
+1
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by dbwong <gi...@git.apache.org>.
Github user dbwong commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r224941422
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java ---
@@ -2060,10 +2084,13 @@ public Integer getScale() {
@Override
public SortOrder getSortOrder() {
- // The parts of the RVC have already been converted
- // to ascending, so we don't need to consider the
- // childPart sort order.
- return SortOrder.ASC;
+ //WARNING HACK: Handle the different paths for InList vs Normal Comparison
--- End diff --
Made https://issues.apache.org/jira/browse/PHOENIX-4969 and will update the comment.
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by dbwong <gi...@git.apache.org>.
Github user dbwong commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r225743277
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryMoreIT.java ---
@@ -511,4 +513,122 @@ private void upsertRows(PhoenixConnection conn, String fullTableName) throws SQL
stmt.execute();
}
}
+
+ @Test public void testRVCWithDescAndAscendingPK() throws Exception {
+ final Connection conn = DriverManager.getConnection(getUrl());
+ String fullTableName = generateUniqueName();
+ try (Statement stmt = conn.createStatement()) {
+ stmt.execute("CREATE TABLE " + fullTableName + "(\n"
+ + " ORGANIZATION_ID CHAR(15) NOT NULL,\n" + " SCORE VARCHAR NOT NULL,\n"
+ + " ENTITY_ID VARCHAR NOT NULL\n"
+ + " CONSTRAINT PAGE_SNAPSHOT_PK PRIMARY KEY (\n"
+ + " ORGANIZATION_ID,\n" + " SCORE DESC,\n" + " ENTITY_ID\n"
+ + " )\n" + ") MULTI_TENANT=TRUE");
+ }
+
+ conn.createStatement().execute("UPSERT INTO " + fullTableName + " VALUES ('org1','c','1')");
+ conn.createStatement().execute("UPSERT INTO " + fullTableName + " VALUES ('org1','b','3')");
+ conn.createStatement().execute("UPSERT INTO " + fullTableName + " VALUES ('org1','b','4')");
+ conn.createStatement().execute("UPSERT INTO " + fullTableName + " VALUES ('org1','a','2')");
+ conn.commit();
+
+ try (Statement stmt = conn.createStatement()) {
+ final ResultSet
+ rs =
+ stmt.executeQuery("SELECT score, entity_id \n" + "FROM " + fullTableName + "\n"
+ + "WHERE organization_id = 'org1'\n"
+ + "AND (score, entity_id) < ('b', '4')\n"
+ + "ORDER BY score DESC, entity_id\n" + "LIMIT 3");
+ assertTrue(rs.next());
+ assertEquals("b", rs.getString(1));
+ assertEquals("3", rs.getString(2));
+ assertTrue(rs.next());
+ assertEquals("a", rs.getString(1));
+ assertEquals("2", rs.getString(2));
+ assertFalse(rs.next());
+ }
+ }
+
+ @Test public void testRVCOrderByDesc() throws Exception {
--- End diff --
Sure
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by dbwong <gi...@git.apache.org>.
Github user dbwong commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r224941255
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryMoreIT.java ---
@@ -17,26 +17,7 @@
*/
package org.apache.phoenix.end2end;
-import static org.junit.Assert.assertEquals;
--- End diff --
Done
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by dbwong <gi...@git.apache.org>.
Github user dbwong commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r225743291
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryMoreIT.java ---
@@ -511,4 +513,122 @@ private void upsertRows(PhoenixConnection conn, String fullTableName) throws SQL
stmt.execute();
}
}
+
+ @Test public void testRVCWithDescAndAscendingPK() throws Exception {
--- End diff --
Sure
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by xcangCRM <gi...@git.apache.org>.
Github user xcangCRM commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r225653134
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryMoreIT.java ---
@@ -511,4 +513,122 @@ private void upsertRows(PhoenixConnection conn, String fullTableName) throws SQL
stmt.execute();
}
}
+
+ @Test public void testRVCWithDescAndAscendingPK() throws Exception {
+ final Connection conn = DriverManager.getConnection(getUrl());
+ String fullTableName = generateUniqueName();
+ try (Statement stmt = conn.createStatement()) {
+ stmt.execute("CREATE TABLE " + fullTableName + "(\n"
+ + " ORGANIZATION_ID CHAR(15) NOT NULL,\n" + " SCORE VARCHAR NOT NULL,\n"
+ + " ENTITY_ID VARCHAR NOT NULL\n"
+ + " CONSTRAINT PAGE_SNAPSHOT_PK PRIMARY KEY (\n"
+ + " ORGANIZATION_ID,\n" + " SCORE DESC,\n" + " ENTITY_ID\n"
+ + " )\n" + ") MULTI_TENANT=TRUE");
+ }
+
+ conn.createStatement().execute("UPSERT INTO " + fullTableName + " VALUES ('org1','c','1')");
+ conn.createStatement().execute("UPSERT INTO " + fullTableName + " VALUES ('org1','b','3')");
+ conn.createStatement().execute("UPSERT INTO " + fullTableName + " VALUES ('org1','b','4')");
+ conn.createStatement().execute("UPSERT INTO " + fullTableName + " VALUES ('org1','a','2')");
+ conn.commit();
+
+ try (Statement stmt = conn.createStatement()) {
+ final ResultSet
+ rs =
+ stmt.executeQuery("SELECT score, entity_id \n" + "FROM " + fullTableName + "\n"
+ + "WHERE organization_id = 'org1'\n"
+ + "AND (score, entity_id) < ('b', '4')\n"
+ + "ORDER BY score DESC, entity_id\n" + "LIMIT 3");
+ assertTrue(rs.next());
+ assertEquals("b", rs.getString(1));
+ assertEquals("3", rs.getString(2));
+ assertTrue(rs.next());
+ assertEquals("a", rs.getString(1));
+ assertEquals("2", rs.getString(2));
+ assertFalse(rs.next());
+ }
+ }
+
+ @Test public void testRVCOrderByDesc() throws Exception {
--- End diff --
nit: the unit test name should mention view and index.
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by dbwong <gi...@git.apache.org>.
Github user dbwong commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r225743271
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java ---
@@ -242,6 +260,12 @@ public static Expression pushKeyExpressionsToScan(StatementContext context, Filt
cnf.add(leftRanges);
clipLeftSpan = 0;
prevSortOrder = sortOrder;
+ // since we have to clip the portion with the same sort order, we can no longer
+ // extract the nodes from the where clause
+ // for eg. for the schema A VARCHAR DESC, B VARCHAR ASC and query WHERE (A,B) < ('a','b')
+ // the range (* - a\xFFb) is converted to (~a-*)(*-b)
+ // so we still need to filter on A
+ stopExtracting = true;
--- End diff --
Ok
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by xcangCRM <gi...@git.apache.org>.
Github user xcangCRM commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r225635175
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryMoreIT.java ---
@@ -511,4 +513,122 @@ private void upsertRows(PhoenixConnection conn, String fullTableName) throws SQL
stmt.execute();
}
}
+
+ @Test public void testRVCWithDescAndAscendingPK() throws Exception {
--- End diff --
nit:annotation should be in separate line.
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r224644448
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java ---
@@ -2060,10 +2084,13 @@ public Integer getScale() {
@Override
public SortOrder getSortOrder() {
- // The parts of the RVC have already been converted
- // to ascending, so we don't need to consider the
- // childPart sort order.
- return SortOrder.ASC;
+ //WARNING HACK: Handle the different paths for InList vs Normal Comparison
--- End diff --
Please file and reference a JIRA to fix this and update this comment.
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by xcangCRM <gi...@git.apache.org>.
Github user xcangCRM commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r225635425
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryMoreIT.java ---
@@ -511,4 +513,122 @@ private void upsertRows(PhoenixConnection conn, String fullTableName) throws SQL
stmt.execute();
}
}
+
+ @Test public void testRVCWithDescAndAscendingPK() throws Exception {
+ final Connection conn = DriverManager.getConnection(getUrl());
+ String fullTableName = generateUniqueName();
+ try (Statement stmt = conn.createStatement()) {
+ stmt.execute("CREATE TABLE " + fullTableName + "(\n"
+ + " ORGANIZATION_ID CHAR(15) NOT NULL,\n" + " SCORE VARCHAR NOT NULL,\n"
--- End diff --
nit: return and have a new line after you have "\n". VIsually it's gonna be easier to read.
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r226392353
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java ---
@@ -242,6 +260,12 @@ public static Expression pushKeyExpressionsToScan(StatementContext context, Filt
cnf.add(leftRanges);
clipLeftSpan = 0;
prevSortOrder = sortOrder;
+ // since we have to clip the portion with the same sort order, we can no longer
+ // extract the nodes from the where clause
+ // for eg. for the schema A VARCHAR DESC, B VARCHAR ASC and query WHERE (A,B) < ('a','b')
+ // the range (* - a\xFFb) is converted to (~a-*)(*-b)
+ // so we still need to filter on A,B
--- End diff --
(~a-*) gets pushed to the start row of the scan and then we the filter only includes B, right?
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r224644325
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java ---
@@ -242,6 +260,12 @@ public static Expression pushKeyExpressionsToScan(StatementContext context, Filt
cnf.add(leftRanges);
clipLeftSpan = 0;
prevSortOrder = sortOrder;
+ // since we have to clip the portion with the same sort order, we can no longer
+ // extract the nodes from the where clause
+ // for eg. for the schema A VARCHAR DESC, B VARCHAR ASC and query WHERE (A,B) < ('a','b')
+ // the range (* - a\xFFb) is converted to (~a-*)(*-b)
+ // so we still need to filter on A,B
--- End diff --
I think the comment should be : we still need to filter on b
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by dbwong <gi...@git.apache.org>.
Github user dbwong commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r224941403
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java ---
@@ -242,6 +260,12 @@ public static Expression pushKeyExpressionsToScan(StatementContext context, Filt
cnf.add(leftRanges);
clipLeftSpan = 0;
prevSortOrder = sortOrder;
+ // since we have to clip the portion with the same sort order, we can no longer
+ // extract the nodes from the where clause
+ // for eg. for the schema A VARCHAR DESC, B VARCHAR ASC and query WHERE (A,B) < ('a','b')
+ // the range (* - a\xFFb) is converted to (~a-*)(*-b)
+ // so we still need to filter on A,B
--- End diff --
This needs to filter for both A and B still since the filter on B requires looking at the value of A.
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by xcangCRM <gi...@git.apache.org>.
Github user xcangCRM commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r225637581
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/expression/ComparisonExpression.java ---
@@ -128,6 +131,27 @@ public static Expression create(CompareOp op, List<Expression> children, Immutab
if ( ! ( lhsExpr instanceof RowValueConstructorExpression ) ) {
lhsExpr = new RowValueConstructorExpression(Collections.singletonList(lhsExpr), lhsExpr.isStateless());
}
+
+ //At this point both sides should be in the same row format
+ //We add the inverts so the filtering can be done properly for mixed sort type RVCs, The entire RVC has to
+ // be in ASC for the actual compare to work since compare simply does a varbyte compare. See PHOENIX-4841
+ List<RowValueConstructorExpression> rvcList = Lists.newArrayList((RowValueConstructorExpression)lhsExpr,(RowValueConstructorExpression)rhsExpr);
--- End diff --
can this be a separate method? Then add a unit test to this?
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r224643012
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryMoreIT.java ---
@@ -17,26 +17,7 @@
*/
package org.apache.phoenix.end2end;
-import static org.junit.Assert.assertEquals;
--- End diff --
Please use the code template / formatter from https://phoenix.apache.org/develop.html
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by xcangCRM <gi...@git.apache.org>.
Github user xcangCRM commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r225660501
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java ---
@@ -242,6 +260,12 @@ public static Expression pushKeyExpressionsToScan(StatementContext context, Filt
cnf.add(leftRanges);
clipLeftSpan = 0;
prevSortOrder = sortOrder;
+ // since we have to clip the portion with the same sort order, we can no longer
+ // extract the nodes from the where clause
+ // for eg. for the schema A VARCHAR DESC, B VARCHAR ASC and query WHERE (A,B) < ('a','b')
+ // the range (* - a\xFFb) is converted to (~a-*)(*-b)
+ // so we still need to filter on A
+ stopExtracting = true;
--- End diff --
Since you added this new scenario setting stopExtracting to true. Can you also update the comment below at line 290:"// Stop extracting nodes once we encounter" accordingly?
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by dbwong <gi...@git.apache.org>.
Github user dbwong commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r230494407
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java ---
@@ -242,6 +260,12 @@ public static Expression pushKeyExpressionsToScan(StatementContext context, Filt
cnf.add(leftRanges);
clipLeftSpan = 0;
prevSortOrder = sortOrder;
+ // since we have to clip the portion with the same sort order, we can no longer
+ // extract the nodes from the where clause
+ // for eg. for the schema A VARCHAR DESC, B VARCHAR ASC and query WHERE (A,B) < ('a','b')
+ // the range (* - a\xFFb) is converted to (~a-*)(*-b)
+ // so we still need to filter on A,B
--- End diff --
So 2 clarifications:
One there is no attempt today to unentwine the RVC comparison into pieces for different columns.
Two, the filter still needs A as the behavior is different if A == 'a' or if A > 'a', if A=='a' then we consider B only, otherwise we do not consider B. The scan considers rows both A == 'a' and A > 'a'
---
[GitHub] phoenix issue #362: [Phoenix-4841] Filters that uses RVC with pk columns whe...
Posted by dbwong <gi...@git.apache.org>.
Github user dbwong commented on the issue:
https://github.com/apache/phoenix/pull/362
Updated my comment in the PR for addition detail and clarity @twdsilva.
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by dbwong <gi...@git.apache.org>.
Github user dbwong commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r225743287
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryMoreIT.java ---
@@ -511,4 +513,122 @@ private void upsertRows(PhoenixConnection conn, String fullTableName) throws SQL
stmt.execute();
}
}
+
+ @Test public void testRVCWithDescAndAscendingPK() throws Exception {
+ final Connection conn = DriverManager.getConnection(getUrl());
+ String fullTableName = generateUniqueName();
+ try (Statement stmt = conn.createStatement()) {
+ stmt.execute("CREATE TABLE " + fullTableName + "(\n"
+ + " ORGANIZATION_ID CHAR(15) NOT NULL,\n" + " SCORE VARCHAR NOT NULL,\n"
--- End diff --
Sure
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by dbwong <gi...@git.apache.org>.
Github user dbwong commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r225743280
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/expression/ComparisonExpression.java ---
@@ -128,6 +131,27 @@ public static Expression create(CompareOp op, List<Expression> children, Immutab
if ( ! ( lhsExpr instanceof RowValueConstructorExpression ) ) {
lhsExpr = new RowValueConstructorExpression(Collections.singletonList(lhsExpr), lhsExpr.isStateless());
}
+
+ //At this point both sides should be in the same row format
+ //We add the inverts so the filtering can be done properly for mixed sort type RVCs, The entire RVC has to
+ // be in ASC for the actual compare to work since compare simply does a varbyte compare. See PHOENIX-4841
+ List<RowValueConstructorExpression> rvcList = Lists.newArrayList((RowValueConstructorExpression)lhsExpr,(RowValueConstructorExpression)rhsExpr);
--- End diff --
Ok
---
[GitHub] phoenix pull request #362: [Phoenix-4841] Filters that uses RVC with pk colu...
Posted by xcangCRM <gi...@git.apache.org>.
Github user xcangCRM commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/362#discussion_r225654063
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java ---
@@ -231,6 +231,24 @@ public static Expression pushKeyExpressionsToScan(StatementContext context, Filt
if (prevSortOrder == null) {
prevSortOrder = sortOrder;
} else if (prevSortOrder != sortOrder) {
+ //Consider the Universe of keys to be [0,7]+ on the leading column A
--- End diff --
what do you mean [0,7]+ here?
---