Skip to content

Use the primary key index for pointInPolygon with a Point key column - #112956

Merged
nihalzp merged 1 commit into
masterfrom
pointinpolygon-point-key
Aug 2, 2026
Merged

Use the primary key index for pointInPolygon with a Point key column#112956
nihalzp merged 1 commit into
masterfrom
pointinpolygon-point-key

Conversation

@alexey-milovidov

@alexey-milovidov alexey-milovidov commented Aug 1, 2026

Copy link
Copy Markdown
Member

Closes: #54805

KeyCondition could use the index for pointInPolygon((x, y), [...]) where x and y are separate key columns, but not for pointInPolygon(coord, [...]) where coord is a whole key column of type Point — such queries performed a full scan even when the table is ordered by the point column.

Now the first argument may also be a single key column (or key expression) of type Point, or another Tuple of two numeric elements. Tuple values are ordered lexicographically, so the range of such a key column in a granule constrains the first coordinate of the point, and also the second coordinate when the first one is fixed; the resulting bounding box is intersected with the polygon as before. Both the regular and the lightweight (use_lightweight_primary_key_index_analysis) analysis paths are covered.

CREATE TABLE points (coord Point) ENGINE = MergeTree ORDER BY coord;
INSERT INTO points SELECT (number, number) FROM numbers(100000);
SELECT count() FROM points WHERE pointInPolygon(coord, [(0, 0), (0, 25000), (25000, 25000), (25000, 0)]);
-- Before: 100000 rows read (full scan). After: ~26000 rows read (26/100 granules).

The IN part of the linked issue (coord IN ((x, y), ...)) already works on master.

Changelog category (leave one):

  • Performance Improvement

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

Use the primary key index for pointInPolygon when the point argument is a whole key column of type Point (or another Tuple of two numeric elements), e.g. pointInPolygon(coord, [...]) for a table ordered by coord. Previously only the pointInPolygon((x, y), [...]) form with two scalar key columns was analyzed. Closes #54805.

Documentation entry for user-facing changes

🤖 Generated with Claude Code

Version info

  • Merged into: 26.8.1.627 (included in 26.8 and later)

Support index analysis for `pointInPolygon(coord, [...])` where `coord` is a
whole key column of type `Point` (or another `Tuple` of two numeric elements),
in addition to the previously supported `pointInPolygon((x, y), [...])` form
with two scalar key columns. Tuple values are ordered lexicographically, so the
range of such a key column constrains the first coordinate (and the second one
when the first is fixed); the resulting bounding box is intersected with the
polygon as before.

Closes: #54805

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@clickhouse-gh

clickhouse-gh Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [d1daacb]

Summary:


AI Review

Summary

This PR extends pointInPolygon primary-key pruning to a whole Point/tuple key column, and the implementation itself looks internally consistent. CI is green, but I do not think the added evidence is strong enough yet: the new stateless test does not deterministically cover both primary-key analysis paths or the advertised whole-key-expression variant.

Findings

⚠️ Majors

  • [tests/queries/0_stateless/04669_point_in_polygon_point_column_primary_key_index.sql:1] The PR changes both checkInHyperrectangle overloads and claims support for a whole key expression, but the new test only exercises whichever use_lightweight_primary_key_index_analysis value the randomized harness happened to pick, and every scenario uses a physical coord column rather than ORDER BY tuple(x, y).
    Suggested fix: add no-random-settings, run representative queries with use_lightweight_primary_key_index_analysis = 0 and 1, and add one focused ORDER BY tuple(x, y) case to prove the new fallback for whole key expressions.
Final Verdict

Needs one more focused test pass before merge: the code looks plausible, but the current evidence does not prove the non-lightweight path or the whole-key-expression form.

@clickhouse-gh clickhouse-gh Bot added the pr-performance Pull request with some performance improvements label Aug 1, 2026
@clickhouse-gh

clickhouse-gh Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

📊 Cloud Performance Report

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

no significant changes detected. K_source=6 K_base=30 flagged=0/65

clickbench

🟢 No significant changes

tpch_adapted_1_official

🟢 No significant changes

Debug info
  • StressHouse run: 2a2a9417-389c-4a8d-8589-a95271c6876d
  • MIRAI run: 22cd22e4-9e7b-41b0-9cbf-de75f7088cba
  • PR check IDs:
    • clickbench_1307087_1785631493
    • clickbench_1307098_1785631493
    • clickbench_1307111_1785631493
    • tpch_adapted_1_official_1307118_1785631493
    • tpch_adapted_1_official_1307144_1785631493
    • tpch_adapted_1_official_1307158_1785631494

@clickhouse-gh

clickhouse-gh Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 86.50% 86.50% +0.00%
Functions 91.90% 91.90% +0.00%
Branches 78.70% 78.70% +0.00%

Changed lines: Changed C/C++ lines covered: 79/92 (85.87%) · Uncovered code

Full report · Diff report

@nihalzp
nihalzp added this pull request to the merge queue Aug 2, 2026
@nihalzp nihalzp self-assigned this Aug 2, 2026
Merged via the queue into master with commit 90042b2 Aug 2, 2026
180 checks passed
@nihalzp
nihalzp deleted the pointinpolygon-point-key branch August 2, 2026 07:00
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Aug 2, 2026
@clickgapai

Copy link
Copy Markdown
Contributor

Hi @alexey-milovidov @nihalzp — while reviewing this PR I found the following:

Could you review the test PR and add the can be tested label if it looks good?
Happy to discuss — close anything that's wrong or already addressed.

pull Bot pushed a commit to Mu-L/ClickHouse that referenced this pull request Aug 3, 2026
alexey-milovidov added a commit to groeneai/ClickHouse that referenced this pull request Aug 3, 2026
… `Point` key

Follow-up to ClickHouse#112956. That change touched both `KeyCondition::checkInHyperrectangle`
overloads - the dense one and the lightweight (sparse) one - but the test did not pin
`use_lightweight_primary_key_index_analysis`, which `clickhouse-test` randomizes, so a
single CI run only exercised whichever overload the harness happened to pick.

It also did not cover the new fallback for a whole key *expression* of `Tuple` type: note
that `ORDER BY tuple(x, y)` is expanded by `extractKeyExpressionList` into the two key
columns `x` and `y` and therefore takes the other code path, so the tuple has to be one
element of a composite key, as in `ORDER BY (tuple(x, y), id)`.

Related: ClickHouse#112956
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-performance Pull request with some performance improvements pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pointInPolygon and IN operator causing full table scans on Point indexes

4 participants