Skip to content

[GH-3398] Keep extra predicates on broadcast and nested KNN joins - #3399

Merged
jiayuasu merged 5 commits into
apache:masterfrom
kalayciburak:fix/gh-3398-knn-extra-predicates
Sep 26, 2026
Merged

jiayuasu merged 5 commits into
apache:masterfrom
kalayciburak:fix/gh-3398-knn-extra-predicates

Conversation

@kalayciburak

@kalayciburak kalayciburak commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Did you read the Contributor Guide?

Is this PR related to a ticket?

What changes were proposed in this PR?

KNN joins could drop additional ON conditions when broadcasting either side or when conditions contained nested conjunctions, producing different results depending on the physical strategy.

Reuse the residual condition already extracted by OptimizableJoinCondition and pass it unchanged to the regular, query-side broadcast, and object-side broadcast KNN executors. Remove the duplicate KNN-specific extractor. The original condition remains available in the regular plan for inspection.

Reject ST_KNN markers left in the residual of any detected spatial join during planning, including markers nested inside residual expressions. A physical KNN join implements only its selected predicate, so multiple KNN predicates are unsupported. If another spatial predicate is selected first, the error directs users to place ST_KNN first, preserving the existing predicate-selection rule while preventing a later per-pair evaluation failure.

Regression coverage checks independently selective predicates, mixed accepted/rejected pairs, null values, predicate ordering and nesting, both SQL table orders, both broadcast directions, three- and four-argument KNN, and filtering after nearest-neighbor selection. It also covers valid KNN-first spatial residuals and planning-time rejection of multiple-KNN conditions or KNN after another spatial predicate.

Document that residual ON predicates filter the selected neighbors without replacing rejected pairs, distinguish this from optimizer filter pushdown, and explain the single-KNN and spatial predicate ordering requirements.

How was this patch tested?

The branch now includes the latest master. Its merged MinIO test-image fix resolves the prior image-pull failure; this PR does not change those reader tests.

Java 17, Maven offline with cached dependencies:

  • RED against the original PR: the nine new multiple-KNN planning-time rejection cases failed; the other 53 KNN tests passed.
  • RED before the mixed-predicate guard: all six new planning-time rejection cases failed. A separate runtime reproduction confirmed the previous failure reached execution with KNN predicate is not supported.
  • GREEN, Spark 3.5 / Scala 2.12: KnnJoinSuite, SpatialJoinSuite, BroadcastIndexJoinSuite, and SphereDistanceJoinSuite — 349 tests passed.
  • GREEN after merging current master, Spark 3.5 / Scala 2.12: KnnJoinSuite — 74 tests passed.
  • GREEN, Spark 4.0.2 / Scala 2.13: KnnJoinSuite — 74 tests passed.
  • GREEN, Spark 4.1.1 / Scala 2.13: KnnJoinSuite — 74 tests passed.
  • Spark 4 builds used separate source snapshots with matching KNN files. All three builds passed Spotless checks; git diff --check passed.

Commands, with JAVA_HOME set to Java 17 and SPARK_LOCAL_IP=127.0.0.1:

mvn -o -pl spark/common -am package \
  -DwildcardSuites=org.apache.sedona.sql.KnnJoinSuite,org.apache.sedona.sql.SpatialJoinSuite,org.apache.sedona.sql.BroadcastIndexJoinSuite,org.apache.sedona.sql.SphereDistanceJoinSuite \
  -Dtest=KnnJoinSuite -Dsurefire.failIfNoSpecifiedTests=false \
  -DfailIfNoTests=false -Dmaven.javadoc.skip=true

mvn -o -pl spark/common -am package -Dspark=4.0 -Dscala=2.13 -Dspark.version=4.0.2 \
  -DwildcardSuites=org.apache.sedona.sql.KnnJoinSuite \
  -Dtest=KnnJoinSuite -Dsurefire.failIfNoSpecifiedTests=false \
  -DfailIfNoTests=false -Dmaven.javadoc.skip=true

mvn -o -pl spark/common -am package -Dspark=4.1 -Dscala=2.13 \
  -DwildcardSuites=org.apache.sedona.sql.KnnJoinSuite \
  -Dtest=KnnJoinSuite -Dsurefire.failIfNoSpecifiedTests=false \
  -DfailIfNoTests=false -Dmaven.javadoc.skip=true

Did this PR include necessary documentation updates?

  • Yes. Updated the nearest-neighbor SQL documentation with residual filtering semantics, the filter-pushdown caveat, and supported join-condition requirements.

Broadcast KNN plans dropped every extra join predicate, and a nested
AND hid the non-KNN conjuncts from the one-level extractor. Flatten
the conjuncts and apply them on both broadcast sides.
@kalayciburak
kalayciburak force-pushed the fix/gh-3398-knn-extra-predicates branch from 437b34e to 44bb618 Compare September 24, 2026 16:44
…edicates

Use the residual condition already returned by OptimizableJoinCondition
for regular and both broadcast KNN plans. Remove duplicate extraction
that discarded every KNN marker, and reject additional markers before
execution instead of silently ignoring their semantics.

Replace all-false-only regression coverage with independently selective
conditions, nullable values, both relation orders and broadcast sides,
three- and four-argument KNN, and planning-time rejection cases.

Validation: 337 join tests pass on Spark 3.5; 62 KNN tests pass on Spark
4.1.1/Scala 2.13. The nine new rejection tests failed on the original PR.
Validate every detected spatial join residual so an ST_KNN marker left
after ST_Intersects or ST_DWithin fails during planning with an actionable
ordering message. Preserve the existing error for multiple KNN predicates
and the behavior of valid KNN-first joins.

Add regression coverage for both matcher paths and all join strategies,
including positive KNN-first spatial filters with reversed table order.
Document residual filtering, the optimizer pushdown caveat, and the
single-KNN and spatial predicate ordering requirements.
…aries

Quay no longer grants anonymous pull access to minio/minio, causing both
S3 reader tests to time out fetching their container image and cancel the
Spark build matrix.

Build the test image from the same pinned MinIO release on GitHub and
verify architecture-specific SHA-256 checksums. Share the image lazily
between the reader suites while retaining separate containers per test.

Write the Dockerfile build context against Spark's bundled tar API to
avoid changing Spark 3.5's older compression dependencies for these tests.
Keep the MinIO S3 test image from upstream apache#3408 and remove the branch-only image builder and Dockerfile. The remaining PR diff contains only the KNN join fix, its tests, and documentation.
@jiayuasu jiayuasu added this to the sedona-2.0.0 milestone Sep 26, 2026
@jiayuasu
jiayuasu merged commit 53e4e7b into apache:master Sep 26, 2026
40 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.

KNN joins silently drop additional ON conditions in broadcast and multi-conjunct plans

2 participants