fix: handle conversion of native geometry columns#116
Conversation
| * @param {Array} opts.schema - Schema columns ({name, type, ...}). | ||
| * @param {Array<string>} opts.geoColumns - Names of geometry columns. | ||
| * @param {string|null} opts.primaryGeoColumn - Primary geometry column name. | ||
| * @param {string|null} opts.encoding - Geometry encoding ('wkb' or 'wkt'). |
There was a problem hiding this comment.
Can't it also be the arrow types?
There was a problem hiding this comment.
Yes it can - will update it to include the native geoArrow geometry type.
| const ringToWkt = (inner) => | ||
| `ring -> '(' || array_to_string(list_transform(ring, ${inner}), ', ') || ')'`; | ||
| const wrapGeom = (wktExpr) => | ||
| `CASE WHEN ${col} IS NOT NULL AND len(${col}) > 0 THEN ST_GeomFromText(${wktExpr}) ELSE NULL END`; | ||
| const arr = (expr, lambda) => `array_to_string(list_transform(${expr}, ${lambda}), ', ')`; |
There was a problem hiding this comment.
It's an abstraction helper functions to avoid having to repeated the same code multiple times on the different cases of the native geoArrow.
ringToWkt: Helper to transform list of points or lines into lambda strings that can be easily converted to duckDB's Geometry type.
wrapGeom: Checks for nulls before converting the constructed wkt to the actual duckDb's geometry type that can be transform.
arr : Helper function to map each element of a list to a lambda that's join in a comma separated list.
There was a problem hiding this comment.
Okay. Any reason these are redefined on each function call instead of defining them once?
There was a problem hiding this comment.
If you are referring to having them defined on both the db.js and converter.worker.js, frankly was planning to have a single buildGeomExpr function that can be invoked on either files to avoid multiple definition, but didn't follow through as was experience some errors after refactor and I decided to not waste more time on such.
There was a problem hiding this comment.
No, I meant why you use inline functions.
|
Looks generally okay, but I left some comments. Generally, we may benefit from documenting our design decisions with regards to the different data types and conversions so that we can reflect on them later. |
|
Regarding documentation of the query geometry expression of the different data types for future reference - we already have an open issue explain the decision and to find an alternative in the future. Unless if it needs to be on the code base as a comment or added on the readme. |
|
Extending/Updating the issue while we develop is okay. Is the issue up-to-date? |
Yes it entails our current implementation and the reason - it's on issue #111 you can take a look to double check. |
| * @param {Array<string>} opts.geoColumns - Names of geometry columns. | ||
| * @param {string|null} opts.primaryGeoColumn - Primary geometry column name. | ||
| * @param {string|null} opts.encoding - Geometry encoding ('wkb' or 'wkt'). | ||
| * @param {string|null} opts.encoding - Geometry encoding ('wkb', 'wkt', or 'native geoArrow'). |
There was a problem hiding this comment.
So was pass in "native geoArrow"? I thought it would be "point", "polygon", etc...?
There was a problem hiding this comment.
Yes, isn't the list (points, linestrings, multipoints, etc )considered native geoArrow.
There was a problem hiding this comment.
I can update it. I just thought native geoArrow reflects the list that's not wkt or wkb.
There was a problem hiding this comment.
It should reflect the API as is.
2 participants
Uh oh!
There was an error while loading. Please reload this page.