handle nested struct in schemaString

[nicklan]

Handle parsing nested structs in schemaString. Setting iceberg compat causes this. Also enable the iceberg_compat_v1 test for dat, since we do now support column mapping.

Closes #253 .

[hntd187]

I never like these catch all serialization things, but seems like in this case it's somewhat of a necessary thing.

[zachschuermann]

yay!

[zachschuermann]

So then IIUC this is letting column/structfield metadata also include arbitrary string->json? Is there any API here other than the ability to just ask for the metadata back?

Not that i'm against it, just thinking of some implications: this makes serde_json::Map/Value part of the public API right?

We could also take the extreme approach and do Json(serde_json::Value)? or do the most specific of Map<Int, String>. or keep the in-between currently in the PR.

This comment is mostly questions/ideas I've thought of - I think most would be reasonable here?

[nicklan]

It does indeed make it part of the public API. I don't think this is exactly correct, but I think there's some work to be done to spec out in the protocol exactly what can go here.

This basically allows:

{
  "key": [number],
  "key": [string],
  "key": [bool],
  "key": {
      [pretty much any object]
   }
}

Allowing serde_json::Map<String, serde_json::Value> is basically equivalent to "anything in a json object" because json objects are just string -> Value maps. So what we're disallowing here is:

{
  "key": [1,2,3], // array
  "key": null
}

and that's it. I can't find use for that in the spec so I slightly prefer the more restrictive form. We can relax this later without breaking things, but it's harder to go from more loose to tighter.

[zachschuermann]

are we disallowing array/null? Isn't there serde_json::Value::Null and serde_json::Value::Array?

[hntd187]

it just means the top level of enum there has to be an object, what is contained in the map can technically be anything serde_json::Value can take

[nicklan]

are we disallowing array/null? Isn't there serde_json::Value::Null and serde_json::Value::Array?

We are only disallowing it at the top level. Inside the Object variant you could have anything, but at the top we only deser into the types I listed above.

[scovich]

The spec only says metadata is a "JSON map" and AFAIK delta-spark (spark, really) represents it as a String -> Any map (see Metadata.scala). So technically even Null and Array values would be allowed.

[nicklan]

Fair enough, I've updated this to now just have an Other variant that will catch anything we don't understand, plus a comment explaining why that's okay.

[zachschuermann]

i misunderstood yay thanks lgtm :)