Cache constant polygon by raw column value in pointInPolygon

[groeneai]

Changelog category (leave one):

• Performance Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Improve performance of the pointInPolygon function with a constant polygon. The preprocessed-polygon cache is now keyed on the raw constant arguments, so a constant polygon is parsed only once (on a cache miss) instead of being re-parsed on every input block.

Description

Closes #106596. Rebased on top of #107247 (now merged).

Root cause

For a constant polygon argument, FunctionPointInPolygon::executeImpl preprocesses the polygon into a grid / R-tree and caches the preprocessed impl in a process-wide cache keyed by a 128-bit hash. The problem is the order of operations: the cache key is computed from the parsed boost::geometry object, so every invocation of executeImpl (one per input block) first parses the constant polygon just to compute the key:

parseConstMultiPolygon(arguments, multi_polygon);          // build a temporary MultiPolygon
auto entry = known_polygons.getOrSet(hash128(multi_polygon), load);

The cache stores the expensive preprocessing but not the parse, so a query that scans many blocks re-parses and re-hashes the (potentially huge) constant polygon on every block. This is the ARM regression in #106596.

Relationship to #107247

#107247 reworked this cache into a shared, bounded LRU (CacheBase) for the memory-growth issue (#106393). It did not change the order of operations: the constant-polygon lookup key is still a hash of the parsed geometry, and parseConst* still runs on every block before getOrSet. So #107247 did not remove the per-block parse behind #106596. This PR is orthogonal and sits on top of it.

Fix

Key the cache on the raw constant arguments (IColumn::updateHashWithValue over the constant columns) and move the parse into the cache-miss load function. On a cache hit, no parse happens at all.

The key also folds in two things that change how the same raw bytes are interpreted, because parsing (and its validity check) now runs only on a miss:

• the validate_polygons setting: otherwise an entry built with validate_polygons = 0 would satisfy a validate_polygons = 1 lookup and skip the validation that should raise an exception;
• the argument types: coordinates are cast to Float64 during parsing, so the same raw bytes under a different declared type would parse to different coordinates.

Moving the parse under getOrSet is exception-safe with the new CacheBase: it inserts the entry only on a successful load, so a failed parse/validation does not leave a dead entry in the cache.

Test

04411_point_in_polygon_const_cache_key covers the validate-mode ordering (both orders), that a failed validation does not poison the cache, that type-distinct polygons are cached independently, and the multipolygon path. Verified locally that the test fails without the validate flag in the key (a validate_polygons = 1 query that should raise BAD_ARGUMENTS instead returns a result) and passes with the fix.

Version info

• Merged into: 26.7.1.154 (included in 26.7 and later)

[groeneai]

Pre-PR validation gate

