Initial support for geo types (+ minor README improvements) #33
Conversation
This file contains 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
Protryon
approved these changes
Jan 4, 2024
| to_from_sql!(Polygon); | ||
| to_from_sql!(MultiPolygon); | ||
| #[cfg(feature = "geo-types")] | ||
| mod nav_types_conversions { |
There was a problem hiding this comment.
Could you add in FromSql/ToSql implementations under the feature flag as well? Then derived types could directly use the geo_types types with no conversion.
This is to later allow efficiently (de)serializing geo values without having to deconstruct Values.
Clickhouse natively supported geo data types: https://clickhouse.com/docs/en/sql-reference/data-types/geo - `Point`, which is an alias for `Tuple(Float64,Float64)` - `Ring`, an alias for `Array(Point)` - `Polygon`, an alias for `Array(Ring)` - `MultiPolygon`, an alias for `Array(Polygon)` To support them in `klickhouse`, it is not sufficient to create a wrapper type and implement `ToSql`/`FromSql`. Indeed, these really are native types (a `Point` is returned as a `Point`, not as a `Tuple`), and this line would error: https://github.com/Protryon/klickhouse/blob/master/klickhouse/src/block.rs#L194 One simple solution would be to add 4 lines to `Type::from_str` ```rust "Point" => Type::Tuple(vec![Type::Float64,Type::Float64]), "Ring" => Type::Array(Box::new(Type::Tuple(vec![Type::Float64,Type::Float64]))), // etc. ``` and provide `Point([f64; 2])`, `Ring(Vec<Polygon>)` newtypes, along with `ToSql`/`FromSql` implementations. It is slightly unsatisfactory for two reasons: 1. The primitive types do not get exposed as such, but rather through their aliases. 2. The `{To,From}Sql` implementation on these newtypes require deconstructing and reconstruction (`into_iter`/`collect`) the inner collections. This instead implements the geo types as native `klickhouse` types and values. The `Value` variants provide strong typing such as `Value::Polygon(Polygon)`, where `Ring(Vec<Point>)`, rather than a `Value::Ring(Vec<Value>)`. The downside is that we still need to transform the inner container to `Value`s when serializing rather than just swapping an `Value::Array` for a `Value::Ring` To avoid code duplication with the `Array` and `Tuple` serializers/deserializers (or having the same inefficiency as 2. above), these are made generic. This allows e.g. directly constructing a `Ring(Vec<Point>)` without having to pass through a `Array(Vec<Value>)`. The implementation is tested with: - New roundtrip tests - The `test_serialize` implementation test.
The integration test is adapted so that these functions are tested too.
Protryon
approved these changes
Jan 4, 2024
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.
This adds support for native clickhouse geo data types: https://clickhouse.com/docs/en/sql-reference/data-types/geo
PointRingPolygonMultiPolygonThe approach taken is documented in 1eb61fd
The changes are tested with unit and integration tests.
In addition to these main changes, the README is updated to document all feature flags. It is now included in
lib.rs, so that it will also display in the docs.rs documentation.Alas it seems like Github does not support stacked PRs without the first PR being a branch of the original repo.
This is based on #32, only the 5 commits from today are new compared to the first PR. A proper diff can be viewed here: https://github.com/Protryon/klickhouse/pull/33/files/30cafa6c49dbf91f6c2463aaeeafc28b09299855..6372019814c1479e791a63e827834c931fa4b0ad