Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6402 Allow using local indexes with uncovered columns in the WHERE clause. - #1159
Conversation
| } | ||
| } | ||
| } | ||
| for (Entry<byte[], NavigableSet<byte[]>> entry : familyMap.entrySet()) { |
There was a problem hiding this comment.
This is one of the bug fixes. Above we add columns and column families sometimes that we would miss here in the trackedColumnBitset as well as the column tracker.
This showed in queries like ... WHERE (a,b) IN ((..., ...), (..., ...)) when 'a' is indexed by a local index and 'b' is included. I expected there are potential other scenarios where this is wrong.
| if (whereFilter != null) { | ||
| whereFilterStr = whereFilter.toString(); | ||
| } else { | ||
| byte[] expBytes = scan.getAttribute(BaseScannerRegionObserver.LOCAL_INDEX_FILTER_STR); |
There was a problem hiding this comment.
Expression.readFields does not restore all display properties. Since I cannot change how expressions are (de) serialized I came up with this.
A bit unfortunate that this has to be on the scan and is uselessly also sent to the server - although it's helpful there for debugging.
| IndexUtil.wrapResultUsingOffset(env, result, offset, dataColumns, | ||
| tupleProjector, dataRegion, indexMaintainer, viewConstants, ptr); | ||
| byte[] expBytes = scan.getAttribute(BaseScannerRegionObserver.LOCAL_INDEX_FILTER); |
There was a problem hiding this comment.
This is really the crux of the change.
Get the WHERE expression from the scan and evaluate it after we have assembled the tuple. This cannot be done with just a regular filter since that is evaluated before.
There was a problem hiding this comment.
Does the client issue a new scan for every row to be returned? Otherwise, how does this work if there are multiple rows to be returned to the client for a query? It seems that it is assumed here that the order of rows to be visited in the index CF is the same as the order of of rows to be visited in the data CF (based on how scanTillScanStartRow is used). If so, this assumption is wrong. Maybe I misunderstood the implementation here.
There was a problem hiding this comment.
Thanks for looking @kadirozde .
No, there's only a single scan. Just like filters the expression here remains the same for the entire scan.
The intention here is that every every row that makes through the index scan (that's where start/end row and scanTillScanStartRow come into the picture) is subsequently passed through to the expression to see whether it should be filtered by non-index condition. The key is that is has be done after we merged in the columns from the main column family.
Also note that the filter expression here only contains the none index-filters (see WhereCompiler.setScanFilter(...) and how that is called).
There was a problem hiding this comment.
The scan scans in index order. The other non-index columns are then merged in via an in-region Get to the main column family.
There was a problem hiding this comment.
As @lhofhansl pointed out on a private discussion, the scan initiated by the client is used to visit the index CF and then the data table rows are picked up using Get operations within IndexUtil.wrapResultUsingOffset.
| buf.append('['); | ||
| if (length > 0) { | ||
| for (int i = o; i < length; i++) { | ||
| for (int i = o; i < o+length; i++) { |
There was a problem hiding this comment.
Another bugfix... Annoying.
stoty
commented
Mar 6, 2021
💔 -1 overall
This message was automatically generated. |
dbwong
commented
Mar 7, 2021
Could we get some unit tests for some of the small bugfixes you put in @lhofhansl ? Things like the cases you changed in PVarBinary or similar. |
@dbwong Yep. On my list. :) There are also two tests join tests that are actually failing now. Actually... For the PVarBinary.toStringLiteral() an test is overkill, though. It's obvious that someone just forgot to include the offset.. (I can also remove that change from this PR as it is an unrelated fix that I just came across when debugging) The ColumnTracker order change is hard to isolate in a unit-test. The only case was the that weird local index case I mention above. It's also obvious that we will not add columns to the trackers when they were added after that, but this one could use a good test. |
Latest push fixes the HashJoinLocalIndexIT and SortMergeJoinLocalIndexIT failures. Should be good now. |
lhofhansl
commented
Mar 7, 2021
Added more tests. |
stoty
commented
Mar 8, 2021
💔 -1 overall
This message was automatically generated. |
stoty
commented
Mar 8, 2021
💔 -1 overall
This message was automatically generated. |
lhofhansl
commented
Mar 8, 2021
Fixed some checkstyle warnings - no functional changes. Shortening the line length to 80 makes some of the code unreadable so I did not do that. |
stoty
commented
Mar 8, 2021
💔 -1 overall
This message was automatically generated. |
| } | ||
| } | ||
| columnsTracker.put(cf, cols); | ||
| } |
There was a problem hiding this comment.
I think the side effect of moving this code block here would be adding some extra columns to the result set. I do not think that would create a functional issue but just result in more data to return to the client.
There was a problem hiding this comment.
I think the extra columns would also only be added of they are needed. Without this the construction of the Filters below would work on a different set of columns as compared to the scan objects. It seems we were lucky that we did not run into this.
This does not add more columns to the scan, just to the column tracker and the extra bits to the trackedColumnsBitSet used in the filters below.
But I agree... In the worst case we added extra columns to the tracker(s).
| if (expBytes != null) { | ||
| ByteArrayInputStream stream = new ByteArrayInputStream(expBytes); | ||
| DataInputStream input = new DataInputStream(stream); | ||
| Expression extraWhere = ExpressionType.values()[WritableUtils.readVInt(input)].newInstance(); |
There was a problem hiding this comment.
This new instance needs to be created only once. Here, it is created for each next. We can create the new instance once at the constructor.
There was a problem hiding this comment.
This is lightweight. But I do agree.
There was a problem hiding this comment.
There's no constructor since this is an anonymous class, but I can use an instance initializer.
There was a problem hiding this comment.
I pushed an update using an instance initializer constructing the expression only once.
lhofhansl
commented
Mar 10, 2021
Great thanks. I'll merge in a bit and then cherry pick into 5.1. And also into 4.x, right? |
stoty
commented
Mar 10, 2021
💔 -1 overall
This message was automatically generated. |
This works by passing down the filter expression as a scan attribute and evaluating it after the full tuple has been assembled by merging in columns from the main column family.
Please review this carefully, this is tricky stuff and I ran into a bunch of snags and unexpected details in the process.