Skip to content

Geospatial datatype support#988

Merged
sreekanth-db merged 18 commits into
databricks:mainfrom
sreekanth-db:geospatial-datatype
Oct 24, 2025
Merged

Geospatial datatype support#988
sreekanth-db merged 18 commits into
databricks:mainfrom
sreekanth-db:geospatial-datatype

Conversation

@sreekanth-db
Copy link
Copy Markdown
Collaborator

@sreekanth-db sreekanth-db commented Sep 9, 2025 •
edited
Loading

Description

Added support for geospatial datatype

Testing

Tested in all paths - combinations of thrift/sea, arrow/non-arrow, complex/non-complex datatype - ensuring similar behaviour with other datatypes.

Additional Notes to the Reviewer

@sreekanth-db
Geospatial datatype support
ab7556e 
Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
@sreekanth-db
changelog
6d2cd46 
Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
@sreekanth-db
additional unit tests
c1b8ce2 
Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
@sreekanth-db
Merge remote-tracking branch 'origin/main' into geospatial-datatype
6d02081
jayantsing-db
jayantsing-db reviewed Oct 7, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@jayantsing-db jayantsing-db left a comment

Choose a reason for hiding this comment

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

Added some comments. Can we have a fake service test for this?

Comment thread NEXT_CHANGELOG.md Outdated
Comment thread src/main/java/com/databricks/jdbc/api/impl/DatabricksGeospatial.java Outdated
Comment thread src/main/java/com/databricks/jdbc/api/impl/DatabricksGeography.java
Comment thread src/main/java/com/databricks/jdbc/api/impl/DatabricksGeometry.java Outdated
Comment thread src/main/java/com/databricks/jdbc/api/impl/IDatabricksGeospatial.java Outdated
Comment thread src/main/java/com/databricks/jdbc/api/impl/converters/ObjectConverter.java
Comment on lines +156 to +158
if (object instanceof DatabricksGeography) {
return (DatabricksGeography) object;
}
Copy link
Copy Markdown
Collaborator

@jayantsing-db jayantsing-db Oct 7, 2025

Choose a reason for hiding this comment

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

This is not a good conversion right (because we seem to have a specific converter)? In that case, please delete this and let the default impl throw exception.

Exceptions from this method in a happy path will then indicate that the correct converter bean is not injected instead of us doing a faulty conversion.

Copy link
Copy Markdown
Collaborator Author

@sreekanth-db sreekanth-db Oct 9, 2025

Choose a reason for hiding this comment

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

#988 (comment)

Comment thread pom.xml
Comment thread src/main/java/com/databricks/jdbc/api/impl/converters/WKTConverter.java Outdated
Comment thread src/main/java/com/databricks/jdbc/common/util/DatabricksTypeUtil.java Outdated
Comment thread src/main/java/com/databricks/jdbc/common/util/DatabricksTypeUtil.java Outdated
@sreekanth-db
addressing review comments
e93c3f8 
Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
@sreekanth-db
addressing review comments - 2
e911082 
Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
@sreekanth-db
thread safe wkt converter
2a7e20a 
Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
@sreekanth-db
Merge remote-tracking branch 'upstream/main' into geospatial-datatype
5178087
@sreekanth-db
geospatial fake service test
4aad99d 
Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
@sreekanth-db
addressing review comments - 3
b01f240 
Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
@sreekanth-db
addressing review comments - 4
61bfff4 
Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
gopalldb
gopalldb reviewed Oct 14, 2025
View reviewed changes
Comment thread pom.xml
gopalldb
gopalldb reviewed Oct 14, 2025
View reviewed changes
Comment thread src/main/java/com/databricks/jdbc/api/impl/AbstractDatabricksGeospatial.java Outdated
gopalldb
gopalldb reviewed Oct 14, 2025
View reviewed changes
Comment thread src/main/java/com/databricks/jdbc/api/impl/AbstractDatabricksGeospatial.java
gopalldb
gopalldb reviewed Oct 14, 2025
View reviewed changes
Comment thread src/main/java/com/databricks/jdbc/api/impl/AbstractDatabricksGeospatial.java Outdated
gopalldb
gopalldb reviewed Oct 14, 2025
View reviewed changes
Comment thread src/main/java/com/databricks/jdbc/api/impl/AbstractDatabricksGeospatial.java
gopalldb
gopalldb reviewed Oct 14, 2025
View reviewed changes
Comment thread src/main/java/com/databricks/jdbc/api/impl/AbstractDatabricksGeospatial.java
gopalldb
gopalldb reviewed Oct 14, 2025
View reviewed changes
Comment thread src/main/java/com/databricks/jdbc/api/impl/AbstractDatabricksGeospatial.java
gopalldb
gopalldb reviewed Oct 14, 2025
View reviewed changes
Comment thread src/main/java/com/databricks/jdbc/api/impl/DatabricksResultSetMetaData.java
@sreekanth-db
addressing review comments - 5
f080ff6 
Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
@sreekanth-db sreekanth-db requested a review from gopalldb October 15, 2025 08:55
gopalldb
gopalldb reviewed Oct 17, 2025
View reviewed changes
Comment thread src/main/java/com/databricks/jdbc/common/util/DatabricksTypeUtil.java Outdated
@sreekanth-db
improving test coverage
585f9fc 
Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
@sreekanth-db sreekanth-db requested a review from gopalldb October 20, 2025 09:25
gopalldb
gopalldb reviewed Oct 23, 2025
View reviewed changes
Comment thread src/main/java/com/databricks/jdbc/api/IDatabricksGeospatial.java
@sreekanth-db
moving interface out of impl
ea2f8fa 
Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
@sreekanth-db
Merge branch 'main' into geospatial-datatype
c90bc62
@sreekanth-db sreekanth-db enabled auto-merge (squash) October 24, 2025 07:10
@sreekanth-db
skipping thrift server fake service test (doesn't support geospatial …
50f27cc 
…on the current version)

Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
@sreekanth-db sreekanth-db merged commit 13046cb into databricks:main Oct 24, 2025
13 checks passed
milastdbx
milastdbx reviewed Oct 24, 2025
View reviewed changes
Comment thread src/main/java/com/databricks/jdbc/api/impl/AbstractDatabricksGeospatial.java
*/
@Override
public byte[] getWKB() throws DatabricksValidationException {
return WKTConverter.toWKB(wkt);
Copy link
Copy Markdown

@milastdbx milastdbx Oct 24, 2025

Choose a reason for hiding this comment

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

we should consider storing this value as computation can be very expensive

Copy link
Copy Markdown
Collaborator Author

@sreekanth-db sreekanth-db Nov 4, 2025

Choose a reason for hiding this comment

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

Storing wkb it in a class variable for reuse. Implemented in #1062

Comment thread src/main/java/com/databricks/jdbc/api/impl/DatabricksResultSetMetaData.java
private boolean isGeometryColumn(List<String> arrowMetadata, int index) {
return arrowMetadata != null
&& arrowMetadata.size() > index
&& arrowMetadata.get(index) != null
Copy link
Copy Markdown

@milastdbx milastdbx Oct 24, 2025

Choose a reason for hiding this comment

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

where is this coming from?
what kind of metadata is this ?

Copy link
Copy Markdown
Collaborator Author

@sreekanth-db sreekanth-db Nov 4, 2025

Choose a reason for hiding this comment

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

removed the metadata usage completely (#1062)

Comment thread src/main/java/com/databricks/jdbc/api/impl/converters/ArrowToJavaObjectConverter.java
int dataSrid = WKTConverter.extractSRIDFromEWKT(ewkt);
String cleanWkt = WKTConverter.removeSRIDFromEWKT(ewkt);

// Extract SRID from metadata if not present in data
Copy link
Copy Markdown

@milastdbx milastdbx Oct 24, 2025

Choose a reason for hiding this comment

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

ok this is a bit confusing.

the only time srid won't be present in EWKT is when SRID=0.
All other times SRID will be present.

Also SRID in metadata will be set to -1 when SRIDs per row differ, and per row will have SRID unless its 0 for that row

Copy link
Copy Markdown
Collaborator Author

@sreekanth-db sreekanth-db Nov 4, 2025

Choose a reason for hiding this comment

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

Updated the implementation for srid extraction: #1062

sshpuntoff pushed a commit to Qualytics/databricks-jdbc that referenced this pull request Oct 24, 2025
@sreekanth-db @sshpuntoff
Geospatial datatype support (databricks#988)
a87cb86 
## Description
<!-- Provide a brief summary of the changes made and the issue they aim
to address.-->
Added support for geospatial datatype

## Testing
<!-- Describe how the changes have been tested-->
Tested in all paths - combinations of thrift/sea, arrow/non-arrow,
complex/non-complex datatype - ensuring similar behaviour with other
datatypes.

## Additional Notes to the Reviewer
<!-- Share any additional context or insights that may help the reviewer
understand the changes better. This could include challenges faced,
limitations, or compromises made during the development process.
Also, mention any areas of the code that you would like the reviewer to
focus on specifically. -->

---------

Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Signed-off-by: sshpuntoff <samshpuntoff@gmail.com>
milastdbx
milastdbx reviewed Oct 27, 2025
View reviewed changes
Comment thread src/main/java/com/databricks/jdbc/api/impl/converters/GeospatialConverter.java
return convertToGeospatial(object, GEOGRAPHY, DatabricksGeography::new);
}

private <T extends IDatabricksGeospatial> T convertToGeospatial(
Copy link
Copy Markdown

@milastdbx milastdbx Oct 27, 2025

Choose a reason for hiding this comment

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

this converted does not behave the same as ArrowToJavaObjectConverter.
tbh this one is more precise

Copy link
Copy Markdown
Collaborator Author

@sreekanth-db sreekanth-db Nov 4, 2025

Choose a reason for hiding this comment

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

updated ArrowToJavaObjectConverter implementation in #1062

milastdbx
milastdbx reviewed Oct 27, 2025
View reviewed changes
Comment thread src/main/java/com/databricks/jdbc/api/impl/converters/ArrowToJavaObjectConverter.java
try {
// Look for pattern like "GEOMETRY(32633)" or "GEOGRAPHY(4326)"
Pattern pattern =
typePrefix.equals(GEOMETRY) ? GEOMETRY_SRID_PATTERN : GEOGRAPHY_SRID_PATTERN;
Copy link
Copy Markdown

@milastdbx milastdbx Oct 27, 2025

Choose a reason for hiding this comment

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

also geography pattern is incorrect i think, this will fail.
we also send GEOGRAPHY(, )

Copy link
Copy Markdown
Collaborator Author

@sreekanth-db sreekanth-db Nov 4, 2025

Choose a reason for hiding this comment

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

removed this method in #1062

@sreekanth-db sreekanth-db mentioned this pull request Nov 4, 2025
Merged
sreekanth-db added a commit that referenced this pull request Nov 9, 2025
@sreekanth-db
geospatial fixes (#1062)
922f590 
## Description
<!-- Provide a brief summary of the changes made and the issue they aim
to address.-->

This PR addresses comments from Milan's review on PR #988 (Geospatial
support implementation).

  ## Changes

  ### 1. Cache WKB Value for Performance
- Added `wkb` field to `AbstractDatabricksGeospatial` to cache WKB
representation
- WKB now computed once in constructor instead of on every `getWKB()`
call

  ### 2. Simplify SRID Extraction Logic
  - Removed metadata-based SRID extraction entirely
- Per Milan's clarification: "SRID is always present in EWKT unless it's
0"
- Simplified `convertToGeospatial()` to only extract SRID from EWKT data
  - Removed `extractSRIDFromMetadata()` method and regex patterns
- Removed unused `arrowMetadata` parameter from `convertToGeospatial()`

  ## Testing
  - All unit tests are passing

## Additional Notes to the Reviewer
<!-- Share any additional context or insights that may help the reviewer
understand the changes better. This could include challenges faced,
limitations, or compromises made during the development process.
Also, mention any areas of the code that you would like the reviewer to
focus on specifically. -->


NO_CHANGELOG=true
(changelog already added in previous PR)

Signed-off-by: Sreekanth Vadigi <sreekanth.vadigi@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gopalldb gopalldb gopalldb approved these changes

@jayantsing-db jayantsing-db Awaiting requested review from jayantsing-db

+1 more reviewer

@milastdbx milastdbx milastdbx left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sreekanth-db @gopalldb @milastdbx @jayantsing-db