Repository navigation
Conversation
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the change is cohesive and carries strong unit coverage (manifest-authority routing, native-Map decoding, per-row/ANY SRID, null structs, malformed-value error code, and full-double-precision round-trips are all tested). One low note on the completeness of the local WKT canonicalization for untested geometry types. Nit: ArrowToJavaObjectConverter uses the fully-qualified java.util.Map<?, ?> inline in two spots while the file could import java.util.Map for consistency — cosmetic only.
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Solid, well-tested change overall — the native Arrow struct<srid,wkb> decode path, manifest-authoritative type resolution, EWKT fallback, and WKT precision handling all look correct. One medium concern: the paren-aware splitMapMetadata fix is undone by consumers that re-split parseMapMetadata's joined string on the first comma, so decimal-keyed maps with geospatial values are still mis-parsed despite the new test suggesting otherwise.
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — clean, well-tested implementation of native Reyden struct<srid,wkb> decoding with correct enable/disable behavior at every nesting depth and preserved EWKT compatibility. The parseMapMetadataParts refactor correctly fixes parameterized-key (e.g. DECIMAL(10,2)) map splitting, and the WKT normalization/full-precision path is validated by round-trip tests. Only one minor consistency note (defensive-copy asymmetry in the geospatial base class).
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 2 Low
Looks good — solid, well-tested implementation of native Arrow struct<srid,wkb> decoding with correct manifest-authority handling, defensive WKB copying, per-row SRID preservation, and consistent enable/disable behavior at every nesting depth. All production parseMapMetadata(...).split(",") callers were migrated to the paren-aware parseMapMetadataParts, and the new exception constructor maps args correctly. Only two low-severity notes: per-row metadata re-parsing on the complex-disabled path, and missing unit coverage for the WKT-normalization heuristic on Z/M nested collections (presentation-only; WKB is preserved).
| chunkIterator.getColumnObjectAtCurrentRow( | ||
| columnIndex, requiredType, arrowMetadata, columnInfo, false); | ||
| return result == null ? null : result.toString(); | ||
| } |
There was a problem hiding this comment.
🔵 Low — MetadataParser.containsGeospatialType(arrowMetadata) is evaluated inside getObjectWithComplexTypeHandling, which runs once per cell (per column, per row). For a complex column with complex-datatype support disabled, this performs a full recursive parse/uppercase/cleanTypeName walk of the nested type string for every single row, even though the metadata string is invariant across all rows of a column. On large result sets with nested STRUCT/MAP/ARRAY metadata this is avoidable repeated work.
Consider hoisting the geospatial-presence decision to a per-column computation (the arrowMetadata/columnInfo are stable for the column) rather than recomputing per row. Not a correctness issue — behavior is right — purely an efficiency note.
(Anchored to the nearest changed line — see the description for the exact location.)
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
e0f8b4c to
9167ba8
Compare
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a well-structured, thoroughly-tested change. Native struct<srid,wkb> decoding, manifest-authoritative type resolution, defensive WKB copies, the DECIMAL-key MAP-split fix, and the new exception constructor all check out. One low note: for the disabled-complex-support path, geospatial-containing complex types now serialize via Databricks*.toString() instead of the legacy formatComplexTypeString, which can diverge for MAP columns and lacks a dedicated test.
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Solid, well-tested feature work decoding Reyden's native Arrow struct<srid,wkb> for GEOMETRY/GEOGRAPHY with recursive nesting, defensive WKB copies, and clean error mapping to INVALID_STATE. One medium concern: zero-SRID EWKT string output differs between top-level (bare WKT) and nested (SRID=0;...) paths, which contradicts the PR's stated goal of consistent behavior at every nesting depth.
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Nit
Looks solid — no correctness or contract issues found. The Reyden struct<srid,wkb> decode is driven by the manifest logical type (correctly guarded so the physical Arrow struct shape can't downgrade GEOMETRY/GEOGRAPHY), the EnableGeoSpatialSupport flag is threaded consistently through both the Arrow and SEA-inline paths, parseMapMetadataParts now tracks paren depth so parameterized keys like DECIMAL(10,2) split correctly, and WKB is defensively copied on construction and on getWKB(). Coverage is thorough (native/EWKT modes, ANY SRID, nesting, null rows, malformed→INVALID_STATE, full-double-precision round-trip). One nit below.
Other findings
- ⚪ Nit — The interface Javadoc for
getWKB()was updated to say "a copy of the WKB representation", but the overriding implAbstractDatabricksGeospatial.getWKB()still documents "@return the WKB representation as a byte array". Minor doc drift now that the method returnsArrays.copyOf(...); consider aligning the impl Javadoc for clarity.
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
| Object result = | ||
| chunkIterator.getColumnObjectAtCurrentRow( | ||
| columnIndex, requiredType, arrowMetadata, columnInfo, false); | ||
| return result == null ? null : result.toString(); |
There was a problem hiding this comment.
Could we avoid using result.toString() to format the whole container here? For STRUCT<geom:GEOMETRY(4326),blob:BINARY> with EnableComplexDatatypeSupport=0, the parser converts blob to byte[], and DatabricksStruct.toString() emits a JVM identity such as [B@.... That loses the binary value and produces an invalid JSON-like result. Please preserve non-geospatial fields when formatting this string, and add a mixed-field Arrow regression test.
| // Verify that GEOGRAPHY type is handled when geospatial support is disabled | ||
| assertFalse(geographyResult.hasNext()); | ||
| @Test | ||
| public void testNestedGeospatialReturnsOuterStringWhenComplexSupportDisabled() throws Exception { |
There was a problem hiding this comment.
Could we add a driver-level regression, or point to named replay cases, for nested native Arrow results and EWKT compatibility? This test mocks ArrowResultChunkIterator and contains only a geospatial field. The existing GeospatialTests covers top-level values but is not selected by PR CI's *IntegrationTests command, and the fake-service geospatial test skips Thrift. The linked live run may cover some combinations, but I cannot verify its test cases or delivery modes from this PR. Please make the coverage explicit for SEA native Arrow and SEA/Thrift EWKT, with the relevant geospatial and complex-support settings.
This branch has not been deployed
2 participants
Description
struct<srid:int32,wkb:binary>representation for bothGEOMETRYandGEOGRAPHYusing the logical type from the result manifest.EnableGeoSpatialSupportbehavior at every nesting depth: returnDatabricksGeometry/DatabricksGeographywhen enabled, and EWKT strings when disabled.ANYSRIDs), and null values without precision loss.Testing
GEOMETRYandGEOGRAPHYnested in arrays, map values, and structs.mainbaseline: https://github.com/databricks/databricks-driver-test/actions/runs/35947909157Telemetry Errors
INVALID_STATEerror code (1019).