Handle nested properties in GeoJSON reader and writer #208
Conversation
| @@ -208,6 +208,12 @@ fn write_str_prop<W: Write>(mut out: W, colname: &str, v: &str) -> Result<()> { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| fn write_json_prop<W: Write>(mut out: W, colname: &str, v: &str) -> Result<()> { | |||
| let colname = colname.replace('\"', "\\\""); | |||
There was a problem hiding this comment.
What's this escaping about? Is this captured in a test somewhere?
There was a problem hiding this comment.
That's identical to the other write* methods
geozero/geozero/src/geojson/geojson_writer.rs
Lines 198 to 199 in cdda9a8
geozero/geozero/src/geojson/geojson_writer.rs
Lines 204 to 205 in cdda9a8
Any embedded " in the column name must be escaped with a \" or else the JSON will be invalid. I'm not sure if there's an existing test for this, but it would be separate from this change at hand.
There was a problem hiding this comment.
Ok, I created #209 as a followup task.
| JsonValue::Number(v) => { | ||
| if v.is_f64() { | ||
| processor.property(i, key, &ColumnValue::Double(v.as_f64().unwrap()))? | ||
| } else if v.is_i64() { | ||
| processor.property(i, key, &ColumnValue::Long(v.as_i64().unwrap()))? | ||
| } else if v.is_u64() { | ||
| processor.property(i, key, &ColumnValue::ULong(v.as_u64().unwrap()))? | ||
| } else { | ||
| unreachable!() | ||
| } |
There was a problem hiding this comment.
This was refactored so that the outer match value could be exhaustive.
### Change list - Support writing Struct, Array, Datetime columns from GeoArrow through geozero. - Vendor geozero GeoJSON updates (georust/geozero#208 and georust/geozero#206) - Handle null attribute values when writing to geozero. - add `writeGeoJSON` to JS binding - Closes #588. ~~I'm still getting issues with the GeoJSONWriter excluding fields. In particular, writing Overture data to GeoJSON completely omits list and struct fields, giving invalid GeoJSON with ", ,"~~ ---- Note: tested this via the Python API
|
Anything else I can do to push this along? |
| // For null values omit the property | ||
| JsonValue::Null => false, | ||
| // Array(Vec<Value>), Object(Map<String, Value>) | ||
| _ => processor.property(i, key, &ColumnValue::String(&value.to_string()))?, |
There was a problem hiding this comment.
@kylebarron - just working on some changelog notes... your PR description says
The GeoJSONReader currently silently omits any nested property. This serializes the Value for an Array or Object back to a string to store within a ColumnValue::Json.
But it looks like we were not omitting them, rather we were previously decoding them as ColumnValue::String, right?
There was a problem hiding this comment.
Hmm maybe I conflated the reading and writing parts in the PR description. I don't remember exactly right now.
I think the primary change is that we change from the String variant to Json. I don't know what was the behavior of value.to_string(). I assume that was not guaranteed to be the same as serde_json::to_string?
There was a problem hiding this comment.
I think the output is the same. I still think your change to the Reader makes sense - if we have JSON we might as well put it in a ColumnValue::JSON rather than a ColumnValue::String. I just wanted to get the changelog notes correct.
There was a problem hiding this comment.
The GeoJSONReader currently silently omits any nested property. This serializes the
Valuefor an Array or Object back to a string to store within aColumnValue::Json.The GeoJSONWriter currently silently omits any
ColumnValue::JsonorColumnValue::Binary. This PR writes the contained JSON string to the output writer. It leaves writing binary to a future PR (which should probably at least have the option of converting to hex?)I'm not sure why but I was unable to run tests locally because they failed with
I'm guessing this has something to do with Postgres' libpq, but
brew install libpqsays it's already installed.