Re: [PR] PHOENIX-7258: Query Optimizer should pick Index hint even for point lookup queries [phoenix]
virajjasani merged PR #1851: URL: https://github.com/apache/phoenix/pull/1851 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] PHOENIX-7258: Query Optimizer should pick Index hint even for point lookup queries [phoenix]
sanjeet006py commented on code in PR #1851:
URL: https://github.com/apache/phoenix/pull/1851#discussion_r1517922771
##
phoenix-core-client/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java:
##
@@ -247,6 +248,10 @@ private List
getApplicablePlansForSingleFlatQuery(QueryPlan dataPlan,
}
plans.add(0, hintedPlan);
}
+if (dataPlan.getContext().getScanRanges().isPointLookup()
+&& stopAtBestPlan && dataPlan.isApplicable()) {
+return Collections. singletonList(dataPlan);
+}
Review Comment:
I have addressed the suggestion and also added an IT for partial index.
Please take a look. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PHOENIX-7258: Query Optimizer should pick Index hint even for point lookup queries [phoenix]
tkhurana commented on code in PR #1851:
URL: https://github.com/apache/phoenix/pull/1851#discussion_r1517620543
##
phoenix-core-client/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java:
##
@@ -247,6 +248,10 @@ private List
getApplicablePlansForSingleFlatQuery(QueryPlan dataPlan,
}
plans.add(0, hintedPlan);
}
+if (dataPlan.getContext().getScanRanges().isPointLookup()
+&& stopAtBestPlan && dataPlan.isApplicable()) {
+return Collections. singletonList(dataPlan);
+}
Review Comment:
But this will not be a common scenario. Most of the time index hint will be
null and you will return early. If index hint is not null then most of the
times you will use the hinted index plan. It will be rare that you will end up
here that you have to exit early.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PHOENIX-7258: Query Optimizer should pick Index hint even for point lookup queries [phoenix]
sanjeet006py commented on code in PR #1851:
URL: https://github.com/apache/phoenix/pull/1851#discussion_r1517610019
##
phoenix-core-client/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java:
##
@@ -247,6 +248,10 @@ private List
getApplicablePlansForSingleFlatQuery(QueryPlan dataPlan,
}
plans.add(0, hintedPlan);
}
+if (dataPlan.getContext().getScanRanges().isPointLookup()
+&& stopAtBestPlan && dataPlan.isApplicable()) {
+return Collections. singletonList(dataPlan);
+}
Review Comment:
@tkhurana I see that if we remove this block then code goes on to create
query plan for each index of the original data table. And, to create query plan
for each index at least 1 RPC call is done per index so, I think this can be
expensive when the end result we know is original data plan but we will still
be doing RPC calls for other indexes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PHOENIX-7258: Query Optimizer should pick Index hint even for point lookup queries [phoenix]
sanjeet006py commented on code in PR #1851:
URL: https://github.com/apache/phoenix/pull/1851#discussion_r1517610019
##
phoenix-core-client/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java:
##
@@ -247,6 +248,10 @@ private List
getApplicablePlansForSingleFlatQuery(QueryPlan dataPlan,
}
plans.add(0, hintedPlan);
}
+if (dataPlan.getContext().getScanRanges().isPointLookup()
+&& stopAtBestPlan && dataPlan.isApplicable()) {
+return Collections. singletonList(dataPlan);
+}
Review Comment:
@tkhurana I see that if we remove this block then code goes on to create
query plan for each index of the original data table. And, to create query plan
for each index at least 1 RPC call is done per index so, I think this can be
expensive when the end result we know is original data plan but we will still
do RPC calls for other indexes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PHOENIX-7258: Query Optimizer should pick Index hint even for point lookup queries [phoenix]
sanjeet006py commented on code in PR #1851:
URL: https://github.com/apache/phoenix/pull/1851#discussion_r1517610019
##
phoenix-core-client/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java:
##
@@ -247,6 +248,10 @@ private List
getApplicablePlansForSingleFlatQuery(QueryPlan dataPlan,
}
plans.add(0, hintedPlan);
}
+if (dataPlan.getContext().getScanRanges().isPointLookup()
+&& stopAtBestPlan && dataPlan.isApplicable()) {
+return Collections. singletonList(dataPlan);
+}
Review Comment:
@tkhurana I see that if we remove this block then code goes on to create
query plan for each index of the original data table. And, to create query plan
for each index at least 1 RPC call is done so, I think this can be expensive
when the end result we know is original data plan but we will still do RPC
calls for other indexes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PHOENIX-7258: Query Optimizer should pick Index hint even for point lookup queries [phoenix]
sanjeet006py commented on code in PR #1851:
URL: https://github.com/apache/phoenix/pull/1851#discussion_r1517470937
##
phoenix-core-client/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java:
##
@@ -247,6 +248,10 @@ private List
getApplicablePlansForSingleFlatQuery(QueryPlan dataPlan,
}
plans.add(0, hintedPlan);
}
+if (dataPlan.getContext().getScanRanges().isPointLookup()
+&& stopAtBestPlan && dataPlan.isApplicable()) {
+return Collections. singletonList(dataPlan);
+}
Review Comment:
Got it, thanks. Checking
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PHOENIX-7258: Query Optimizer should pick Index hint even for point lookup queries [phoenix]
tkhurana commented on code in PR #1851:
URL: https://github.com/apache/phoenix/pull/1851#discussion_r1517442429
##
phoenix-core-client/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java:
##
@@ -247,6 +248,10 @@ private List
getApplicablePlansForSingleFlatQuery(QueryPlan dataPlan,
}
plans.add(0, hintedPlan);
}
+if (dataPlan.getContext().getScanRanges().isPointLookup()
+&& stopAtBestPlan && dataPlan.isApplicable()) {
+return Collections. singletonList(dataPlan);
+}
Review Comment:
The optimizer will select the optimal plan from the list of plans. You can
test by commenting this logic. There is no need to duplicate code. It is
confusing to maintain. Same logic is in multiple places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PHOENIX-7258: Query Optimizer should pick Index hint even for point lookup queries [phoenix]
sanjeet006py commented on code in PR #1851:
URL: https://github.com/apache/phoenix/pull/1851#discussion_r1517375166
##
phoenix-core/src/it/java/org/apache/phoenix/end2end/RowValueConstructorOffsetIT.java:
##
@@ -1221,4 +1222,29 @@ public void rvcOffsetTrailingNullKeyTest() throws
Exception {
}
}
+// Test point lookup over data table with index hint and hinted plan is
not applicable
+@Test
+public void testRVCOffsetWithNotApplicableIndexHint() throws Exception {
+String sql = String.format("SELECT /*+ INDEX(%s %s)*/ %s FROM %s "
++ "WHERE t_id = 'b' AND k1 = 2 AND k2 = 3 OFFSET
(%s)=('a', 1, 2)",
+TABLE_NAME, INDEX_NAME,
TABLE_ROW_KEY,TABLE_NAME,TABLE_ROW_KEY);
+try (Statement statement = conn.createStatement()){
+ResultSet rs = statement.executeQuery("EXPLAIN " + sql);
+String actualQueryPlan = QueryUtil.getExplainPlan(rs);
+// As hinted plan is not applicable so use data plan which is
point lookup
+assertTrue(actualQueryPlan.contains("POINT LOOKUP ON 1 KEY OVER "
+ TABLE_NAME));
+}
+}
+
+@Test
+public void testRVCOffsetWithNotApplicableAndPointLookup() throws
Exception {
Review Comment:
Yes, I added this test as I am doing changes for point lookup case and
wanted to assert that I am not breaking the scenario where its point lookup but
no query plan is applicable. Just wanted to be extra sure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PHOENIX-7258: Query Optimizer should pick Index hint even for point lookup queries [phoenix]
sanjeet006py commented on code in PR #1851:
URL: https://github.com/apache/phoenix/pull/1851#discussion_r1517343363
##
phoenix-core-client/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java:
##
@@ -247,6 +248,10 @@ private List
getApplicablePlansForSingleFlatQuery(QueryPlan dataPlan,
}
plans.add(0, hintedPlan);
}
+if (dataPlan.getContext().getScanRanges().isPointLookup()
+&& stopAtBestPlan && dataPlan.isApplicable()) {
+return Collections. singletonList(dataPlan);
+}
Review Comment:
Please correct me if I misunderstood something.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PHOENIX-7258: Query Optimizer should pick Index hint even for point lookup queries [phoenix]
sanjeet006py commented on code in PR #1851:
URL: https://github.com/apache/phoenix/pull/1851#discussion_r1517343103
##
phoenix-core-client/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java:
##
@@ -247,6 +248,10 @@ private List
getApplicablePlansForSingleFlatQuery(QueryPlan dataPlan,
}
plans.add(0, hintedPlan);
}
+if (dataPlan.getContext().getScanRanges().isPointLookup()
+&& stopAtBestPlan && dataPlan.isApplicable()) {
+return Collections. singletonList(dataPlan);
+}
Review Comment:
Yes, this fix honors index hint even if original data plan is point lookup.
But if hinted plan is null or not applicable then we need to return original
data plan as we are already maximum optimized.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PHOENIX-7258: Query Optimizer should pick Index hint even for point lookup queries [phoenix]
virajjasani commented on code in PR #1851:
URL: https://github.com/apache/phoenix/pull/1851#discussion_r1517163722
##
phoenix-core-client/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java:
##
@@ -247,6 +248,10 @@ private List
getApplicablePlansForSingleFlatQuery(QueryPlan dataPlan,
}
plans.add(0, hintedPlan);
}
+if (dataPlan.getContext().getScanRanges().isPointLookup()
+&& stopAtBestPlan && dataPlan.isApplicable()) {
+return Collections. singletonList(dataPlan);
+}
Review Comment:
I was thinking about the same.
It seems that we might have more plans in the list of plans than just data
plan, so if we return single plan here as data plan, it would help cover this
case: Even if the hinted index is not applicable, and the data table has point
lookup, we return best plan as point lookup on data table.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PHOENIX-7258: Query Optimizer should pick Index hint even for point lookup queries [phoenix]
tkhurana commented on code in PR #1851:
URL: https://github.com/apache/phoenix/pull/1851#discussion_r1516937729
##
phoenix-core-client/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java:
##
@@ -247,6 +248,10 @@ private List
getApplicablePlansForSingleFlatQuery(QueryPlan dataPlan,
}
plans.add(0, hintedPlan);
}
+if (dataPlan.getContext().getScanRanges().isPointLookup()
+&& stopAtBestPlan && dataPlan.isApplicable()) {
+return Collections. singletonList(dataPlan);
+}
Review Comment:
Why is this logic added here ? The dataPlan is already added to the list of
plans. This feels like code smell.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PHOENIX-7258: Query Optimizer should pick Index hint even for point lookup queries [phoenix]
kadirozde commented on code in PR #1851:
URL: https://github.com/apache/phoenix/pull/1851#discussion_r1516869330
##
phoenix-core/src/it/java/org/apache/phoenix/end2end/RowValueConstructorOffsetIT.java:
##
@@ -1221,4 +1222,29 @@ public void rvcOffsetTrailingNullKeyTest() throws
Exception {
}
}
+// Test point lookup over data table with index hint and hinted plan is
not applicable
+@Test
+public void testRVCOffsetWithNotApplicableIndexHint() throws Exception {
+String sql = String.format("SELECT /*+ INDEX(%s %s)*/ %s FROM %s "
++ "WHERE t_id = 'b' AND k1 = 2 AND k2 = 3 OFFSET
(%s)=('a', 1, 2)",
+TABLE_NAME, INDEX_NAME,
TABLE_ROW_KEY,TABLE_NAME,TABLE_ROW_KEY);
+try (Statement statement = conn.createStatement()){
+ResultSet rs = statement.executeQuery("EXPLAIN " + sql);
+String actualQueryPlan = QueryUtil.getExplainPlan(rs);
+// As hinted plan is not applicable so use data plan which is
point lookup
+assertTrue(actualQueryPlan.contains("POINT LOOKUP ON 1 KEY OVER "
+ TABLE_NAME));
+}
+}
+
+@Test
+public void testRVCOffsetWithNotApplicableAndPointLookup() throws
Exception {
Review Comment:
is this test applicable to this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PHOENIX-7258: Query Optimizer should pick Index hint even for point lookup queries [phoenix]
virajjasani commented on code in PR #1851:
URL: https://github.com/apache/phoenix/pull/1851#discussion_r1516856703
##
phoenix-core/src/it/java/org/apache/phoenix/end2end/TenantSpecificTablesDDLIT.java:
##
@@ -616,6 +621,52 @@ public void testTableMetadataScan() throws Exception {
conn.close();
}
}
+
+@Test
+public void testIndexHintWithTenantView() throws Exception {
+String schemaName = generateUniqueName();
+String dataTableName = generateUniqueName();
+String fullDataTableName = SchemaUtil.getTableName(schemaName,
dataTableName);
+String viewName = generateUniqueName();
+String fullViewName = SchemaUtil.getTableName(schemaName, viewName);
+String viewIndexName = generateUniqueName();
+try(Connection conn = DriverManager.getConnection(getUrl());
+Statement stmt = conn.createStatement()) {
+String createDataTable = "create table " + fullDataTableName + "
(orgid varchar(10) not null, "
++ "id1 varchar(10) not null, id2 varchar(10) not null, id3
integer not null, "
++ "val1 varchar(10), val2 varchar(10) " +
+"CONSTRAINT PK PRIMARY KEY (orgid, id1, id2, id3))
MULTI_TENANT=true";
+stmt.execute(createDataTable);
+stmt.execute("create view " + fullViewName + " as select * from "
+ fullDataTableName);
+stmt.execute("create index " + viewIndexName + " on " +
fullViewName + "(id3, id2, id1) include (val1, val2)");
+}
+try(Connection conn =
DriverManager.getConnection(PHOENIX_JDBC_TENANT_SPECIFIC_URL);
+Statement stmt = conn.createStatement()) {
+String grandChildViewName = generateUniqueName();
+String fullGrandChildViewName =
SchemaUtil.getTableName(schemaName, grandChildViewName);
+stmt.execute("create view " + fullGrandChildViewName + " as select
* from " + fullViewName);
+PhoenixConnection pconn = conn.unwrap(PhoenixConnection.class);
+pconn.getTableNoCache(pconn.getTenantId(), fullGrandChildViewName);
+stmt.execute("upsert into " + fullGrandChildViewName + " values
('a1', 'a2', 3, 'a4', 'a5')");
+conn.commit();
+stmt.execute("upsert into " + fullGrandChildViewName + " values
('b1', 'b2', 3, 'b4', 'b5')");
+conn.commit();
+String physicalViewIndexTableName =
MetaDataUtil.getViewIndexPhysicalName(fullDataTableName);
+TableName viewIndexHBaseTable =
TableName.valueOf(physicalViewIndexTableName);
+TestUtil.assertRawRowCount(conn, viewIndexHBaseTable, 2);
+String sql = "SELECT /*+ INDEX(" + fullGrandChildViewName + " " +
viewIndexName + ")*/ "
++ "val2, id2, val1, id3, id1 FROM " +
fullGrandChildViewName
++ " WHERE id2 = 'a2' AND (id1 = 'a1' OR id1 = 'b1') AND
id3 = 3";
+ResultSet rs = stmt.executeQuery("EXPLAIN " + sql);
+String actualQueryPlan = QueryUtil.getExplainPlan(rs);
+String expectedQueryPlan = "CLIENT PARALLEL 1-WAY POINT LOOKUP ON
2 KEYS OVER "
++ physicalViewIndexTableName;
Review Comment:
Could you please also use the same query without hint and assert that the
Explain plan now uses 1-WAY POINT LOOKUP on the base table?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
