Skip to content

Fix distinguishing the joined geospatial types for ST_KNN#87

Merged
Kontinuation merged 2 commits intoapache:mainfrom
Kontinuation:fix-skip-geog-for-knn
Sep 16, 2025
Merged

Fix distinguishing the joined geospatial types for ST_KNN#87
Kontinuation merged 2 commits intoapache:mainfrom
Kontinuation:fix-skip-geog-for-knn

Conversation

@Kontinuation
Copy link
Member

@Kontinuation Kontinuation commented Sep 16, 2025

This PR fixes a regression introduced by #57, which added geography type checking to prevent optimized spatial joins from being used with unsupported geography types.

The problematic patch incorrectly matches left and right expressions in KNNPredicate to query plans. Actually, how left and right maps to the original query plans depends on the value of probe_side field. This is quite misleading and may be addressed in a future PR by renaming left and right to build and probe.

The function for checking if geospatial types are supported now returns Result<bool> to propagate errors during the field type extraction and matching process. This prevents silent matching failures due to bugs from happening.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a regression in geospatial type checking for ST_KNN operations by correctly mapping left/right expressions to query plans based on the probe_side field. The previous implementation incorrectly assumed a direct mapping between left/right expressions and schema positions.

  • Changes error handling from boolean returns to Result types for better error propagation
  • Adds proper probe_side consideration when validating KNN predicates to ensure correct schema-to-expression mapping
  • Includes comprehensive test coverage for the fixed KNN predicate validation logic

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@Kontinuation Kontinuation merged commit 3ed337e into apache:main Sep 16, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants