Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6458 Using global indexes for queries with uncovered columns - #1256
Conversation
Uh oh!
There was an error while loading. Please reload this page.
| if (dataRegion != null) { | ||
| joinResult = dataRegion.get(get); | ||
| } else { | ||
| TableName dataTable = |
There was a problem hiding this comment.
I wonder if this part is even still needed for local indexes. (but that's a separate issue)
stoty
commented
Jun 28, 2021
💔 -1 overall
This message was automatically generated. |
| table = ServerUtil.ConnectionFactory. | ||
| getConnection(ServerUtil.ConnectionType.INDEX_WRITER_CONNECTION, environment). | ||
| getTable(TableName.valueOf(dataTableName)); | ||
| joinResult = table.get(get); |
There was a problem hiding this comment.
This is doing a remote get for each row that matches the index. The index would have to be very selective for this to be an improvement.
I was hoping we'd come up with some smart batching. But that's tricky to do here. :)
There was a problem hiding this comment.
I was not sure if I do it in this PR or open a separate jira for that. We can buffer lots of data row keys in memory and then use a skip scan filter and even multiple threads to issue a separate scan for each data table region. Essentially, this is what we do for reverse index verification. However, it requires quite a bit code refactoring. Please note this PR updates both client and server code. If we leave the improvement to the second PR, that will update only the server side. What do you think?
There was a problem hiding this comment.
I think that's perfectly fine to do in a separate PR.
And, yes, doing buffering will require quite some refactoring, since you (a) have to do it at a place where you still know the rows involved, and (b) a level higher than here, so that you can buffer.
The only concern I have that in case using an index can be significantly slower than a full scan (this is true even for local indexes). For example a query of the form SELECT colA FROM table WHERE colB > 0; assuming colB is not selective will take longer. Note that FAST_DIFF (unfortunately Phoenix' default) is particularly slow for Get operations. My guess is that with defaults if the WHERE is not 99% - 99.9% selective, the query might be slower.
In any case :) Another PR is cool.
There was a problem hiding this comment.
I have created https://issues.apache.org/jira/browse/PHOENIX-6501 for this improvement
stoty
commented
Jun 28, 2021
💔 -1 overall
This message was automatically generated. |
@kadirozde@lhofhansl FYI. 1.You said "Phoenix client does not use a global index for the queries with the columns that are not covered by the global index" is not right , In QueryOptimizer.addPlan, for the sql with the columns that are not covered by the global index, if user specify a Index Hint and there exists where clause, the sql would be rewritten as 2.Whether or not scaning the gobal index and retrieving the corresponding rows from the data table is better than just scaning the data table is a complex problem, because there are many factors we need to consider such as Network cost, random disk access cost , data distribution , column selective etc. You said "It is expected that such performance improvement will happen when the index row key prefix length is greater than the data row key prefix length for a given query" is extremely insufficient. Lack of a CBO framework in Phoenix, seems that it is sensible to be conservative, I think it is better to left whether or not select this strategy to user by user specifying the Index Hint just as the existing code. |
stoty
commented
Jun 29, 2021
💔 -1 overall
This message was automatically generated. |
kadirozde
commented
Jun 29, 2021
What I meant is that by default the uncovered global index is not used. One can construct a query plan manually using hints as you pointed it out to use the uncovered global index. Please note that you can also construct a SQL join statement and achieve the same thing.
I agree that there is no guarantee that the index always performs better. However, based on my experience, it will perform better in most of the cases in practice. This is because the index PK is designed by the user who knows the use case (the type and shape of queries) and the user wants that the index should be used if the index row key prefix length is greater than the data row key prefix length for a given query in general. I understand your concern here and please help me out on how to proceed here. I can add a config param to use uncovered indexes without a specific hint. This mean that we will preserve the existing behavior if the config param is not specified. Would that address your concern? |
@kadirozde ，thank you for reply.
If there is just one index, your said may be right, but if there is lots of index, it is hard to make sure what is the user's intention, may be the user just leave out some columns in the global index.
In my opinion, your implemention now is better than the existing implemention which rewrite the sql with the columns that are not covered by the global index as In short , I think if we could not make sure the index performs better, we would better be conservative and let the user to decide rather than making decisions for the user. In any case, you may also let the user know you scaning the gobal index and retrieving the corresponding rows from the data table when they execute explain sql. |
If you do SELECT count(uncovered_column) FROM T WHERE covered_column = xyz, the global uncovered index is not used even when you hint it as expected (I just verified that current 5.x. Phoenix). I found that uncovered local indexes (that's what I tested) are sometimes much slower than doing to a full table scan. That happens when there is a WHERE clause that an index could be used for, but the WHERE restriction is not selective. (As noted above, FAST_DIFF (Phoenix' default) is actually the worst choice since SEEKs are slow with it. ROW_INDEX_V1 with ZSTD compression are far better. I blogged about this here: https://hadoop-hbase.blogspot.com/2018/10/apache-hbase-and-apache-phoenix-more-on.html a while ago: With FAST_DIFF the WHERE clause needed to be 0.5% (return 1/200 of the data) to be effective. With ROW_INDEX_V1 + ZSTD that was 10%.) This is at best as good as uncovered local indexing, and probably worse since we need to go remote for each row, unless we do batching. And the batches would still be requiring a SKIP_SCAN, which in the general case is still very slow for FAST_DIFF. Anyway... I think we should check this in. Presumable folks would create uncovered global indexes only when they know what they are doing. |
comnetwork
commented
Jul 7, 2021
@lhofhansl ，thank you for reply, |
| @@ -156,7 +155,7 @@ public static PTable createProjectedTable(SelectStatement select, StatementConte | |||
| } | |||
| // add LocalIndexDataColumnRef | |||
There was a problem hiding this comment.
nit: IndexDataColumnRef in place of LocalIndexDataColumnRef?
| @@ -1168,8 +1168,8 @@ public ColumnRef resolveColumn(String schemaName, String tableName, String colNa | |||
| colRef = super.resolveColumn(schemaName, tableName, colName); | |||
| } catch (ColumnNotFoundException e) { | |||
| // This could be a ColumnRef for local index data column. | |||
There was a problem hiding this comment.
nit: we can remove local reference here?
| try { | ||
| table = environment.getConnection().getTable(dataTable); | ||
| joinResult = table.get(get); | ||
| } finally { | ||
| if (table != null) table.close(); | ||
| } |
There was a problem hiding this comment.
Good to replace with try-with-resources?
| try { | ||
| table = environment.getConnection().getTable(dataTable); | ||
| table = ServerUtil.ConnectionFactory. |
There was a problem hiding this comment.
Same here reg try-with-resources
| @Deprecated | ||
| public static final String LOCAL_INDEX_FILTER = "_LocalIndexFilter"; | ||
| @Deprecated | ||
| public static final String LOCAL_INDEX_LIMIT = "_LocalIndexLimit"; | ||
| @Deprecated | ||
| public static final String LOCAL_INDEX_FILTER_STR = "_LocalIndexFilterStr"; |
There was a problem hiding this comment.
I believe these deprecated fields can be removed only after we come to a major release (e.g 6.x/7.x) where server running on that release can no longer be directly supported by client version <= 4.16/4.17, is that correct?
kadirozde
commented
Jul 24, 2021
@Lars already enhanced the explain plan to indicate the server side merge for uncovered columns in PHOENIX-6409. Regarding to merging this PR and the existing subquery implementation without creating compatibility issues, it is a bit tricky for me now. I need to spend time to figure that out. Would you please create a jira for that? If you like to implement that jira, you would be more than welcome, @comnetwork. |
stoty
commented
Jul 25, 2021
💔 -1 overall
This message was automatically generated. |
kadirozde
commented
Sep 7, 2021
@lhofhansl , @comnetwork, I have done some performance testing on a cluster with 15 region servers. I created a data table with 16 million rows. Each row is about 2500 bytes. The row key of this table is composed of four fields (VARCHAR, INTEGER, TIMESTAMP, VARCHAR). I run the same test without an index, with a covered index and with an uncovered index. The timestamp field is indexed. The query used in the test returned N rows that fall in to the a supplied timestamp range, where N is supplied as the limit parameter. The query returns four fields. The query times in ms are as follows: limit covered uncovered no index It is clear that if the number of selected rows is large (in this case 10000 or more) the uncovered index starts to perform worse than the full table scan. No sure if these results are generalizable. Instead of using an uncovered index by default, I will add a logic to use an uncovered index only if it is given as a hint. |
kadirozde
commented
Jan 27, 2022
@comnetwork@lhofhansl, I have updated the PR such that the uncovered global indexes will be used only when the index hint is provided as @comnetwork suggested. |
lhofhansl
commented
Feb 22, 2022
Sorry for the late reply. I have not looked the updated PR, gating this on a hint seems fine. Let's make this hint is for global indexes only, and does not apply to local indexes. |
| } | ||
| } | ||
| } else if (ScanUtil.isUncoveredGlobalIndex(scan)) { | ||
| byte[] dataTableName = scan.getAttribute(PHYSICAL_DATA_TABLE_NAME); |
There was a problem hiding this comment.
Can we have a test where the index table has a different Physical Table name and data table has a different physical table name?
There was a problem hiding this comment.
Not sure if I understood this comment. Data and index tables have different physical tables always.
There was a problem hiding this comment.
This code reminded me a test case where we have separate physical and logical table names for (case1. data table and case2. index table). I was just asking to make sure that we are not breaking anything.
Something like this:
You have an index hint for a global index which has a different physical table name (logical name appears in the hint but its PHYSICAL_TABLE_NAME in syscat points to different hbase table). I think your code doesn't break this case but I wanted to make sure. Does this make sense?
There was a problem hiding this comment.
I see. I think this concern is outside the context of this PR as this PR does not change the existing index hint implementation or how we pass the data table physical name via a scan attribute.
| } | ||
| if (ScanUtil.isLocalIndex(scan) && !ScanUtil.isAnalyzeTable(scan)) { | ||
| if ((ScanUtil.isLocalIndex(scan) | ||
| || ScanUtil.isUncoveredGlobalIndex(scan)) |
There was a problem hiding this comment.
You seem to have this in Line 137 and in GroupedAggregateRegion... as well. I think it is better to have a ScanUtil.isLocalOrUncoveredGlobalIndex
| int clientVersion = ScanUtil.getClientVersion(scan); | ||
| List<IndexMaintainer> indexMaintainers = | ||
| IndexUtil.deSerializeIndexMaintainersFromScan(scan); | ||
| indexMaintainer = indexMaintainers.get(0); |
There was a problem hiding this comment.
Why are we getting the first one? I see that we used to do this before and it is not new but I am confused why how we sort these
There was a problem hiding this comment.
These methods can serialize/deserialize more than one index maintainers. I do not know if there is a case where we do that actually. As far as know, we only need to do this only for one index maintainer.
| serializeIndexMaintainerIntoScan(scan, dataTable); | ||
| // Set view constants if exists. | ||
| serializeViewConstantsIntoScan(scan, dataTable); | ||
| if (table.getIndexType() == IndexType.LOCAL) { |
There was a problem hiding this comment.
Don't we need this for global uncovered ones too?
There was a problem hiding this comment.
No. We populate scan attributes for global indexes at the constructor of the TableResultIterator using ScanUtil#setScanAttributesForClient in general when the table to be scanned is a global index table.
| " SERVER FILTER BY FIRST KEY ONLY\n" + | ||
| " DYNAMIC SERVER FILTER BY \\(\"" + dataTableName + ".K1\", \"" + dataTableName + ".K2\"\\) IN \\(\\(\\$\\d+.\\$\\d+, \\$\\d+.\\$\\d+\\)\\)"; | ||
| assertTrue("Expected:\n" + expected + "\ndid not match\n" + actual, Pattern.matches(expected, actual)); | ||
| //assertTrue("Expected:\n" + expected + "\ndid not match\n" + actual, Pattern.matches(expected, actual)); |
There was a problem hiding this comment.
nit: commented code. Forgotten?
There was a problem hiding this comment.
Yes, it was. I will fix it
kadirozde
commented
Feb 23, 2022
Yes, the index hint is required only for global indexes. |
kadirozde
commented
Feb 24, 2022
@gokceni, Thank you for approving the updated version. @lhofhansl and @comnetwork, thank you for reviewing the earlier versions and I have updated the PR based on your comments. I am going to merge this now. |
The Phoenix query optimizer does not use a global index for a query with the columns that are not covered by the global index if the query does not have the corresponding index hint for this index. With the index hint, the optimizer rewrites the query where the index is used within a subquery. With this subquery, the row keys of the index rows that satisfy the subquery are retrieved by the Phoenix client and then pushed into the Phoenix server caches of the data table regions. Finally, on the server side, data table rows are scanned and joined with the index rows using HashJoin. Based on the selectivity of the original query, this join operation may still result in scanning a large amount of data table rows.
Eliminating these data table scans would be a significant improvement. To do that, instead of rewriting the query, the Phoenix optimizer simply treats the global index as a covered index for the given query. With this, the Phoenix query optimizer chooses the index table for the query especially when the index row key prefix length is greater than the data row key prefix length for the query. On the server side, the index table is scanned using index row key ranges implied by the query and the index row keys are then mapped to the data table row keys (please note an index row key includes all the data row key columns). Finally, the corresponding data table rows are scanned using server-to-server RPCs. PHOENIX-6458 (this PR) retrieves the data table rows one by one using the HBase get operation. PHOENIX-6501 replaces this get operation with the scan operation to reduce the number of server-to-server RPC calls.