#	Question	Answer
a	Deterministic repro?	Yes. The exact benchmark query (500K points, 1000-polygon multipolygon) on a debug build gives a stable median server query_duration_ms of ~449 ms, and the slowdown scales deterministically with block count: ~0.53 s at default max_block_size vs ~0.71 s at max_block_size=4096 (more blocks -> more re-parses). Not a timing flake.
b	Root cause explained?	Yes. For a constant polygon, executeImpl calls parseConstMultiPolygon/parseConstPolygon to build a temporary boost::geometry object, then keys the preprocessed-impl cache on sipHash128(parsed_object). So the parse + hash run on every executeImpl invocation (once per input block), even though preprocessing is already cached (PolygonsAddedToPool stays 0). The reported ARM regression came from an ActionsDAG/FilterTransform window that increased how often executeImpl runs over the constant, amplifying this pre-existing per-block parse cost.
c	Fix matches root cause?	Yes. The cache key is computed from the raw constant column via IColumn::updateHashWithValue before any parse, and the parse is moved into the cache-miss factory. This eliminates the redundant parse on the hot path, addressing the mechanism directly rather than a symptom. No bounds widened, no tags added, no data reduced.
d	Test intent preserved / new tests added?	Preserved. No test was weakened. The existing point_in_polygon stateless tests assert exact output against committed .reference files (pre-fix behavior); they all still pass, which is the regression guard for geometry correctness. No new functional test is added because behavior is unchanged and the existing perf test (point_in_polygon_3d_huge_multipolygon) already covers the performance assertion.
e	Both directions demonstrated?	Yes. Before fix (Build ID 73b189...): median 449 ms server-side, ~0.53 s client, block-size-sensitive. After fix (Build ID 568daa1e..., verified differing and matching the running server's build id): median 36 ms, ~0.09 s, block-size-insensitive. Result unchanged at count() = 500000 in both.
f	Fix is general, not a narrow patch?	Yes. Both constant-polygon branches (multipolygon and polygon) are fixed via one shared hashConstPolygonArguments helper, so there is no sibling code path left with the old parse-then-hash pattern. The now-dead geometry hash helpers are removed. The non-const path is unaffected (it never used the cache). Not a defensive guard.

Note: measured on a local x86 debug build, so absolute numbers are not comparable to the ARM release figures in the issue; the mechanism is architecture-independent. ARM-hardware confirmation is requested from the reporter.

Session id: cron:clickhouse-worker-slot-4:20260622-163100

[groeneai]

cc @nikitamikhaylov @scanhex12 — could you review this? It keys the pointInPolygon constant-polygon cache on the raw constant column value instead of on the parsed boost::geometry object, so a constant polygon is parsed once (on cache miss) instead of re-parsed and re-hashed on every input block. Fixes the ARM regression in #106596 (server-side query duration 449 ms -> 36 ms on a local debug build).

[nihalzp]

The proper fix is here for the regression:

• Make the pointInPolygon preprocessed polygon cache shared, bounded, and configurable #107247

We can consider this once the above PR is merged.

[clickhouse-gh]

Workflow [PR], commit [2581f24]

Summary: ✅

• Performance Comparison: Performance dashboard

---

AI Review

Summary

This PR changes pointInPolygon so constant polygon cache keys are computed from the raw constant polygon arguments, plus validate_polygons, argument type names, and a polygon/multipolygon discriminator, then parsing and preprocessing happen only on a CacheBase miss. The current code addresses the earlier cache-key aliasing and exception-safety concerns, and I did not find any unresolved correctness, safety, or performance issue that warrants an inline finding.

Missing context / blind spots

• ⚠️ I could not run the local stateless test or the performance-report helper in this checkout because no built clickhouse binary is available in PATH; the PR/linked issue discussion contains focused before/after measurements for the affected point_in_polygon cases, and the Praktika CI helper currently reports no failed checks.

Final Verdict

Status: ✅ Approve

[groeneai]

Thanks for the pointer. I read #107247: it reworks the cache into a shared, bounded LRU for the memory-growth issue (#100759 / #106393), which is valuable, but the constant-polygon lookup key there is still sipHash128(parsed_polygon) and parseConstMultiPolygon / parseConstPolygon still run before the lookup on every input block. So #107247 does not remove the per-block re-parse that drives the #106596 regression.

That per-block parse is exactly what the reporter's own validated prototype in #106596 targets ("constant-column cache key before parse; parse only on cache miss", before_after -95.355%), which is what this PR does: key the cache on the raw constant column via updateHashWithValue and move the parse into the cache-miss factory.

So the two are orthogonal and both touch the same region. I am happy to hold this behind #107247; once it lands I can fold the raw-column cache key into its bounded-cache lookup as a small follow-up so the parse-cost fix is not lost (or rebase this on top now, whichever you prefer).

[groeneai]

Pre-PR validation gate (follow-up commit: validate flag in cache key)

Addressing the 00500_point_in_polygon_bug_3_linestring_rotation_precision failure introduced by the first commit on this PR.

#	Question	Answer
a	Deterministic repro?	Yes. SET validate_polygons=0; <invalid polygon query>; SET validate_polygons=1; <same invalid polygon query> returns 0 instead of throwing. The first query caches a usable impl under the raw-column key; the second reuses it. Confirmed on the local debug build (Build ID 6650a6ff): without the fix the second query returns 0; with it the second query throws Code: 36 BAD_ARGUMENTS.
b	Root cause explained?	The validity check (bg::is_valid -> throw BAD_ARGUMENTS) lives inside parseConstPolygon/parseConstMultiPolygon. The first commit moved those parses into the cache-miss factory, but the cache key (hashConstPolygonArguments) did not include the per-query validate_polygons flag, and the object pool is a process-wide static shared across all queries. So a validate_polygons=0 entry on an invalid polygon satisfied a later validate_polygons=1 lookup on the same polygon, the factory was skipped, and validation never ran -> no throw. The targeted check repeats the test file, so one iteration's validate_polygons=0 line poisoned the next iteration's validate_polygons=1 line.
c	Fix matches root cause?	Yes. The cached impl is geometry-only; the only thing validate controls is whether an invalid polygon throws. Adding validate to the cache key makes validate=0 and validate=1 distinct entries, so a validate=1 lookup can never be served by a validate=0 entry. An invalid polygon under validate=1 re-runs the throwing factory; a factory that throws caches nothing (the pool entry is left empty), so it re-throws on every call. Not a band-aid: keeping validation outside the cache would re-introduce the per-block parse and undo the perf win, so keying on validate is the only fix that preserves both.
d	Test intent preserved / new tests added?	Preserved. No test was weakened; 00500_point_in_polygon_bug_3_linestring_rotation_precision (which exists precisely to assert the BAD_ARGUMENTS throw on an invalid polygon) now passes again, single run and --test-runs 30. The existing test already covers this regression class, so no new test is needed.
e	Both directions demonstrated?	Yes. Before the follow-up commit: the cross-mode repro returns 0,0 and the test FAILS. After: the repro throws Code: 36 on the validate=1 query and the full 16-test point_in_polygon stateless suite passes.
f	Fix is general, not a narrow patch?	Yes. The single helper hashConstPolygonArguments is used by both the polygon and multipolygon branches, so the one change covers both const paths (no sibling path left). The non-const path does not use the cache and is unaffected. Perf win confirmed unchanged: single-thread 123-block scan of a fresh multipolygon shows PolygonsAddedToPool=1 (parse runs once, not per block), and a repeat query shows 0 (pure cache hit).

Session id: cron:clickhouse-worker-slot-1:20260622-184700

[clickhouse-gh]

📊 Cloud Performance Report

✅ AI verdict: no_change — no significant changes across 38 queries analysed

This PR changes only the pointInPolygon constant-polygon cache key (keying on raw arguments instead of the parsed geometry). ClickBench Q15 is a plain GROUP BY/count aggregation that never invokes pointInPolygon, so the flagged -11.49% improvement cannot plausibly come from this change. The underlying measurements were also fairly noisy on the new build, so we read this as run-to-run variance rather than a real PR effect and downgraded it to not-sure. No other queries showed a real shift.

clickbench

⚠️ 1 inconclusive

Flagged queries (1 of 43)

	Query	Verdict	Baseline median (ms)	PR median (ms)	Change	q-value	Hint
⚠️	15	not_sure	248	220	-11.5%	<0.0001	aggregation: PR only touches pointInPolygon's const-polygon cache key; Q15 is a GROUP BY/count aggregation that never calls it. Off-p

_{q-value = BH-FDR adjusted p; smaller is stronger evidence. MIRAI flags a query when q < fdr_q (default 0.10) — the value the verdict is based on.}

tpch_adapted_1_official

🟢 No significant changes

Debug info

• StressHouse run: 450377a3-f91a-4e5a-8bd7-2380656ba284
• MIRAI run: e53fddcd-0579-4137-8f1f-8af1c4e879d1
• PR check IDs:
  • clickbench_153619_1782487261
  • clickbench_153625_1782487261
  • clickbench_153632_1782487261
  • tpch_adapted_1_official_153642_1782487262
  • tpch_adapted_1_official_153654_1782487261
  • tpch_adapted_1_official_153665_1782487261

[groeneai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. With the validate flag omitted from the key, SELECT pointInPolygon((p), <invalid_poly>) SETTINGS validate_polygons = 0 followed by the same query with validate_polygons = 1 returns a result instead of raising BAD_ARGUMENTS on every run. The performance side is deterministic per-block parsing visible in the #106596 benchmark.
b	Root cause explained?	The cache key was a hash of the parsed geometry, so parseConst* ran on every input block before getOrSet, re-parsing the constant polygon each block (the #106596 regression). #107247 reworked the cache into a bounded LRU but kept this order, so it did not remove the parse.
c	Fix matches root cause?	Yes. The key is now computed from the raw constant arguments before any parse, and the parse is moved into the cache-miss load function, so a hit does no parse. Directly removes the per-block parse.
d	Test intent preserved / new tests added?	New test 04411_point_in_polygon_const_cache_key added (validate-mode ordering both ways, no-cache-poisoning on failed validation, type-distinct caching, multipolygon path, many-block functional). No existing test weakened.
e	Both directions demonstrated?	Yes. The new test FAILS without the validate flag in the key (a validate_polygons = 1 query that must raise BAD_ARGUMENTS instead returns a result) and PASSES with the fix (10/10 deterministic). The whole 00500_point_in_polygon* suite and #107247's 04337 test also pass.
f	Fix is general across code paths?	Yes. Both branches (polygon and multipolygon) use the same hashConstPolygonArguments keying and the same parse-in-load pattern; the discriminator byte keeps their shared-cache key spaces separate.
g	Fix generalizes across inputs (params/datatypes/wrappers)?	Yes. The key includes each argument's type->getName(), so same-raw-byte / different-declared-type polygons (which cast to different Float64 coordinates) do not alias; and the validate_polygons flag, so the two validate modes are cached independently. Tested across Int32/Float64 coordinate types and both validate modes.
h	Backward compatible?	Yes. No setting default, on-disk/wire format, or behavior change. Same results as before; only the per-block parse is eliminated.
i	Invariants and contracts preserved?	Yes. CacheBase::getOrSet propagates load exceptions and inserts the entry only on a successful load, so moving parseConst* (which can throw under validate_polygons = 1) into load cannot leave a dead entry; the previous validate-before-cache validation contract is preserved by folding validate_polygons into the key.

Session id: cron:clickhouse-worker-slot-0:20260625-220800

[groeneai]

@nihalzp #107247 is merged now, thanks. I rebased this on top of it.

#107247's bounded cache still keys the constant-polygon lookup on the parsed geometry and re-parses on every input block, so the per-block parse behind #106596 remained. This PR keys the cache on the raw constant arguments and moves the parse into the cache-miss load function, so a hit does no parse. The key folds in validate_polygons and the argument types (parse runs only on a miss), and the cache-miss insert is exception-safe on the new CacheBase (entry inserted only on a successful load). New test 04411_point_in_polygon_const_cache_key added. Ready for review.

[groeneai]

cc @nikitamikhaylov @scanhex12 - could you review this? It keys the pointInPolygon constant-polygon cache on the raw constant arguments (plus validate_polygons and the argument types) and moves the parse into the cache-miss load function, so a constant polygon is parsed once instead of on every input block. This removes the per-block parse behind the ARM regression in #106596, which #107247's bounded cache did not address (it still re-parses before the lookup).

[groeneai]

CI finish ledger — 480c9f8

Every failure below has an owner: a fixing PR (ours or external), or a full-effort fix task whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Integration tests (amd_msan, 4/6) / test_backup_restore_new::test_async_backup_restore_with_max_execution_time_zero (+ Timeout)	flaky (3 unrelated PRs/30d, 0 master; unrelated to pointInPolygon)	a fix task is created (investigating at full effort — fixing PR link to follow here)
Stress test (amd_debug) / Hung check failed, possible deadlock found	chronic hung-check deadlock family (hung query was a plain SELECT count() FROM test WHERE ...; unrelated to this PR)	#101680 / #105905 (ours, open)
CH Inc sync	—	CH Inc sync (private, not actionable by us)

Neither functional failure is in this PR's changed files (src/Functions/pointInPolygon.cpp, tests/queries/0_stateless/04411_*, tests/queries/0_stateless/00500_*). The pointInPolygon tests (00500_, 04411_) all pass on this head.

Session id: cron:our-pr-ci-monitor:20260626-023000

[groeneai]

Fixing PR for test_async_backup_restore_with_max_execution_time_zero created: #108567

[nihalzp]

Why did you replace the xxhash with siphash? It degraded performance of one of the existing performance tests:

point_in_polygon_const_hashing | 1.0651 | 1.7929 | +68.3% | arm | Likely | 0 | SELECT count() FROM numbers(1000000) WHERE NOT ignore(pointInPolygon( (number % 997 / 997. - 0.5, (number * 7 % 991) / 991. - 0.5), arrayMap(x -> (cos(x / 8000. * 2 * pi()), sin(x / 8000. * 2 * pi())), range(8000)))) SETTINGS max_block_size = 64, validate_polygons = 0;

Make sure there are no slowdown.

[groeneai]

@nihalzp good catch, fixed. The slowdown came from how I hashed the raw columns, not from dropping xxhash in principle.

Root cause: the previous key hashed the parsed geometry, so it ran one bulk XXH3_128bits_update over a contiguous std::vector<Point>. My first revision keyed on the raw columns via IColumn::updateHashWithValue, which walks the array element by element (~16000 tiny SipHash::update calls for the 8000-vertex polygon). With max_block_size = 64 that per-block walk dominates. Even a bulk-SipHash variant (updateHashFast) stayed ~+68% because SipHash throughput is well below XXH3 for this volume.

Fix: I kept the raw-column key (so the per-block parse behind #106596 is still removed) but hash it with XXH3 again, over the columns' contiguous buffers in bulk (updateHashWithColumn descends Const/Array/Tuple and feeds each leaf buffer to XXH3_128bits_update). So there is no element-wise hashing and no parse on the hot path.

Local debug build, your perf query (max_block_size = 64, validate_polygons = 0, single thread):

	master (#107247)	this PR
point_in_polygon_const_hashing	0.576s	0.337s (-41%)
#106596 huge multipolygon (1000 polys, 500k points)	0.918s	0.399s (-57%)

Both are faster than master now: master parses + XXH3 per block, this PR does XXH3-only (no parse). The validate_polygons + argument-type folding and the cache-miss exception-safety are unchanged; 04411_point_in_polygon_const_cache_key covers them (the type case now uses Int8/UInt8 polygons with identical raw bytes, 0xFF, which alias to the wrong entry without the type in the key).

[groeneai]

Pre-PR validation gate for the perf fix (commit 7892439, addressing the point_in_polygon_const_hashing regression):

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. Perf: the regressed point_in_polygon_const_hashing query (max_block_size=64, validate_polygons=0) ran at 1.33s vs master 0.576s, every run. Correctness: on a build with the type omitted from the key, the UInt8 polygon returns 1 instead of 0 (aliases the Int8 entry) every run.
b	Root cause explained?	Yes. The previous key hashed the parsed geometry with one bulk XXH3 call. Keying on raw columns via updateHashWithValue walks the array element by element (~16000 SipHash::update calls per block); with tiny blocks that walk dominates. Even bulk SipHash stays +68% because SipHash throughput is far below XXH3 for this data volume.
c	Fix matches root cause?	Yes. Keep the raw-column key (so the #106596 per-block parse stays removed) but hash it with XXH3 over the columns' contiguous buffers in bulk (updateHashWithColumn descends Const/Array/Tuple, feeds each leaf buffer to XXH3_128bits_update). No element-wise hashing, no parse on the hot path.
d	Test intent preserved / new tests added?	Yes. 04411_point_in_polygon_const_cache_key strengthened: the type-identity case now uses Int8/UInt8 polygons with identical raw bytes (0xFF) that cast to different coordinates, so it actually catches a missing type byte. validate-mode and no-cache-poisoning cases unchanged.
e	Both directions demonstrated?	Yes. Perf: 1.33s -> 0.337s (faster than master 0.576s); #106596 huge multipolygon 0.918s -> 0.399s. Correctness: type-omitted build returns wrong 1 for the UInt8 box; correct build returns 0 (both orders). All 17 point_in_polygon tests pass; 04411 passes 20/20.
f	Fix is general across code paths?	Yes. A single hashConstPolygonArguments serves both the polygon and multipolygon branches; the walker handles every column shape the validated polygon arguments can take, and the variadic multi-arg form via the argument loop.
g	Fix generalizes across inputs (params/datatypes/wrappers)?	Yes. The recursive walker handles Array/Tuple/Const and any fixed-and-contiguous numeric leaf, with an element-wise fallback for any other shape; type identity and validate_polygons are folded into the key so wrapper/type/mode variants get distinct entries. Verified with Int8/UInt8/Int32/Float64 coords, polygons with holes, and the variadic multipolygon form.
h	Backward compatible?	Yes. Internal, process-local cache-key hashing only. No setting, on-disk, wire, or replication format change; nothing user-visible. The key value changes but the cache is ephemeral.
i	Invariants and contracts preserved?	Yes. The key still uniquely identifies (discriminator, validate flag, per-argument type name, raw value bytes) with length/size prefixes so concatenations cannot collide. Parse and validation still run only on a cache miss; CacheBase::getOrSet inserts only on a successful load, so a failed validation does not poison the cache.

Session id: cron:clickhouse-worker-slot-0:20260626-095500

[groeneai]

Fixed the Style check failure on the previous push. The regression test 04411 used SYSTEM DROP POINT IN POLYGON CACHE to verify the type-identity gap in both cache-insertion orders, which requires the no-parallel tag (SYSTEM DROP mutates the process-wide cache). To keep the test parallel-safe instead, both insertion orders are now covered with two byte-disjoint type pairs (Int8 -1 / UInt8 255 = byte 0xFF, and Int8 -2 / UInt8 254 = byte 0xFE) whose cache keys never collide, so no eviction is needed. Verified on a type-omitted-key build the test still fails as expected (returns 1,1,0,0 vs the reference 1,0,0,1), and passes 30/30 with randomization on the fix.

[nihalzp]

Why not use 0 and 1 instead of -1 and -2?

[groeneai]

0 and 1 would not exercise the type-identity gap this case targets. The point is to use a raw byte that is shared by Int8 and UInt8 but casts to a different Float64 coordinate, so a key that omits the argument type aliases the two polygons to the same cache entry. That requires a negative Int8 (high bit set):

• -1 is byte 0xFF -> Int8 -1.0 vs UInt8 255.0. Box -1..3 contains (0, 0) (-> 1), box 3..255 does not (-> 0). Same bytes, different geometry, different result.
• 0/1 are bytes 0x00/0x01, identical in both types and casting to the same 0.0/1.0. The two polygons would be byte-for-byte and coordinate-for-coordinate identical, so the test would pass even with the type omitted from the key -> it could not catch the regression.

-1 and -2 are just the two smallest-magnitude negatives, giving the two byte-disjoint pairs (0xFF/0x03 and 0xFE/0x04) used to check both insertion orders without a SYSTEM DROP. I added a comment in the test spelling this out.

[clickhouse-gh]

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	85.30%	85.30%	+0.00%
Functions	92.60%	92.60%	+0.00%
Branches	77.50%	77.50%	+0.00%

Changed lines: Changed C/C++ lines covered by tests: 56/58 (96.55%) | Lost baseline coverage (was covered on master, now uncovered in this PR): 1 line(s) · Uncovered code

Full report · Diff report

[groeneai]

CI finish ledger — 2581f24

Every failure below has an owner; none is in this PR's diff (pointInPolygon.cpp, 00500_*, 04411_point_in_polygon_const_cache_key), so none is PR-caused.

Check / test	Reason	Owner / fixing PR
AST fuzzer (amd_debug) / Logical error: Bad cast from type A to B (STID 3267-3db3)	trunk LOGICAL_ERROR family, 112 PRs / 18 master in 30d	#108011 (Bad-cast-from-type-A-to-B fix, in review)
Stress test (arm_debug) / refresh scheduling LOGICAL_ERROR (STID 2508-34af; the Cannot-start-server + Check-failed rows are downstream of that crash)	chronic RefreshTask shutdown race, 183 PRs / 30 master in 30d	a fix task is tracking it (RefreshTask scheduling family, on review)
Mergeable Check / PR	aggregators reflecting the two reds above	n/a
CH Inc sync	private fork mirror	CH Inc sync (not actionable by us)

Session id: cron:our-pr-ci-monitor:20260626-173000