Check that the radix sort and the in-memory sort agree - #404
Open
e-n-f wants to merge 1 commit into
Open
Conversation
The result of a sort shouldn't depend on how the sort was performed, so rather than checking the sorted output against a committed copy of it, check that --prefer-radix-sort, which lowers the memory limit to 8K to force the radix subdivision to recurse, produces the same tiles as sorting in memory. Nothing new has to be kept up to date, and the comparison holds regardless of how deeply the subdivision recurses on a given machine, which depends on how many files it will let us open at once. What sends the sort down the paths that are otherwise almost never taken is the shape of the input rather than the size of it, so two small inputs are generated for the purpose: several well-separated features that are each too big to sort in memory, which are each sorted as a bucket of their own, and many features at one location, which have to be subdivided until there are no index bits left. Between them and tests/feature-filter, all three of radix1()'s branches are covered, including sorting in memory because there are no files left to subdivide with. The whole target runs in about ten seconds. This will not pass until both #402 and #403 have landed: the two inputs above detect the two separate bugs that those fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011wLk2itWETPBAS9a9yE8zu
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
Do not merge before #402 and #403. This target fails on
mainon purpose: the two inputs it generates are what detect the two bugs those PRs fix. It goes green once both are in.Approach
The result of a sort shouldn't depend on how the sort was performed. So rather than checking sorted output against a committed copy of it, this checks that
--prefer-radix-sort— which lowers the memory limit to 8K so the radix subdivision has to recurse — produces the same tiles as sorting in memory. Same shape as the existingparallel-test, which compareslinear-file.jsonagainstparallel-file.jsonrather than against a golden file.Two things follow from that:
radix1()varies from machine to machine — but both runs happen on the same machine and the sorted result is the same either way, so the assertion is stable even though the path isn't. (This is what I'd wrongly worried about when I said in Distinguish duplicate feature locations during the index sort #402 that an-aRtest would risk being flaky. A golden-file test would have been fine too; it's the crash in Keep the radix sort from recursing forever when it runs out of files #403 that was the real hazard, and that's now fixed.)Inputs
What sends the sort down the branches that are otherwise almost never taken is the shape of the input, not the size, so the two purpose-built inputs are small and adversarial rather than large:
bigfeatures.json— eight well-separated 2000-point linestrings. Each is bigger than the lowered memory limit on its own, so each is sorted as a bucket of one feature.onelocation.json— 500 points at identical coordinates. They share the whole index, so they have to be subdivided until there are no bits left.Then
tests/feature-filter,tests/ne_110m_ocean,tests/border,tests/loop,tests/tl_2022_11_tract, andtests/epsg-3857for breadth.Instrumenting the branch taken in
radix1()confirms all three are covered:Worth noting that an earlier version of
bigfeatures.jsonhad a single feature and passed onmaineven though it did reach the buggy path: with only one feature in the whole tileset, the stray byte lands right before the EOF marker and happens to be zero, so it reads as EOF and the corruption is invisible. It needs a following record to desync against, hence eight features rather than one.Runtime
About ten seconds. Wired into both
testandfewer-tests— the latter because the out-of-boundsgeomfiles[which]write described in #403 is exactly what that build's address sanitizer is for.Not included
I left out the
TIPPECANOE_SORT_MEMORY/TIPPECANOE_MAX_FILESoverrides I'd suggested, to keep this to just the test. They aren't needed for it to be correct —-aRis enough of a lever, and the comparison is machine-independent regardless. They'd still be worth having so a test could sweep several budgets and pin the recursion depth by construction instead of inheriting it fromulimit -n; happy to do that separately if you want it.Generated by Claude Code