-
Notifications
You must be signed in to change notification settings - Fork 14k
[FLINK-39904][table] Add GEOGRAPHY logical type support #28740
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
base: master
Are you sure you want to change the base?
Changes from all commits
739d451
9681723
6642e98
94bae30
db4ce02
1740ad9
d071fc3
6aabe83
1206a28
179666b
3463d4a
27c808c
0281a8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -409,6 +409,7 @@ components: | |
| - VARIANT | ||
| - BITMAP | ||
| - UUID | ||
| - GEOGRAPHY | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please could you add documentation so it is obvious what the externals are .
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, added docs for the GEOGRAPHY type and its external representation. |
||
| OpenSessionRequestBody: | ||
| type: object | ||
| properties: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -526,6 +526,7 @@ components: | |
| - VARIANT | ||
| - BITMAP | ||
| - UUID | ||
| - GEOGRAPHY | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would end up advertising GEOGRAPHY to REST clients that isn't supported by the gateway yet. I know your PR description says that follow-up changes are coming, but do you think it's worth making updates to It probably just needs a new case in the switch statements in serializeInternal and deserializeInternal
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, agreed. Since the generated REST schema advertises We added The SQL constructors, accessors, and spatial functions are implemented separately in the follow-up PR #28788. |
||
| OpenSessionRequestBody: | ||
| type: object | ||
| properties: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.flink.sql.parser.type; | ||
|
|
||
| import org.apache.flink.annotation.Internal; | ||
| import org.apache.flink.table.calcite.ExtendedRelTypeFactory; | ||
|
|
||
| import org.apache.calcite.rel.type.RelDataType; | ||
| import org.apache.calcite.sql.SqlIdentifier; | ||
| import org.apache.calcite.sql.SqlTypeNameSpec; | ||
| import org.apache.calcite.sql.SqlWriter; | ||
| import org.apache.calcite.sql.parser.SqlParserPos; | ||
| import org.apache.calcite.sql.validate.SqlValidator; | ||
| import org.apache.calcite.util.Litmus; | ||
|
|
||
| /** Represents the GEOGRAPHY data type. */ | ||
| @Internal | ||
| public final class SqlGeographyTypeNameSpec extends SqlTypeNameSpec { | ||
|
|
||
| private static final String GEOGRAPHY_TYPE_NAME = "GEOGRAPHY"; | ||
|
|
||
| public SqlGeographyTypeNameSpec(SqlParserPos pos) { | ||
| super(new SqlIdentifier(GEOGRAPHY_TYPE_NAME, pos), pos); | ||
| } | ||
|
|
||
| @Override | ||
| public RelDataType deriveType(SqlValidator validator) { | ||
| return ((ExtendedRelTypeFactory) validator.getTypeFactory()).createGeographyType(); | ||
| } | ||
|
|
||
| @Override | ||
| public void unparse(SqlWriter writer, int leftPrec, int rightPrec) { | ||
| writer.keyword(GEOGRAPHY_TYPE_NAME); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equalsDeep(SqlTypeNameSpec spec, Litmus litmus) { | ||
| if (!(spec instanceof SqlGeographyTypeNameSpec)) { | ||
| return litmus.fail("{} != {}", this, spec); | ||
| } | ||
| return litmus.succeed(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,7 @@ | |
| import org.apache.flink.table.types.logical.DescriptorType; | ||
| import org.apache.flink.table.types.logical.DoubleType; | ||
| import org.apache.flink.table.types.logical.FloatType; | ||
| import org.apache.flink.table.types.logical.GeographyType; | ||
| import org.apache.flink.table.types.logical.IntType; | ||
| import org.apache.flink.table.types.logical.LocalZonedTimestampType; | ||
| import org.apache.flink.table.types.logical.LogicalType; | ||
|
|
@@ -1087,6 +1088,15 @@ public static DataType UUID() { | |
| return new AtomicDataType(new UuidType()); | ||
| } | ||
|
|
||
| /** | ||
| * Data type of geography data. | ||
| * | ||
| * @see GeographyType | ||
| */ | ||
| public static DataType GEOGRAPHY() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you think we need a new datatype converter for geography in https://github.com/akvelon/flink/blob/fccf3126db72d8fbbb3829f81c4f123d1f005df4/flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/data/util/DataFormatConverters.java#L115-L165 ?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we did need a dedicated converter for
We also added coverage for converter lookup, nullable and non-nullable types, WKB round-trip, row extraction, null handling, Thanks for pointing this out. |
||
| return new AtomicDataType(new GeographyType()); | ||
| } | ||
|
|
||
| // -------------------------------------------------------------------------------------------- | ||
| // Helper functions | ||
| // -------------------------------------------------------------------------------------------- | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think we should provide a references to the new acronyms EWKB, CRS, SRID, OGC:CRS84 and ISO WKB somewhere in the wording. So the user can find the meanings of these acronyms and how they should use them here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed. I expanded the GEOGRAPHY documentation in
docs/content/docs/sql/reference/data-types.md.It now explains CRS, SRID, ISO WKB, and EWKB, and includes references for OGC:CRS84, OGC Simple Feature Access, and EWKB. I also clarified that the v1 contract uses standard 2D ISO WKB, while EWKB is not supported and CRS/SRID metadata is not stored in the WKB payload.
Thanks for pointing this out.