[feature](iceberg) Add Iceberg V3 spatial type support - #67907
[feature](iceberg) Add Iceberg V3 spatial type support#67907OIiveirra wants to merge 41 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate and critical findings affect metadata serialization, spatial I/O, function binding, validation, and test compliance.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Iceberg V3 GEOMETRY and GEOGRAPHY support for external tables, including CRS/algorithm metadata, raw-WKB Parquet I/O, and spatial-function integration.
Changes:
- Adds FE/BE spatial types and Thrift/protobuf metadata.
- Adds Iceberg schema conversion, Parquet annotations, WKB handling, and write validation.
- Adds spatial functions, tests, and regression coverage.
File summaries
| File | Summary |
|---|---|
regression-test/suites/external_table_p0/iceberg/test_iceberg_spatial_v3.groovy |
Adds Iceberg V3 spatial regression coverage. |
gensrc/thrift/Types.thrift |
Adds spatial primitive metadata. |
gensrc/proto/types.proto |
Adds spatial protobuf metadata fields. |
fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/info/ColumnDefinitionTest.java |
Tests internal-table spatial rejection. |
fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/scalar/SpatialConstructorFunctionTest.java |
Tests spatial constructors. Critical: uses disallowed JUnit 4 imports; switch to JUnit 5. |
fe/fe-core/src/test/java/org/apache/doris/datasource/iceberg/IcebergUtilsTest.java |
Tests Iceberg spatial mapping and write rules. |
fe/fe-core/src/test/java/org/apache/doris/datasource/iceberg/IcebergSpatialWriteAnalyzerTest.java |
Tests spatial write validation. |
fe/fe-core/src/main/java/org/apache/doris/nereids/types/GeometryType.java |
Adds parameterized geometry type. |
fe/fe-core/src/main/java/org/apache/doris/nereids/types/GeographyType.java |
Adds parameterized geography type. |
fe/fe-core/src/main/java/org/apache/doris/nereids/types/DataType.java |
Integrates spatial Nereids types. |
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ColumnDefinition.java |
Rejects spatial internal-table columns. Moderate: nested spatial types can bypass this check. |
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/visitor/ScalarFunctionVisitor.java |
Adds spatial function visitors. |
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/StGeomFromWKB.java |
Adds geometry WKB construction. |
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/StGeometryFromWKB.java |
Adds geometry WKB construction alias. |
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/StGeogFromWKB.java |
Adds geography WKB construction. |
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/StAstext.java |
Adds spatial-aware text conversion. Moderate: other supported spatial functions still lack spatial-aware signatures. |
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java |
Integrates spatial INSERT validation. |
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java |
Integrates spatial MERGE validation. |
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java |
Maps Iceberg spatial schemas and validates writes. Moderate: nested spatial schemas pass analysis but fail in the Parquet writer. |
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergSpatialWriteAnalyzer.java |
Validates spatial write compatibility. Moderate: untyped NULL is rejected before sink coercion. |
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/DorisTypeToIcebergType.java |
Converts Doris spatial types to Iceberg types. Moderate: CREATE/ALTER paths lack the required V3/Parquet guard. |
fe/fe-core/src/main/java/org/apache/doris/catalog/BuiltinScalarFunctions.java |
Registers spatial functions. |
fe/fe-core/src/main/java/org/apache/doris/analysis/ColumnDef.java |
Rejects spatial legacy internal-table columns. Moderate: nested spatial types can bypass this check. |
fe/fe-common/src/test/java/org/apache/doris/catalog/SpatialTypeTest.java |
Tests catalog spatial types. |
fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java |
Handles spatial type metadata. |
fe/fe-common/src/main/java/org/apache/doris/catalog/ScalarType.java |
Preserves CRS and geography algorithms. |
fe/fe-common/src/main/java/org/apache/doris/catalog/PrimitiveType.java |
Registers spatial primitive types. |
be/test/format/transformer/vparquet_transformer_test.cpp |
Tests spatial Parquet writing. |
be/test/format/table/iceberg/arrow_schema_util_test.cpp |
Tests Iceberg Arrow schema mapping. |
be/test/exprs/function/function_geo_test.cpp |
Tests spatial functions. Moderate: both Geography test paths omit the required CRS and algorithm. |
be/test/core/data_type/storage_field_type_test.cpp |
Tests storage field type mappings. |
be/test/core/data_type/data_type_spatial_test.cpp |
Tests backend spatial type behavior. |
be/src/format/transformer/vparquet_transformer.cpp |
Writes spatial Parquet annotations. |
be/src/format/table/iceberg/types.h |
Defines Iceberg spatial types. |
be/src/format/table/iceberg/types.cpp |
Implements Iceberg spatial type handling. |
be/src/format/table/iceberg/arrow_schema_util.h |
Declares Arrow schema utilities. |
be/src/format/table/iceberg/arrow_schema_util.cpp |
Adds Iceberg spatial Arrow annotations. Moderate: the read path lacks matching spatial schema conversion. |
be/src/exprs/function/geo/wkb_parse.h |
Declares WKB parsing helpers. |
be/src/exprs/function/geo/wkb_parse.cpp |
Implements raw-WKB parsing. |
be/src/exprs/function/geo/geo_types.h |
Defines geometry types. |
be/src/exprs/function/geo/geo_types.cpp |
Implements geometry types. |
be/src/exprs/function/geo/geo_common.cpp |
Provides shared spatial functionality. |
be/src/exprs/function/geo/functions_geo.h |
Declares spatial functions. |
be/src/exprs/function/geo/functions_geo.cpp |
Adds WKB parsing and spatial validation. Moderate: Geography construction can omit its required default algorithm; 0x-prefixed hex is mishandled; spatial function signatures are not reachable from many Nereids calls. |
be/src/core/data_type/storage_field_type.cpp |
Adds spatial storage mappings. |
be/src/core/data_type/primitive_type.cpp |
Registers backend primitive mappings. |
be/src/core/data_type/define_primitive_type.h |
Defines primitive type registration. |
be/src/core/data_type/data_type_spatial.h |
Declares backend spatial types. |
be/src/core/data_type/data_type_spatial.cpp |
Implements spatial types and serialization. Moderate: protobuf serialization omits CRS and Geography algorithm metadata. |
be/src/core/data_type/data_type_factory.cpp |
Constructs backend data types. Moderate: protobuf deserialization ignores serialized spatial metadata. |
be/src/core/data_type_serde/data_type_varbinary_serde.cpp |
Supports spatial-related varbinary serialization. |
be/src/core/column/column_spatial.h |
Declares raw-WKB spatial columns. |
be/src/core/column/column_spatial.cpp |
Implements raw-WKB spatial columns. |
Review details
Suppressed comments (9)
be/src/core/data_type/data_type_factory.cpp:503
- Even if the protobuf fields are populated, the protobuf
DataTypeFactorypath falls through tocreate_data_type(primitive_type, ...), which reaches these default constructors and ignoresPScalarType.spatial_*. As a result, non-default CRS/algorithm values cannot survive protobuf deserialization. Add a spatial branch in the protobuf overload that constructsDataTypeSpatialfrom the serialized fields, using defaults only for genuinely missing legacy fields.
case TYPE_GEOMETRY:
nested = std::make_shared<DataTypeSpatial>(TYPE_GEOMETRY);
break;
case TYPE_GEOGRAPHY:
nested = std::make_shared<DataTypeSpatial>(TYPE_GEOGRAPHY, "OGC:CRS84", "spherical");
be/src/exprs/function/geo/functions_geo.cpp:790
- The decoder below explicitly accepts
0x-prefixed hex, but the precedingGeoShape::from_wkbcall parses the original text and only strips the\\xform. ConsequentlyST_GEOMFROMWKB('0x...')andST_GEOGFROMWKB('0x...')are marked NULL before this decoder is reached. Decode the hex first and validate the resulting bytes withfrom_wkb_bytes.
auto value = geo->get_data_at(row);
std::unique_ptr<GeoShape> shape = GeoShape::from_wkb(value.data, value.size, status);
if (shape == nullptr || status != GEO_PARSE_OK) {
null_map_data[row] = 1;
res->insert_default();
continue;
}
if (!decode_wkb_hex(value, &wkb)) {
be/src/exprs/function/geo/functions_geo.cpp:80
- The new raw-WKB decoder is not reachable for most Nereids calls with the new spatial types.
StLength,StAsBinary,StGeometryType,StX/StY, and the measurement/relationship functions still advertise only Varchar/String signatures and, unlikeStAstext, have no spatial-awaresearchSignature; aGEOGRAPHY(...)argument therefore fails FE signature resolution before this helper andvalidate_geography_semanticscan run. Add spatial overloads or equivalent FE signature handling for the functions covered by this backend support.
static std::unique_ptr<GeoShape> decode_geo_shape(StringRef value, const DataTypePtr& type,
GeoParseStatus* parse_status = nullptr) {
if (!is_spatial_type(type)) {
return GeoShape::from_encoded(value.data, value.size);
}
GeoParseStatus status;
auto shape = GeoShape::from_wkb_bytes(value.data, value.size, status);
if (parse_status != nullptr) {
*parse_status = status;
}
return status == GEO_PARSE_OK ? std::move(shape) : nullptr;
be/src/format/table/iceberg/arrow_schema_util.cpp:95
- These annotations make the writer emit spatial Parquet metadata, but the Iceberg read path still has no matching support: V1
FieldDescriptor::convert_to_doris_typefalls back from GEOMETRY/GEOGRAPHY BYTE_ARRAY fields to VARBINARY/STRING, andPhysicalToLogicalConverterhas no conversion intoDataTypeSpatial. Consequently the regression's read-back after insert cannot materialize the WKB into a spatial column. Add the Parquet schema/converter mapping for both annotations before relying on this write-only Arrow mapping.
case iceberg::TypeID::GEOMETRY:
metadata[ICEBERG_BINARY_TYPE] = GEOMETRY_BINARY_TYPE_VALUE;
arrow_type = arrow::binary();
break;
be/test/exprs/function/function_geo_test.cpp:181
- This second test path creates a Geography result type with an empty algorithm as well. It violates the same
DataTypeSpatialinvariant and can fail under DCHECK-enabled builds before the unsupported-WKB behavior is tested; provideOGC:CRS84andsphericalfor the Geography branch.
auto result_type = make_nullable(std::make_shared<DataTypeSpatial>(
function_name == "st_geomfromwkb" ? TYPE_GEOMETRY : TYPE_GEOGRAPHY));
fe/fe-core/src/main/java/org/apache/doris/analysis/ColumnDef.java:431
- The legacy analyzer has the same root-only check: nested
ARRAY<GEOMETRY>/STRUCT<...GEOGRAPHY>types do not have GEOMETRY or GEOGRAPHY as their root primitive and therefore can pass internal-table analysis, even thoughDataTypeSpatialcannot be stored by Doris internal tables. Apply the recursive spatial rejection used by the Nereids path here as well.
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/DorisTypeToIcebergType.java:144 - These new mappings are also used by Iceberg CREATE/ALTER schema operations, but those paths only apply the existing VARIANT validation (
IcebergMetadataOps.java:399,772) and CREATE defaults to format-version 2 (IcebergMetadataOps.java:397). A spatial column can reach schema creation or alteration without the required v3/Parquet guard, even though the new writer only supports that combination; add the spatial validation before committing the schema.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/StAstext.java:80 - This special-case signature handling only makes
ST_ASTEXTbind a spatial operand.StDistance,StContains,StLength,StAsBinary,StGeometryType,StX, andStYstill expose only Varchar/String signatures, so Nereids rejects calls such asST_DISTANCE(iceberg_geography, iceberg_geography)before the new BE implementations can run. Add corresponding spatial-aware signatures/search handling for the supported functions while retaining the Geography metadata checks.
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ColumnDefinition.java:388 - This check only examines the root catalog primitive, so an internal column such as
ARRAY<GEOMETRY>orSTRUCT<shape GEOGRAPHY>bypasses the rejection because its root is ARRAY/STRUCT. The nestedDataTypeSpatialis later unsupported by storage, so reject spatial types recursively here (or reject nested spatial schemas explicitly) to preserve the stated internal-table restriction.
- Files reviewed: 53/53 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| import org.junit.Assert; | ||
| import org.junit.Test; |
| void DataTypeSpatial::to_protobuf(PTypeDesc* /* ptype */, PTypeNode* /* node */, | ||
| PScalarType* /* scalar_type */) const {} |
| DataTypePtr get_return_type_impl(const DataTypes&) const override { | ||
| return make_nullable(std::make_shared<DataTypeSpatial>(Impl::OUTPUT_TYPE)); | ||
| } |
| input_column->insert_data("0101000000000000000000F03F0000000000000040", 42); | ||
| auto input_type = std::make_shared<DataTypeString>(); | ||
| ColumnsWithTypeAndName arguments {{std::move(input_column), input_type, "wkb"}}; | ||
| auto result_type = make_nullable(std::make_shared<DataTypeSpatial>(TYPE_GEOGRAPHY)); |
| static void validateSpatialConversion(Type sourceType, ScalarType targetType, String columnName) { | ||
| if (!(sourceType instanceof ScalarType) || !((ScalarType) sourceType).isSpatialType()) { | ||
| throw new AnalysisException("Iceberg spatial write cannot convert input column '" + columnName | ||
| + "' from " + sourceType.toSql() + " to " + targetType.toSql()); | ||
| } |
| public static void validateWriteSchema(Table table, List<Column> columns) { | ||
| boolean writesVariant = columns.stream().anyMatch(column -> containsVariant(column.getType())); | ||
| boolean writesSpatial = columns.stream().anyMatch(column -> containsSpatial(column.getType())); | ||
| FileFormat fileFormat = getFileFormat(table); | ||
| if (writesVariant) { | ||
| if (writesVariant || writesSpatial) { | ||
| validateWriteSchema(columns, getFormatVersion(table), fileFormat); | ||
| } |
|
/review |
Gabriel39
left a comment
Gabriel39
left a comment
There was a problem hiding this comment.
Static review of this revision found three correctness/compatibility issues and one performance issue, detailed inline. I recommend addressing these before merging. I have not built this revision or run the BE/FE tests or Iceberg integration suite; the findings are based on tracing the implementation and its callers.
| continue; | ||
| } | ||
| for (size_t row = 0; row < column->size(); ++row) { | ||
| if (column->is_null_at(row)) { |
There was a problem hiding this comment.
[P1] Preserve the null map when validating spatial arguments
GeoFunction uses the default nullable implementation. PreparedFunctionImpl::default_implementation_for_nulls() calls unnest_nullable() before invoking execute_impl(), so this validator receives the nested ColumnSpatial, whose is_null_at() always returns false. A NULL row with the default empty payload is therefore parsed as WKB and returns InvalidArgument before the framework can merge the original null map into the result.
A batch containing both a valid geometry and NULL will fail for SELECT ST_AsText(geom) FROM spatial_source. The same path is reachable through ST_AsText(ST_GeomFromWKB(wkb)) when a nonconstant input column contains both valid and invalid WKB: the constructor produces a NULL with an empty payload for the invalid row, and the outer function rejects it. An all-NULL test can miss this because the framework may short-circuit it.
Please preserve the original null information at the validation boundary and add a mixed NULL/non-NULL function test, while retaining explicit errors for invalid non-NULL spatial values.
| if (_iceberg_schema != nullptr) { | ||
| ColumnNumbers column_numbers(block.columns()); | ||
| std::iota(column_numbers.begin(), column_numbers.end(), 0); | ||
| RETURN_IF_ERROR(validate_spatial_wkb_inputs(block, column_numbers)); |
There was a problem hiding this comment.
[P1] Do not require S2 longitude/latitude semantics for raw GEOMETRY writes
This call validates all spatial payloads through decode_geo_shape() -> GeoShape::from_wkb_bytes() -> WkbParse::readPoint() -> GeoPoint::from_coord() -> to_s2point(). The last step requires abs(x) <= 180 && abs(y) <= 90, regardless of the column's CRS.
For example, a valid GEOMETRY(EPSG:3857) value containing POINT (1000 2000) cannot be copied with INSERT INTO spatial_target SELECT geom FROM spatial_source, even when both schemas have exactly the same CRS: FE validation succeeds, but this writer rejects the payload as invalid WKB. ST_AsText and ST_X encounter the same restriction through the shared validator. The existing non-default CRS write test uses POINT (1 2), which does not expose this.
Please separate WKB structural validation/raw preservation from S2 computation support. Copying a projected geometry must not require its coordinates to be valid longitude/latitude. Iceberg defines GEOMETRY as CRS-parameterized with planar edges: https://iceberg.apache.org/spec/#primitive-types
| public static final List<FunctionSignature> SIGNATURES = ImmutableList.of( | ||
| FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).args(VarcharType.SYSTEM_DEFAULT), | ||
| FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).args(StringType.INSTANCE) | ||
| FunctionSignature.ret(new GeometryType("OGC:CRS84")).args(VarcharType.SYSTEM_DEFAULT), |
There was a problem hiding this comment.
[P1] Preserve compatibility of existing WKB constructor consumers
Changing the existing ST_GeomFromWKB/ST_GeometryFromWKB result from the legacy VARCHAR spatial encoding to GEOMETRY breaks existing function compositions outside Iceberg. The downstream validate_geography_semantics() now explicitly rejects GEOMETRY for ST_Distance, ST_Length, ST_Contains, and other measurement/relationship functions.
For example, this previously valid expression should return zero, but now reaches the NotSupported error:
SELECT ST_Distance(
ST_GeomFromWKB('0101000000000000000000F03F0000000000000040'),
ST_GeomFromWKB('0101000000000000000000F03F0000000000000040'));The new tests verify that GEOMETRY is rejected, but do not cover compatibility of these pre-existing SQL functions. Please retain the existing constructor contract via a separate typed constructor, or provide an explicit compatibility path for existing consumers.
| row); | ||
| } | ||
| GeoParseStatus parse_status; | ||
| if (decode_geo_shape(value, type, &parse_status) == nullptr) { |
There was a problem hiding this comment.
[P2] Reuse decoded shapes and validate constant arguments once
The pre-validation pass fully parses every WKB value and immediately destroys the resulting GeoShape; implementations such as StAsText and StDistance then decode the same payload again. This includes stream-buffer copies, coordinate parsing, and S2 object construction, which is expensive for large polygons.
It also defeats the constant-argument optimization in StDistance: for ST_Distance(geog_column, constant_geography), a ColumnConst still reports the logical batch size, so this loop decodes the same constant WKB N times before StDistance reaches its decode-once branch.
Please combine validation with decoding and reuse the result, handling constant arguments once per execution rather than once per logical row. No benchmark was run for this review, so this finding concerns the directly observable redundant work rather than a measured slowdown.
There was a problem hiding this comment.
Thanks for the review. We agree that the current validation path can decode the same WKB more than once, and that a ColumnConst may be validated once per logical row. This does not affect correctness, but eliminating it cleanly requires sharing decoded shapes across the validation and execution paths and handling constant-column semantics in the function framework. To keep this PR focused on the correctness and compatibility of Iceberg V3 spatial read/write support, we will track that as a follow-up performance optimization, with benchmarks and a focused design for cache lifetime and invalid-input behavior.
|
Codex automated review failed and did not complete. Error: Review step was failure (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
run buildall |
Gabriel39
left a comment
Gabriel39
left a comment
There was a problem hiding this comment.
Follow-up review of the latest revision: the legacy result-type restoration and the Geometry structural validator address parts of the earlier findings, but three correctness/compatibility issues remain, detailed inline. Please address these before merging. I have not run the complete BE/FE suites or Iceberg regression; the only runtime check in this review was an isolated probe of the original WKB validator source. I will add a separate comment with the concrete test-coverage gaps.
| String get_name() const override { return name; } | ||
| size_t get_number_of_arguments() const override { return Impl::NUM_ARGS; } | ||
| bool is_variadic() const override { return false; } | ||
| bool use_default_implementation_for_nulls() const override { return false; } |
There was a problem hiding this comment.
[P1] Keep NULL propagation for numeric GeoFunction implementations
Disabling the default nullable implementation for every GeoFunction introduces incorrect results in existing numeric functions. StPoint::execute() uses ColumnView<TYPE_DOUBLE>::value_at() without checking is_null_at(). ColumnView can unwrap a nullable column, but value_at() only reads the underlying payload; it does not propagate NULL. The result null map starts at zero, and the framework no longer merges the input null maps after execution.
For a nonconstant nullable DOUBLE column x, a NULL row with its default zero payload makes SELECT ST_AsText(ST_Point(x, 5)) FROM coordinates produce POINT (0 5) instead of NULL. ST_Distance_Sphere, ST_Angle_Sphere, and ST_Circle use the same unchecked numeric access pattern.
Please restrict the custom nullable handling to the functions that need it, or implement explicit null checks and propagation in every affected implementation. Run the existing VGeoFunctionsTest.function_geo_st_point_test, which already expects NULL for {Null(), 5} and {5, Null()}, and cover nonconstant mixed-null batches and constant/nullable combinations. A SQL NULL literal alone may be optimized away by FE and miss this regression.
| for (int row = 0; row < size; ++row) { | ||
| auto shape_value = input->get_data_at(row); | ||
| shape = GeoShape::from_encoded(shape_value.data, shape_value.size); | ||
| shape = decode_geo_shape(shape_value, input_type); |
There was a problem hiding this comment.
[P1] Preserve projected coordinates in accessors as well as the write validator
The new structural validator accepts projected GEOMETRY values, but the accessor still calls decode_geo_shape() -> GeoShape::from_wkb_bytes() -> WkbParse::readPoint() -> GeoPoint::from_coord() -> to_s2point(). The final step still enforces longitude/latitude limits without consulting the CRS.
Consequently, a non-NULL GEOMETRY(EPSG:3857) containing POINT (1000 2000) can now pass validation and be copied to Parquet, but ST_AsText(geom) returns NULL here. ST_X, ST_Y, ST_GeometryType, and ST_AsBinary share the same decoding restriction. This changes the previous explicit validation error into a silent NULL for valid projected data.
Please use coordinate-preserving WKB handling for accessors and exports; in particular, exporting the existing raw WKB should not depend on constructing an S2 shape. Add a Doris query test that verifies the non-NULL projected value's text, coordinates, type, and binary export after reading it back. The current Arrow writer round-trip test does not exercise these functions.
I independently compiled the original WKB validator source from this revision and verified that it accepts POINT (1000 2000). The downstream NULL result follows from the call chain above; I have not run a complete Doris SQL reproduction.
| constexpr uint32_t ewkb_m_flag = 0x40000000; | ||
| constexpr uint32_t ewkb_srid_flag = 0x20000000; | ||
| constexpr uint32_t ewkb_metadata_flags = ewkb_z_flag | ewkb_m_flag | ewkb_srid_flag; | ||
| if ((typeInt & ewkb_metadata_flags) != 0 || (typeInt >= 1000 && typeInt < 4000)) { |
There was a problem hiding this comment.
[P1] Retain SRID-bearing EWKB support in the legacy constructors
Restoring the legacy constructors' VARCHAR result does not restore their full input compatibility. LegacyStGeoFromWkb still calls GeoShape::from_wkb() -> WkbParse::parse_wkb() -> this shared readGeometry(), which now rejects the SRID flag unconditionally. Previously, this path consumed the SRID and parsed the coordinates.
There is already a regression for this in regression-test/suites/query_p0/sql_functions/spatial_functions/test_gis_function.groovy:460:
SELECT ST_AsText(ST_GeometryFromWKB(
'01010000208A11000068270210774C5D40B8DECA334C3B4240'));Its checked-in expected result is POINT (117.194767000297 36.46326301008). The fixture's type is 0x20000001 with SRID 4490, so this branch rejects it before reading the coordinates and the legacy constructor returns NULL.
Please separate the typed Iceberg WKB restrictions from legacy EWKB parsing, and run the existing SQL regression. Add a focused BE test proving that both legacy constructor aliases still accept this input while the new typed constructor follows its explicitly documented SRID policy.
|
The current tests are not sufficient to establish the feature's correctness yet. There is useful coverage for type/metadata round trips, schema rejection, malformed WKB, and Parquet annotations, but the integration suite inserts only one non-NULL point using the default CRS. The PR description also reports object/script compilation rather than execution of the BE spatial tests and the Iceberg regression. Please add or extend the following coverage and provide execution results for the final revision:
Please use the normal repository test runners and include results from the affected existing BE geo suite, the targeted FE tests, and the expanded Iceberg regression. For deterministic SQL results, use the repository's query/golden-output mechanism and generate the expected output via the runner; use |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
What problem does this PR solve?
Issue Number: N/A (DORIS-28228)
Related PR: N/A
Problem Summary:
Add Iceberg V3
GEOMETRYandGEOGRAPHYsupport for external tables. The change adds bidirectional Iceberg schema mapping with CRS and Geography algorithm preservation, Arrow/Parquet spatial annotations, raw WKB read/write handling, and spatial function support.Scope is limited to Iceberg external tables. Doris internal tables reject these types. Paimon, Lance, GeoArrow, spatial indexes, bounds pruning, and predicate pushdown are intentionally excluded.
For Geography, non-default CRS/algorithm values are preserved for read and write. Spatial measurement and relationship functions currently require
GEOGRAPHY(OGC:CRS84, spherical)and return a stable NotSupported error for other combinations to avoid incorrect calculations.Release note
Support Iceberg V3 GEOMETRY and GEOGRAPHY external-table schema mapping and raw-WKB Parquet read/write.
Check List (For Author)
Test
IcebergUtilsTest,IcebergSpatialWriteAnalyzerTest,SpatialConstructorFunctionTest, andColumnDefinitionTest).javax.annotation.Generatedand Nereids pattern-generator classpath issues.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)