Skip to content

Conversation

@raunaqmorarka
Copy link
Member

@raunaqmorarka raunaqmorarka commented Jun 17, 2025

Description

Previous code was using underlying value position of one channel on another channel's block.
This can give incorrect results when one of channels uses an RLE or Dictionary block.

Additional context and related issues

Seems to be broken since de3c160

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

## General
* Fix incorrect results with spatial joins. ({issue}`26021`)

Previous code was using underlying value position of one channel
on another channel's block. This can give incorrect results when
one of channels uses an RLE or Dictionary block.
@cla-bot cla-bot bot added the cla-signed label Jun 17, 2025
@raunaqmorarka raunaqmorarka requested review from Copilot and dain June 17, 2025 13:19
Copy link

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 incorrect block position lookups during spatial joins, addressing potential errors with channels using RLE or Dictionary blocks.

  • Update block position lookup in the spatial index supplier to correctly derive the underlying value position.
  • Adjust probe partition lookup to use the correct row index.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
core/trino-main/src/main/java/io/trino/operator/PagesSpatialIndexSupplier.java Corrects block position retrieval logic for geometry channels.
core/trino-main/src/main/java/io/trino/operator/PagesRTreeIndex.java Uses the proper index for probe partition lookup.
Comments suppressed due to low confidence (2)

core/trino-main/src/main/java/io/trino/operator/PagesSpatialIndexSupplier.java:112

  • [nitpick] The variable name 'chennelBlock' appears to be a typo. Consider renaming it to 'channelBlock' for clarity.
            Block chennelBlock = channels.get(geometryChannel).get(blockIndex);

core/trino-main/src/main/java/io/trino/operator/PagesRTreeIndex.java:156

  • The change from 'probePosition' to 'position' should be verified to confirm it correctly indexes into the partition data according to the intended row mapping.
        int probePartition = probePartitionChannel.map(channel -> INTEGER.getInt(probe.getBlock(channel), position)).orElse(-1);

@raunaqmorarka raunaqmorarka merged commit 5248e2a into master Jun 17, 2025
101 of 103 checks passed
@raunaqmorarka raunaqmorarka deleted the raunaq/geo-fix branch June 17, 2025 15:31
@github-actions github-actions bot added this to the 477 milestone Jun 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants