fix: accept GeoJSON strings for Edm.GeographyPoint in AzureSearchWriter - #2556
fix: accept GeoJSON strings for Edm.GeographyPoint in AzureSearchWriter#2556HCL (chon3806) wants to merge 5 commits into
Conversation
|
Hey HCL (@chon3806) 👋! We use semantic commit messages to streamline the release process. Examples of commit messages with semantic prefixes:
To test your commit locally, please follow our guild on building from source. |
There was a problem hiding this comment.
Pull request overview
This PR fixes Azure AI Search ingestion failures when AzureSearchWriter is given Edm.GeographyPoint values in StringType columns containing GeoJSON by parsing those strings into the expected struct shape before JSON serialization.
Changes:
- Added a private normalization step to parse
StringTypeGeoJSON intostruct<type:string, coordinates:array<double>>forEdm.GeographyPointindex fields. - Wired the new normalization into
AzureSearchWriter.prepareDFbefore schema parity checks and request serialization. - Added an end-to-end Scala test covering GeographyPoint values supplied as strings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cognitive/src/main/scala/com/microsoft/azure/synapse/ml/services/search/AzureSearch.scala | Adds GeoJSON string → struct conversion for Edm.GeographyPoint fields and applies it during DF preparation. |
| cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/search/split2/SearchWriterSuitePart2.scala | Adds an end-to-end test that writes docs with GeoJSON GeographyPoint stored as strings. |
@microsoft-github-policy-service agree |
|
Hi Brendan Walsh (@BrendanWalsh) — friendly nudge on this one when you have a moment. The CLA is signed, the semantic title check passes, and the Copilot review's 3 comments have all been addressed in 95a7ae1. CI hasn't run yet (fork PR — needs a maintainer to approve workflows). Happy to address any further feedback. Thanks! |
95a7ae1 to
dbe4ff5
Compare
Review: safe and worth mergingRebased onto current What it does. Azure Search declares Why it is correct.
Rebase note. The rebase adapted this PR to master's Validation. All three review threads were already addressed in Running |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2556 +/- ##
==========================================
+ Coverage 86.76% 87.00% +0.23%
==========================================
Files 338 338
Lines 18785 18811 +26
Branches 1804 1806 +2
==========================================
+ Hits 16299 16366 +67
+ Misses 2486 2445 -41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dbe4ff5 to
7dafbd4
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Review details
Suppressed comments (2)
cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/search/split2/SearchWriterSuitePart2.scala:323
- Including "" and whitespace-only strings in
wrongShapesis likely to fail with a FAILFAST JSON parse error ("Malformed records are detected") before your custom GeoJSON-shape validation runs. If the intent is to exercise shape validation, remove the blank values here (and keep malformed/blank inputs covered by the dedicated FAILFAST test).
"""{"type":"Point","coordinates":[-122.3493, 47.6205, 12.0]}""",
"""{"type":"Point","coordinates":[-122.3493, null]}""",
"",
" "
)
cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/search/split2/SearchWriterSuitePart2.scala:312
- The comment says blank strings are “syntactically valid JSON” and that FAILFAST will accept them, but empty/whitespace strings aren’t valid JSON. Since this test is for valid JSON with the wrong GeoJSON shape, the comment should reflect that (or the blank cases should be tested in the malformed-JSON FAILFAST test instead).
This issue also appears on line 319 of the same file.
// Every one of these is syntactically valid JSON (or blank), so Spark's FAILFAST parser
// accepts it and yields a partially-null struct. Without explicit shape validation these
// would be silently indexed as a null location instead of failing the write.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
66745c4 to
56f989d
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
…er (microsoft#2420) Azure AI Search expects spatial values as GeoJSON objects, but when users supplied a StringType column the writer JSON-escaped the entire string and the service rejected the request with HTTP 400. Convert string GeographyPoint columns into the canonical struct<type, coordinates> shape via from_json before serialization, mirroring the existing Edm.DateTimeOffset handling. Existing struct-based input is unchanged.
…or GeographyPoint conversion - Replace unicode em-dash with ASCII to avoid encoding/scalastyle surprises. - Replace 'microsoft#2420' in Scaladoc (member-reference syntax) with a full URL. - Make convertGeographyPointToStruct private[ml] so it can be exercised by in-tree unit tests without requiring live Azure Search credentials. - Add two non-network unit tests covering the string->struct rewrite, null preservation, and the no-op path for already-structured columns.
- Use Spark's FAILFAST parsing mode in from_json so malformed GeoJSON surfaces an explicit exception instead of being silently coerced to null and shipped to Azure Search. - Clarify Scaladoc to state the conversion is top-level-only (mirroring convertDateTimeToISO8601), and document the FAILFAST behavior. - Add a unit test asserting malformed GeoJSON fails fast at materialization. - Document why the end-to-end test's count assertion is sufficient: with fatalErrors=true (default) any service-side rejection throws, so a passing count proves the documents were accepted as valid spatial objects.
The GeoJSON struct was declared twice: once inline in convertGeographyPointToStruct and once in edmTypeToSparkType. Those two must stay identical or checkSchemaParity rejects every converted row, so derive the parse schema from edmTypeToSparkType and share a single GeographyPointEdmType constant for the type string. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Spark's FAILFAST mode only rejects syntactically malformed JSON. Valid JSON
of the wrong shape ({"foo":"bar"}, {"type":"Point"}, an empty string) parses
into a partially- or fully-null struct, so a GeographyPoint that used to fail
loudly in checkSchemaParity would instead be silently indexed as a null
location.
Validate the parsed value is a genuine GeoJSON Point (type == "Point" with
exactly two non-null coordinates) and raise an error naming the column and
the offending value otherwise. NULL inputs are still preserved as NULL.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
56f989d to
254466b
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Review details
Suppressed comments (2)
cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/search/split2/SearchWriterSuitePart2.scala:323
- Including empty/whitespace strings in
wrongShapeswill typically fail at thefrom_json(..., FAILFAST)parse step ("Malformed records are detected") rather than producing the custom "not a valid GeoJSON Point" error, so the assertions below may fail intermittently across Spark versions/optimizations. These cases are already covered by the dedicated malformed-JSON FAILFAST test; keepwrongShapesfocused on syntactically-valid JSON with the wrong structure.
"",
" "
)
cognitive/src/test/scala/com/microsoft/azure/synapse/ml/services/search/split2/SearchWriterSuitePart2.scala:312
- The comment says these values are “syntactically valid JSON (or blank)” and that FAILFAST accepts blanks, but empty/whitespace strings are not valid JSON. With
from_json(..., FAILFAST)they are likely to throw a parse error (“Malformed records are detected”) rather than reaching the GeoJSON shape validation, making this test misleading/brittle (and overlapping with the prior malformed-JSON FAILFAST test).
This issue also appears on line 321 of the same file.
// Every one of these is syntactically valid JSON (or blank), so Spark's FAILFAST parser
// accepts it and yields a partially-null struct. Without explicit shape validation these
// would be silently indexed as a null location instead of failing the write.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
Fixes #2420.
AzureSearchWriterfailed with400 Bad Requestwhen a user provided anEdm.GeographyPointvalue as aStringTypecolumn containing GeoJSON. The string was JSON-escaped during serialization, so Azure AI Search received a quoted string instead of a spatial object:Fix
Added
convertGeographyPointToStructinAzureSearch.scala, mirroring the existingconvertDateTimeToISO8601handling. For every top-level index field declared asEdm.GeographyPoint, if the corresponding DataFrame column is aStringType, it is parsed viafrom_json(withmode = FAILFAST) into the canonicalstruct<type: string, coordinates: array<double>>beforecheckSchemaParityandto_json. Struct-based inputs continue to work unchanged.FAILFASTwas chosen so malformed GeoJSON surfaces as a loudSparkExceptionat row materialization rather than silently nulling out the field (the defaultPERMISSIVEbehavior offrom_json). This is consistent withAzureSearchWriter.writedefaulting tofatalErrors = true.Tests
Added in
SearchWriterSuitePart2.scala:Handle GeoJSON GeographyPoint fields supplied as strings— writes documents with aStringTypelocation column and verifies the index ingests them (any400would throw viafatalErrors = true).convertGeographyPointToStruct parses GeoJSON strings into structsconvertGeographyPointToStruct leaves struct columns untouchedconvertGeographyPointToStruct fails fast on malformed GeoJSON instead of silently nullingCompatibility
private[ml]) — no public API changes.Review feedback addressed
All 3 Copilot review comments have been addressed in
95a7ae1:from_jsontoFAILFASTmode so malformed GeoJSON is not silently nulled.convertDateTimeToISO8601).Update: malformed GeoJSON now fails loudly
Review flagged that
from_json(..., FAILFAST)only rejects syntactically malformed JSON. Values that parse but have the wrong shape --{"foo":"bar"},{"type":"Polygon",...}, a 1- or 3-elementcoordinatesarray -- yielded a struct of nulls and were silently ingested into the search index.The transform now validates the shape and raises an actionable error. Failing loudly is the right default here: before this PR the same input hard-failed at
checkSchemaParity, so silent nulls would be a softening of the existing contract, and nulls in a search index are near-undetectable downstream. Genuine SQLNULLstill passes through untouched.The expression was validated with a standalone PySpark 3.5.0 probe across 12 cases before being wired in. 5/5 tests pass locally, scalastyle clean.