test: Add E2E tests for geo functions#26969
Open
jkhaliqi wants to merge 1 commit intoprestodb:masterfrom
Open
Conversation
jkhaliqi
requested review from
a team,
elharo,
feilong-liu,
jaystarshot and
vivek-bharathan
as code owners
prestodb-ci
requested review from
a team,
namya28 and
sh-shamsan
and removed request for
a team
Contributor
Reviewer's GuideAdds native E2E geospatial tests for Bing tile and great-circle distance functions by creating shared coordinate test data, temporary table helpers, and enabling BING_TILE handling in the test client. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- For the positive cases in
testBingTileAt,testBingTilesAround, andtestGreatCircleDistance, consider asserting against concrete expected values (e.g., specific tile IDs, polygon WKT, or distances) rather than justassertQuery/SELECT cardinality(...)so the tests verify correctness and not just successful execution. - In
testBingTilesAround, you can strengthen the tests by checking thatcardinality(bing_tiles_around(...))equals an expected constant (e.g.,SELECT 9), which will catch regressions in the number of tiles returned for a given zoom. - Now that
BING_TILEis handled inTestingPrestoClient.convertToRowValue, it may be worth adding at least one query in the tests that returns aBING_TILEdirectly in a result set (not just using it as an argument) to exercise that conversion path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- For the positive cases in `testBingTileAt`, `testBingTilesAround`, and `testGreatCircleDistance`, consider asserting against concrete expected values (e.g., specific tile IDs, polygon WKT, or distances) rather than just `assertQuery`/`SELECT cardinality(...)` so the tests verify correctness and not just successful execution.
- In `testBingTilesAround`, you can strengthen the tests by checking that `cardinality(bing_tiles_around(...))` equals an expected constant (e.g., `SELECT 9`), which will catch regressions in the number of tiles returned for a given zoom.
- Now that `BING_TILE` is handled in `TestingPrestoClient.convertToRowValue`, it may be worth adding at least one query in the tests that returns a `BING_TILE` directly in a result set (not just using it as an argument) to exercise that conversion path.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
aditi-pandit
left a comment
aditi-pandit
left a comment
There was a problem hiding this comment.
Thanks @jkhaliqi for the tests.
| @Test | ||
| public void testBingTileAt() | ||
| { | ||
| assertQuery("SELECT bing_tile_at(0.0, 0.0, 0)"); |
Contributor
There was a problem hiding this comment.
@jkhaliqi : A lot of these tests are using constants which are typically constant folded in the co-ordinator. Please write tests that involve using a column of data from a table as those run on workers.
jkhaliqi
force-pushed
the
e2e_geo
branch
2 times, most recently
from
b9b0f0a to
55ef8c1
Compare
jkhaliqi
force-pushed
the
e2e_geo
branch
2 times, most recently
from
9c9fe3d to
de07ad0
Compare
czentgr
reviewed
Feb 5, 2026
Contributor
czentgr
left a comment
czentgr
left a comment
There was a problem hiding this comment.
Thanks, for the description - release notes we don't need it.
Remember release notes are what goes into the changelog. That we added tests is great but not necessarily relevant like a fix/feature etc.
Should we look at the presto-native-tests as well and see what the support for testing the bing tile is?
I assume that these tests originally came from the Java E2E test. Maybe we can refer to it in the description.
Contributor
Thanks, good point. Just update the release note entry to |
jkhaliqi
force-pushed
the
e2e_geo
branch
3 times, most recently
from
4139ca8 to
f5029f7
Compare
| } | ||
| } | ||
|
|
||
| public static void createCoordinates(QueryRunner queryRunner) |
Contributor
There was a problem hiding this comment.
@jkhaliqi : Is this table used by any other tests ? Can we move this function to TestPrestoNativeGeospatial or to another specific file for GeoSpatial utility functions ?
Contributor
Author
There was a problem hiding this comment.
@aditi-pandit Thank you, it's only used and will be used for Geo testing, so created GeoSpatialTestUtility class.
czentgr
previously approved these changes
Mar 18, 2026
Contributor
czentgr
left a comment
czentgr
left a comment
There was a problem hiding this comment.
Thanks. @aditi-pandit Please also take another look.
Contributor
aditi-pandit
left a comment
aditi-pandit
left a comment
There was a problem hiding this comment.
Thanks @jkhaliqi for these tests.
| String tmpTableName = generateRandomTableName(getQueryRunner()); | ||
| getQueryRunner().execute(format("CREATE TABLE %s " + | ||
| "(x integer, y integer, z integer, isvalid boolean)", tmpTableName)); | ||
| getQueryRunner().execute(format("INSERT INTO %s VALUES " + |
Contributor
There was a problem hiding this comment.
Can we add some cases here where either of (or 2 of) x, y, z are NULL ?
Contributor
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
coordinatestable is created as a shared, non-temporary table increateTables()and reused across tests, which can lead to cross-test coupling and flakiness; consider using per-test temporary tables or a helper likeTestTablethat creates and drops isolated tables. generateRandomTableNamecreates persistent tables (e.g., intestBingTilePolygon) but the tests never drop them, so the test suite will leave behind state; it would be better to wrap such tables in a utility that ensuresDROP TABLEin atry/finallyor equivalent.- The
assertQueryFailspatterns are tightly coupled to full error messages like specific phrasing and capitalization (e.g.,Latitude.*outside of valid range), which can be brittle; consider relaxing the regexes to match only the most stable parts of the message or using error codes if available.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `coordinates` table is created as a shared, non-temporary table in `createTables()` and reused across tests, which can lead to cross-test coupling and flakiness; consider using per-test temporary tables or a helper like `TestTable` that creates and drops isolated tables.
- `generateRandomTableName` creates persistent tables (e.g., in `testBingTilePolygon`) but the tests never drop them, so the test suite will leave behind state; it would be better to wrap such tables in a utility that ensures `DROP TABLE` in a `try/finally` or equivalent.
- The `assertQueryFails` patterns are tightly coupled to full error messages like specific phrasing and capitalization (e.g., `Latitude.*outside of valid range`), which can be brittle; consider relaxing the regexes to match only the most stable parts of the message or using error codes if available.
## Individual Comments
### Comment 1
<location path="presto-native-tests/src/test/java/com/facebook/presto/nativetests/TestPrestoNativeGeospatial.java" line_range="73-82" />
<code_context>
+ @Test
</code_context>
<issue_to_address>
**suggestion (testing):** Drop the temporary table created in testBingTilePolygon to avoid leaving artifacts between test runs.
`testBingTilePolygon` creates a temporary table with a random name but never drops it, so it persists after the test and can clutter shared environments or repeated runs. Please ensure the table is dropped, e.g., by wrapping `CREATE`/`INSERT`/`SELECT` in a try/finally with a `DROP TABLE` in the finally block, or by using a helper that manages temporary test table lifecycle.
Suggested implementation:
```java
@Test
public void testBingTilePolygon()
{
String tableName = "tmp_bing_tile_polygon_" + randomNameSuffix();
try {
assertUpdate(format("CREATE TABLE %s AS SELECT ...", tableName), /* expectedRows */ ...);
assertQuery(format("SELECT ... FROM %s", tableName), "SELECT ...");
}
finally {
// Best-effort cleanup to avoid leaving artifacts between test runs
assertUpdate(format("DROP TABLE IF EXISTS %s", tableName));
}
}
```
I only saw a small portion of the file, so please adjust the SEARCH block to match the actual implementation of `testBingTilePolygon`:
1. Keep the existing `tableName` initialization expression exactly as it is (if it already uses a random name, retain that).
2. Move all existing `CREATE` / `INSERT` / `SELECT` calls inside the `try` block.
3. Ensure `assertUpdate(format("DROP TABLE IF EXISTS %s", tableName));` in the `finally` block matches your existing assertion utilities and imports (e.g., `format` from `java.lang.String` or `com.facebook.presto.util.MoreStrings` as currently used in the file).
4. If the test currently uses a different variable name than `tableName`, update the `DROP TABLE` statement accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...o-native-tests/src/test/java/com/facebook/presto/nativetests/TestPrestoNativeGeospatial.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Adds end-to-end coverage for Presto Native geospatial functions (Bing tile utilities + great circle distance) and extends test result handling to support BING_TILE values.
Changes:
- Extend
TestingPrestoClientrow-value conversion to handleBING_TILEresults in test assertions. - Add native E2E tests for
bing_tile_at,bing_tile_polygon,bing_tiles_around, andgreat_circle_distance(including failure cases). - Introduce
GeoSpatialTestUtilsto provision shared coordinate test data and generate temp table names.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| presto-tests/src/main/java/com/facebook/presto/tests/TestingPrestoClient.java | Adds BING_TILE handling in test value conversion to support new assertions. |
| presto-native-tests/src/test/java/com/facebook/presto/nativetests/TestPrestoNativeGeospatial.java | Adds E2E geospatial tests and table setup for shared coordinate data. |
| presto-native-tests/src/test/java/com/facebook/presto/nativetests/GeoSpatialTestUtils.java | New utility to create coordinate test table and generate random temp table names. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...o-native-tests/src/test/java/com/facebook/presto/nativetests/TestPrestoNativeGeospatial.java
Outdated
Show resolved
Hide resolved
presto-native-tests/src/test/java/com/facebook/presto/nativetests/GeoSpatialTestUtils.java
Outdated
Show resolved
Hide resolved
presto-native-tests/src/test/java/com/facebook/presto/nativetests/GeoSpatialTestUtils.java
Show resolved
Hide resolved
...o-native-tests/src/test/java/com/facebook/presto/nativetests/TestPrestoNativeGeospatial.java
Outdated
Show resolved
Hide resolved
Member
Should you add tests with the 4 parameter version that has radius?
|
Member
|
Should we have NULL array inputs (CAST(NULL AS ...))? Is that worth adding as a test? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add E2E tests for bing_tile_at, bing_tile_polygon, bing_tiles_around, and great_circle_distance.
Some testing data came from https://github.com/prestodb/presto/blob/master/presto-main-base/src/test/java/com/facebook/presto/geospatial/TestBingTileFunctions.java
Motivation and Context
closes #26935
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
Summary by Sourcery
Add native geospatial E2E coverage for Bing tile and great-circle distance functions using a shared coordinates table utility.
Enhancements:
Tests: