-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support spatial extension types #168
Conversation
aa9515a
to
29b7d6e
Compare
Spatial extension installation fails on GitHub CI for some reason:
Temporary disabled spatial test, it's run is verified locally. |
Thanks for the PR! This currently hard-codes the spatial extension types - but I wonder if that's a good idea for a client. Other extensions can add their own types, including community extensions, which indicates that it's not a scalable solution to custom types. Given that nothing special is done for these types anyway - would it be possible to just ignore the alias and use the underlying type instead? (i.e.
Extensions are not distributed for all git hashes - but rather (1) for releases, and (2) (usually) for the hash that is used for the nightly tests, but then only after the nightly tests have finished running. |
Thanks for the review!
M, this should work I think. Though the way how the DB type is exposed to Java needs to be changed (for all types), because name matching is used now. I think we can expose the underlying type instead, and only match by it. It should be sufficient on Java side because only core types are mapped to Java classes anyway (and generic |
I believe Maia mentioned a solution to this in ODBC workflows (though it is not included in current ODBC) - will follow up on this in a separate PR. |
This PR changes the handling of extension types. Instead of treating all custom types (detected by having an alias) as `VARCHAR`, it ignores type alias and uses underlying logical type. This allows to work with the following types from the spatial extension using default JDBC types `java.sql.Struct`, `java.sql.Array`, and `java.sql.Blob`: - `POINT_2D`: as `STRUCT{x: DOUBLE, y: DOUBLE}` - `POINT_3D`: as `STRUCT{x: DOUBLE, y: DOUBLE, z: DOUBLE}` - `POINT_4D`: as `STRUCT{x: DOUBLE, y: DOUBLE, z: DOUBLE, m: DOUBLE}` - `LINESTRING_2D`: as `LIST[STRUCT{x: DOUBLE, y: DOUBLE}]` - `POLYGON_2D`: as `LIST[LIST[STRUCT{x: DOUBLE, y: DOUBLE}]]` - `BOX_2D`: as `STRUCT{min_x: DOUBLE, max_x: DOUBLE, min_y: DOUBLE, max_y: DOUBLE}` - `BOX_2DF`: as `STRUCT{min_x: FLOAT, max_x: FLOAT, min_y: FLOAT, max_y: FLOAT}` - `GEOMETRY`: as `BLOB` - `WKB_BLOB`: as `BLOB` Special handling is left only for `JSON` type because it is mapped to driver-specific `JsonNode` class. Testing: existing test for extnsion types is adjusted to use `java.sql.Struct` and `java.sql.Blob`. New test is added to cover all spatial types in bind parameters and in result set, minor enhancements are added to tests runner. Fixes: duckdb#37
29b7d6e
to
97bdc34
Compare
Just FYI, I've updated the PR ignoring aliases and using underlying types instead. Type handling looks cleaner now and there is actually no spatial specifics in this change - other extension that use composite types from DuckDB base types should all work the same way as spatial. Rebased the PR to latest main and updated the description above. |
Thanks! |
Just for the record, this PR was added to the main repo to trigger ODBC after the main tests passed and extensions are built. This way all versions ODBC will support installing extensions, including in tests in dev/CI. I intend to add the JDBC workflow triggering the same way, going to follow-up on this once workflow changes in #172 are ready. |
This PR changes the handling of extension types. Instead of treating
all custom types (detected by having an alias) as
VARCHAR
, it ignorestype alias and uses underlying logical type.
This allows to work with the following types from the spatial
extension using default JDBC types
java.sql.Struct
,java.sql.Array
,and
java.sql.Blob
:POINT_2D
: asSTRUCT{x: DOUBLE, y: DOUBLE}
POINT_3D
: asSTRUCT{x: DOUBLE, y: DOUBLE, z: DOUBLE}
POINT_4D
: asSTRUCT{x: DOUBLE, y: DOUBLE, z: DOUBLE, m: DOUBLE}
LINESTRING_2D
: asLIST[STRUCT{x: DOUBLE, y: DOUBLE}]
POLYGON_2D
: asLIST[LIST[STRUCT{x: DOUBLE, y: DOUBLE}]]
BOX_2D
: asSTRUCT{min_x: DOUBLE, max_x: DOUBLE, min_y: DOUBLE, max_y: DOUBLE}
BOX_2DF
: asSTRUCT{min_x: FLOAT, max_x: FLOAT, min_y: FLOAT, max_y: FLOAT}
GEOMETRY
: asBLOB
WKB_BLOB
: asBLOB
Special handling is left only for
JSON
type because it is mapped todriver-specific
JsonNode
class.Testing: existing test for extnsion types is adjusted to use
java.sql.Struct
andjava.sql.Blob
. New test is added to cover allspatial types in bind parameters and in result set, minor enhancements
are added to tests runner.
Fixes: #37
Edit: description updated