[ntuple] Fixes lookup & searching in the descriptor #17004
Conversation
jblomer
force-pushed
the
ntuple-fix-descriptor-search
branch
from
81ca30f
to
e9dd2d5
Compare
| /// May contain only a subset of all the available clusters, e.g. the clusters of the current file | ||
| /// from a chain of files | ||
| std::unordered_map<DescriptorId_t, RClusterDescriptor> fClusterDescriptors; | ||
| std::vector<RExtraTypeInfoDescriptor> fExtraTypeInfoDescriptors; | ||
| std::unique_ptr<RHeaderExtension> fHeaderExtension; | ||
|
|
||
| // We don't expose this publicy because when we add sharded clusters, this interface does not make sense anymore |
There was a problem hiding this comment.
Typo: publicy
Also, it should probably be a TODO?
There was a problem hiding this comment.
I don't think this is a TODO. We never plan to make this a public interface. Because with sharded clusters, this interface would be confusing (multiple possible clusters for the same entry number).
Test Results 18 files 18 suites 3d 22h 41m 49s ⏱️ For more details on these failures, see this check. Results for commit 2c5a443. ♻️ This comment has been updated with latest results. |
| const auto &clusterIds = GetClusterGroupDescriptor(fSortedClusterGroupIds[cgMidpoint]).GetClusterIds(); | ||
| R__ASSERT(!clusterIds.empty()); |
There was a problem hiding this comment.
unrelated (for now): this requires deserializing all page lists to populate all cluster group descriptors. In the future, we may first want to search loaded cluster groups under the assumption that by loading the (global) entry first, we already have the necessary information...
| const auto firstEntryInNextCluster = clusterDesc.GetFirstEntryIndex() + clusterDesc.GetNEntries(); | ||
| return FindClusterId(firstEntryInNextCluster); |
There was a problem hiding this comment.
Is it worth shortcutting the common case here and check if clusterId + 1 contains firstEntryInNextCluster?
| return kInvalidDescriptorId; | ||
| if (clusterDesc.GetFirstEntryIndex() == 0) | ||
| return kInvalidDescriptorId; | ||
| return FindClusterId(clusterDesc.GetFirstEntryIndex() - 1); |
jblomer
force-pushed
the
ntuple-fix-descriptor-search
branch
from
e9dd2d5
to
2c5a443
Compare
|
|
Fixes several instances of lookups in the descriptor from linear to logarithmic complexity. As a result, many of the limit tests results improve significantly. So much so that I think we can turn on most of them on a regular basis.
Relies on #16986