PHOENIX-4925 Use a Variant Segment tree to organize Guide Post Info #482
Conversation
|
@BinShi-SecularBird can you fix the conflicts? |
phoenix-core/src/main/java/org/apache/phoenix/iterate/SnapshotScanner.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/schema/stats/DefaultStatisticsCollector.java
Show resolved
Hide resolved
dbwong
left a comment
dbwong
left a comment
There was a problem hiding this comment.
Initial high level review. Generally okay with the high level picture but I think the tree classes need unit tests badly before I review in detail.
| // The size of the thread pool used for refreshing cached table stats in stats client cache | ||
| public static final String STATS_CACHE_THREAD_POOL_SIZE = "phoenix.stats.cache.threadPoolSize"; | ||
| // The targeted size of the guide post chunk measured in the number of guide posts. | ||
| public static final String STATS_TARGETED_CHUNK_SIZE = "phoenix.stats.targeted.chunk.size"; |
There was a problem hiding this comment.
I don’t think this should be on the query level and Should be on the table level sicne we won't be collecting stats for the table multiple times ideally. With you guidepost chunk shouldn't this match the collected size? Is this for a 1 size chunk for backward compatibility?
There was a problem hiding this comment.
This is a cluster configuration applied to all queries and all tables. Isn't it the right place to add a cluster configuration?
There was a problem hiding this comment.
Mmm what about the case where the chunk size here is > the table guide post size for example will you combine chunks?
There was a problem hiding this comment.
if we are doing it on the table level, we have to add an extra column to store this chunk size. Adding more and more columns on the syscat is not a good idea, what's the best practice here @dbwong ?
There was a problem hiding this comment.
Chunk size is max(1, count of guide posts).
phoenix-core/src/main/java/org/apache/phoenix/schema/stats/DefaultStatisticsCollector.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * A guide post chunk is comprised of a group of guide posts, and it has one of key ranges below: | ||
| * (UNBOUND, gp_i0], (gp_i0, gp_i1], (gp_i1, gp_i2], ..., (gp_in, gp_n], (gp_n, UNBOUND) | ||
| * where gp_x is one of guide post collected on the server side. The last guide post chunk is a DUMMY chunk |
There was a problem hiding this comment.
Let’s call it something other than dummy maybe residual? And describe that it covers the key range not covered by the guideposts
There was a problem hiding this comment.
Changed name to ending chunk.
phoenix-core/src/it/java/org/apache/phoenix/end2end/ExplainPlanWithStatsEnabledIT.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/it/java/org/apache/phoenix/schema/stats/BaseStatsCollectorIT.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/schema/stats/GuidePostsInfo.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/schema/stats/GuidePostsInfo.java
Show resolved
Hide resolved
| Construct(this.guidePostChunks, 0, n - 1, 0); | ||
| } else { | ||
| this.treeSize = 0; | ||
| this.nodes = null; |
There was a problem hiding this comment.
dislike the use of null for presence consider Optional.
There was a problem hiding this comment.
I don't need to check whether nodes is null and there is no nested check for null object, so use Optional class is overkilled here.
| */ | ||
| public boolean trackGuidePost(ImmutableBytesWritable row, long byteCount, long rowCount, | ||
| long updateTimestamp) { | ||
| public boolean trackGuidePost(ImmutableBytesWritable row, long byteCount, long rowCount, long updateTimestamp) { |
There was a problem hiding this comment.
This isn't really following the builder pattern. consider rename or handling through exceptions or changing the caller.
There was a problem hiding this comment.
This intend not to use builder pattern here. No need to use builder pattern.
| /** | ||
| * The index of the guide post chunk in the chunk array. | ||
| */ | ||
| private final int guidePostChunkIndex; |
There was a problem hiding this comment.
This seems to be leaking the top level classes lower into this class. The accessor is never used. Can you briefly explain why this index is needed to be tracked in multiple levels? InnerPointLookupResult in GuidePostsInfo even has 2 separate indexes tracked.
There was a problem hiding this comment.
It's used in DecodedGuidePostCache to track the guide post chunks in the cache.
|
@dbwong , could you please review BaseResultIterators.getParallelScans()? All the required changes should be there now. BaseResultIterators.getRowKeyRanges() contains the logic of generating query key ranges for local index. |
dbwong
left a comment
dbwong
left a comment
There was a problem hiding this comment.
Initial thoughts feedback on the code in BaseResultIterators @BinShi-SecularBird
phoenix-core/src/main/java/org/apache/phoenix/iterate/BaseResultIterators.java
Outdated
Show resolved
Hide resolved
| // We'll never have a case where a table is both salted and local. | ||
| assert !(isSalted && isLocalIndex); | ||
|
|
||
| Long pageLimit = getUnfilteredPageLimit(scan); |
There was a problem hiding this comment.
I had a thought for a followup JIRA. If pageLimit approaches the size of our stored guideposts we can probably improve our estimate from a combination of data.
phoenix-core/src/main/java/org/apache/phoenix/iterate/BaseResultIterators.java
Show resolved
Hide resolved
| boolean crossesRegionBoundary = false; | ||
|
|
||
| if (j == keyRangeCount - 1) { | ||
| crossesRegionBoundary = true; |
There was a problem hiding this comment.
My understanding is crossesRegionBoundary was only needed for LocalIndexes or Salted Tables. Maybe we should rearrange to indicate? It is also used in addNewScan to force add the scan without useStatsForParallelization, but again I believe that should only be true for local indexes or salted tables. Ideally maybe we remove the logic there in addNewScan?
There was a problem hiding this comment.
I tried to keep the same semantics as the old code. See line 1102 "Scan newScan = scanRanges.intersectScan(scan, currentKeyBytes, endKey, keyOffset, true);" in BaseResultIterators.getParallelScans() in original code.
phoenix-core/src/main/java/org/apache/phoenix/iterate/BaseResultIterators.java
Show resolved
Hide resolved
| } | ||
| regionIndex++; | ||
|
|
||
| regionScans = addNewScan(parallelScans, regionScans, newScan, endKey, crossesRegionBoundary, regionLocation); |
There was a problem hiding this comment.
Lets discuss the naming and usage a bit of this as in ParallelScanGrouper this is "startKey". This is used for the crossesPrefixBoundary check and indicated in the interface as a startKey and compared to the last scans startRow Prefix looking for for example the salt byte changing or region changing. By using end key is there any cases we might miss?
There was a problem hiding this comment.
Let double check this. This is exactly one of the places where we need to pay attention to. Hopefully I'll add test cases to cover the scenario.
phoenix-core/src/main/java/org/apache/phoenix/iterate/BaseResultIterators.java
Outdated
Show resolved
Hide resolved
Change the return type of splitKeyRangesByBoundaries() from List<KeyRange> to List<Pair<Integer, List<KeyRange>>>. Handled feedback.
binshi-bing
changed the title
phoenix-core/src/main/java/org/apache/phoenix/schema/stats/GuidePostChunk.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/schema/stats/StatisticsUtil.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/schema/stats/StatisticsWriter.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/schema/stats/StatisticsWriter.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { |
There was a problem hiding this comment.
In java typically if you need a custom equals you need a custom hashcode function.
There was a problem hiding this comment.
It seems that it's not necessary. I can see my equals function is called for comparison.
There was a problem hiding this comment.
https://stackoverflow.com/questions/30026097/equals-without-hashcode
ppl believe without hashcode function might result in weird bugs.
There was a problem hiding this comment.
I added hashCode function, although the bug exists when they are used for hash containers and these objects shouldn't be used for hash containers.
| scanStartKey = ScanUtil.getMinKey(schema, newSkipScanFilter.getSlots(), slotSpan); | ||
| scanStopKey = ScanUtil.getMaxKey(schema, newSkipScanFilter.getSlots(), slotSpan); | ||
| scanStartKey = ScanUtil.getMinKey( | ||
| newSkipScanFilter.getSchema(), newSkipScanFilter.getSlots(), newSkipScanFilter.getSlotSpan()); |
There was a problem hiding this comment.
Why we made this change? Whats' the diff btw 'RowKeySchema' from the ScanRangers and SkipScanFilter.getSchema()?
There was a problem hiding this comment.
When the PK column type is varchar, the column is translated to multiple char[1] columns in SkipScanFilter's schema, and the SkipScanFilter's schema is supposed to use here, otherwise it will cause the test failures in SkipScanFilterIntersectTest.java.
|
|
||
| final boolean isStatsEnabled = config.getBoolean(STATS_COLLECTION_ENABLED, DEFAULT_STATS_COLLECTION_ENABLED); | ||
| if (queryServices == null || !isStatsEnabled) { | ||
| if (queryServices == null || ! isStatsEnabled) { |
There was a problem hiding this comment.
I always add an extra space between ! and the variable for better readness. I saw the other places use this coding style too.
| assertFalse(rs.next()); | ||
| // Hit the first guide post. | ||
| info = getByteRowEstimates(conn, sql, binds); | ||
| estimatedRows = 10; |
There was a problem hiding this comment.
By default, one guide post refers to 10 rows?
There was a problem hiding this comment.
Because the query will hit the first guide post which has key 100. This guide post contains 10 estimated rows.
4 participants
@dbwong please review this early draft. Haven't completely sorted out local index and cross region boundary condition when generating scan. Still adding unit test and integration test and trying to pass the tests. Please focus on main logic, data structure, main algorithms and interfaces first. The main data structure change is in GuidePostsInfo for now.