Gabriel39 commented on PR #67907:
URL: https://github.com/apache/doris/pull/67907#issuecomment-5659188350
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:
1. **NULL propagation and column wrappers.** Exercise `ST_Point`,
`ST_Distance_Sphere`, `ST_Angle_Sphere`, and `ST_Circle` with nonconstant
nullable numeric columns, mixed NULL/non-NULL rows, all-NULL batches, and
constant arguments on either side. For spatial accessors, cover both GEOMETRY
and GEOGRAPHY with nullable and constant inputs. Assert the output null map as
well as values. Reuse and run existing numeric-function NULL tests; the new
`ST_AsText`-only NULL test cannot detect the global `GeoFunction` regression.
2. **Legacy API compatibility and actual function results.** Run the
existing spatial SQL suite, including its EWKB/SRID fixture, and retain tests
for legacy WKB constructors composed with distance/relationship functions. Test
the typed constructor's separate metadata-rejection policy. Strengthen
`function_geo_st_distance_accepts_supported_geography`: it currently checks
only `Status::OK`, so an incorrect value or a NULL result would still pass.
Assert zero and a known nonzero spherical distance with an appropriate
tolerance, and add true/false relationship results and a nonzero length case
for supported Geography inputs.
3. **Projected coordinates through Doris accessors.** Read a non-NULL
`GEOMETRY(EPSG:3857)` containing `POINT (1000 2000)` and assert `ST_AsText`,
`ST_X`, `ST_Y`, `ST_GeometryType`, and `ST_AsBinary` results. Cover this after
an `INSERT ... SELECT` round trip. The writer test now preserves this point
through Arrow, which is useful, but does not test Doris decoding/accessors and
therefore misses silent NULL results.
4. **Iceberg end-to-end read/write and analyzer integration.** Extend the
one-row smoke test with supported lines/polygons, mixed NULL rows, `INSERT ...
SELECT`, and explicit NULL insertion into optional spatial columns. Read
spatial Parquet data through both reader implementations changed by this PR,
including dictionary/plain encoding and more than one batch. Verify raw WKB and
CRS/algorithm preservation for representative non-default metadata. Add actual
SQL analysis tests for spatial kind/CRS/algorithm mismatch and NULL handling in
INSERT and MERGE; direct calls to `IcebergSpatialWriteAnalyzer` do not prove
that sink binding and coercion invoke these checks correctly.
5. **New column serialization and ownership paths.** `DataTypeSpatialTest`
currently serializes one nonconstant Geometry value. Add multirow, nullable,
empty, and constant-column round trips for both spatial kinds, checking exact
payloads and logical row counts. Exercise the in-place filter/COW path and
mutation of a copied column to verify that the original payloads remain intact.
These tests cover the new ColumnSpatial/serialization implementation
independently of the SQL smoke test.
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 `test { sql ...; exception ... }` for expected errors. If an environment
prevents a suite from running, identify the unexecuted suite and the remaining
gap explicitly. Compiling test sources alone does not verify their assertions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
