Spec: Support geo type

[szehon-ho]

This is the spec change for #10260.

Also this is based closely on the decisions taken in the Parquet proposal for the same : apache/parquet-format#240

[szehon-ho]

@jiayuasu do you have any suggestion for default here?

[hsiang-c]

GeoMesa has 12 as the default: https://github.com/locationtech/geomesa/blob/geomesa-4.0.3/geomesa-z3/src/main/scala/org/locationtech/geomesa/curve/XZSFC.scala#L13

[jiayuasu]

12 sounds fine. CC @Kontinuation

[Kontinuation]

GeoMesa uses a high XZ2 resolution when working with key-value stores such as Accumulo and HBase, it is not appropriate to always use a resolution that high for partitioning data (for instance, GeoMesa on FileSystems).

XZ2 resolution 11~12 works for city-scale data, but will generate too many partitions for country-scale or world-scale data. I'd like to have a smaller default value such as 7 to be safe on various kinds of data.

[szehon-ho]

I could add this section later too, once its implemented (same for ORC below)

[RussellSpitzer]

[Appendix G](#appendix-g)

[rdblue]

How does this work? Does Iceberg need to interpret each WKB to produce this value? Will it be provided by Parquet?

[szehon-ho]

Yes, once we switch to Geometry logical type from Parquet we will get these stats from Parquet.

[flyrain]

Should we mention that it is the parquet type BoundingBox?

[szehon-ho]

yea will add a footnote here

[jiayuasu]

@szehon-ho BTW, the reason why we had a separate bbox statistics in havasu is to be compatible with existing Iceberg tables. Since this is to add the native geometry support, so lower_bound and upper_bound are good choices.

[Kontinuation]

I thought that the bounds were stored as WKB-encoded points (according to Appendix D and G), and WKB encodes dimensions of geometries in the header. It is more consistent to make bound values the same type/representation as the field data type.

More sophisticated coverings in Parquet statistics cannot be easily mapped to lower_bounds and upper_bounds, so do we simply use the bbox statistics and ignore the coverings for now?

[zhangfengcdt]

I think it is ok since these two bounds are optional and in case they are not presented, it still follow the spec.

[jiayuasu]

Does it support different dimensions like XY, XYZ, XYM, XYZM? If yes, how can we tell if the binary is for XYZ or XYM?

We should say For Geometry type, this is a WKB-encoded Point composed of the min value of each dimension in all Points in the Geometry. Then we don't have to worry about the Z and M value.

CC @szehon-ho

[szehon-ho]

We should say For Geometry type, this is a WKB-encoded Point composed of the min value of each dimension in all Points in the Geometry. Then we don't have to worry about the Z and M value.

@jiayuasu @Kontinuation @wgtmac Done, thanks.

[szehon-ho]

@flyrain

Should we mention that it is the parquet type BoundingBox?

Actually looking again after some time, not sure how to mention this here, as that is filetype specific. This is an optional field, and only set if type is parquet and bounding_box is set, but that's implementation detail .

[rdblue]

Except for geometry?

[szehon-ho]

@rdblue @flyrain hmm is there a specific reason? I think it could work technically as Geometry can be compared, unless I miss something

[flyrain]

Maybe that's fine if it is comparable, but practically people will always use xz2, right? I'm not sure, but wondering if there is some implications, e.g., too expensive, or super high cardinality, so that we don't recommend user to use the original GEO value as the partition spec.

[szehon-ho]

yea I think its possible to do (it's just the wkb value after all), you are right , not sure if any good use case. Yea we have to get the wkb in any case, i am not sure if its that expensive, but can check. But I guess the cardinality is the same consideration as any other type (uuid for example), and we let the user choose ?

[flyrain]

Are we going to support bucketing on GEO?

[szehon-ho]

I think its possible, again not sure the utility. Geo boils down to just WKB bytes

[flyrain]

I feel that the argument for identity can apply here as well. In that case, we can support it, but it's users' call to use it or not.

[rdblue]

We may want to change this to be like identity, using Any except [...].

I would not include geo as a source column for bucketing because there is not a clear definition of equality for geo. The hash would depend on the structure of the object and weird things happen when two objects are "equal" (for some definition) but have different hash values.

[jiayuasu]

1. When we say OGC:CRS84, the value you put in this field should be the following PROJJSON string (see GeoParquet spec)

{
    "$schema": "https://proj.org/schemas/v0.5/projjson.schema.json",
    "type": "GeographicCRS",
    "name": "WGS 84 longitude-latitude",
    "datum": {
        "type": "GeodeticReferenceFrame",
        "name": "World Geodetic System 1984",
        "ellipsoid": {
            "name": "WGS 84",
            "semi_major_axis": 6378137,
            "inverse_flattening": 298.257223563
        }
    },
    "coordinate_system": {
        "subtype": "ellipsoidal",
        "axis": [
        {
            "name": "Geodetic longitude",
            "abbreviation": "Lon",
            "direction": "east",
            "unit": "degree"
        },
        {
            "name": "Geodetic latitude",
            "abbreviation": "Lat",
            "direction": "north",
            "unit": "degree"
        }
        ]
    },
    "id": {
        "authority": "OGC",
        "code": "CRS84"
    }
}

2. Both crs and crs_kind field are optional. But when the CRS field presents, the crs_kind field must present. In this case, since we hard code this crs field in this phase, then we need to set crs_kind field (string) to PROJJSON.

[wgtmac]

Should we include this example in the parquet spec as well?

[wgtmac]

BTW, should we advise accepted forms or values for CRS and Edges?

[jiayuasu]

@wgtmac We should add this value to the Parquet spec for sure. CC @zhangfengcdt

@szehon-ho There is another situation mentioned in the GeoParquet spec: If the CRS field presents but its value is null, it means the data is in unknown CRS. This situation happens sometimes because the writer somehow cannot find or lose the CRS info. Do we want to support this? I think we can use the empty string to cover this case

[jiayuasu]

accepted forms or values for CRS and Edges

If we borrow the conclusion from Parquet Geometry proposal, then C,T,E fields are the follows:

C is a string. Based on what I understand from this PR, @szehon-ho made this field a required field, which is fine.
T is optional and a string. Currently, it only allows this value PROJJSON. When it is not provided, it defaults to PROJJSON too.
E is a string. The only allowed value is PLANAR in this phase. Based on what I understand from this PR, @szehon-ho made this field a required field, which is fine. @szehon-ho According to our meeting with Snowflake, I think maybe we can allow SPHERICAL too? We can add in the spec that: currently it is unsafe to perform partition transform / bounding box filtering when E = SPHERICAL because they are built based on PLANAR edges. It is the reader's responsibility to decide if they want to use partition transform / bounding box filtering.

[zhangfengcdt]

BTW, the parquet-format uses crs_encoding instead of crs_kind. Do we want to unify the names as well?

[wgtmac]

What is the expectation from C, T and E fields from the Parquet/ORC data files? Are they required to be set by Iceberg? From the Parquet spec, only E is required, both C and T are optional.

[szehon-ho]

Changed from type to encoding.

Actually i made all three have default values (C="OGC:CRS84", CE="PROJJSON", E="planar"), will that make sense? Trying to make the common case be less verbose. @wgtmac @jiayuasu

[jiayuasu]

@szehon-ho

1. Does it make sense to include the following CRS84 example from the Parquet Geometry PR?

  /**
   * Coordinate Reference System, i.e. mapping of how coordinates refer to
   * precise locations on earth. Writers are not required to set this field.
   * Once crs is set, crs_encoding field below MUST be set together.
   * For example, "OGC:CRS84" can be set in the form of PROJJSON as below:
   * {
   *     "$schema": "https://proj.org/schemas/v0.5/projjson.schema.json",
   *     "type": "GeographicCRS",
   *     "name": "WGS 84 longitude-latitude",
   *     "datum": {
   *         "type": "GeodeticReferenceFrame",
   *         "name": "World Geodetic System 1984",
   *         "ellipsoid": {
   *             "name": "WGS 84",
   *             "semi_major_axis": 6378137,
   *             "inverse_flattening": 298.257223563
   *         }
   *     },
   *     "coordinate_system": {
   *         "subtype": "ellipsoidal",
   *         "axis": [
   *         {
   *             "name": "Geodetic longitude",
   *             "abbreviation": "Lon",
   *             "direction": "east",
   *             "unit": "degree"
   *         },
   *         {
   *             "name": "Geodetic latitude",
   *             "abbreviation": "Lat",
   *             "direction": "north",
   *             "unit": "degree"
   *         }
   *         ]
   *     },
   *     "id": {
   *         "authority": "OGC",
   *         "code": "CRS84"
   *     }
   * }
   */

2. It is ok to have them all fixed to default values for this phase.

[szehon-ho]

@jiayuasu i put it in the example (if you render the page). let me know if its not what you meant.

[wgtmac]

What syntax to use for an engine to create the geometry type? Does it require C/T/E to appear in the type?

[szehon-ho]

Related to above comment, I think these will all be optional (take a default value if not specified).

[szehon-ho]

Hi all fyi i have unfortunately encountered some problems while remote and probably cant update this, will come back to this after i get back home in two weeks.

[szehon-ho]

@jiayuasu @Kontinuation @wgtmac @flyrain @rdblue sorry for the delay, as I only got access now. Updated the pr.

[dmitrykoval]

Can we maybe explicitly mention here that both "planar" and "spherical" are supported as edge type enum values?

[szehon-ho]

@dmitrykoval i was debating this.

I guess we talked about it before, but the Java reference implementation, we cannot easily do pruning (file level, row level, or partition level) because the JTS library and the XZ2 only support non-spherical. We would need new metrics types, new Java libraries , and new partition transform proposals if we wanted to support it in Java reference implementation.

But if we want to support it, Im ok to list it here and have checks to just skip pruning for spherical geometry columns.
@flyrain @jiayuasu @Kontinuation does it make sense?

[dmitrykoval]

I see. I think if "planar" is the default edge type, then there shouldn't be many changes to the planar geometry code path, except for additional checks to skip some partitioning/pruning cases, right?

Regarding the reference implementation of the "spherical" type, do we need to fully support it from day one, or can we maybe mark it as optional in the initial version of the spec? For example, it would work if the engine supports it, but by default, we would fall back to the planar edge type?

[jiayuasu]

We could list spherical as an allowed edge type here. Maybe just mark it that it is not safe to perform partition transform or lower_bound/upper_bound filtering when the edge is spherical. We did the same in the Parquet Geometry PR.

[szehon-ho]

Yea , forgot to mention explicitly that in Iceberg, pruning is always an optional feature for reads, so no issue.

[wgtmac]

For geometry type, this is a WKB-encoded point composed of the min value of each dimension among all component points of all geometry objects for the file.

As we are finishing the PoC on the Parquet side, the remaining issue is what value to write to min_value/max_value fields of statistics and page index. To give some context, Parquet requires min_value/max_value fields to be set for page index and statistics are used to generate page index. The C++ PoC is omitting min_value/max_value values and the Java PoC is pretending geometry values are plain binary values while collecting the stats. Should we do similar things here? Then the Iceberg code can directly consume min_value/max_value from statistics instead of issuing another call to get the specialized GeometryStatistics which is designed for advanced purpose.

@Kontinuation @zhangfengcdt @paleolimbot @jiayuasu

[jiayuasu]

@wgtmac do you mean that Iceberg uses the Parquet Geometry GeometryStatistics or Parquet Geometry uses the min_value/max_value idea from Iceberg?

[wgtmac]

I mean the latter. The ColumnOrder of the new geometry type is undefined as specified at https://github.com/apache/parquet-format/pull/240/files#diff-834c5a8d91719350b20995ad99d1cb6d8d68332b9ac35694f40e375bdb2d3e7cR1144. It means that the min_value/max_value fields are meaningless and should not be used. I'm not sure if it is a good idea to set min_value/max_value fields in the same way as lower_bounds/upper_bounds of Iceberg.

[Kontinuation]

I suggest defining the sort order of geometry columns as WKB-encoded points in the parquet format spec. This is the most simple yet useful way of defining the min and max bounds for geometry columns, and the sort order is better to be well-defined rather than left undefined.

[wgtmac]

I agree that it is better to explicitly define the column order than being undefined. If we go with this approach, the format PR and two PoC impls need to reflect this change, which might get more complicated.

[szehon-ho]

Is there anything for this specific line we need to change? As long as we get from Parquet some way we are ok here, but is the format of the lower/upper bound still ok?

[wgtmac]

No, I was thinking if Parquet could do better by doing similar things in the future.

[RussellSpitzer]

I think maybe we should just link out for the requirements here since it's a bit complicated.

The description as well could be

Simple feature geometry Appendix G, Parameterized by ....

I also don't think we should allow it to be unset ... can we just require that a subclass is always picked? We could recommend a set of defaults for engines to set on field creation but I'm not sure we need to be that opinionated here.

[RussellSpitzer]

link again

[RussellSpitzer]

link

[RussellSpitzer]

might as well fix this one too while we are at it :)

[RussellSpitzer]

I think maybe we should just link out for the requirements here since it's a bit complicated. Remove "an object"

The description as well could be

Simple feature geometry Appendix G, Parameterized by ....

I also don't think we should allow it to be unset ... can we just require that a subclass is always picked? We could recommend a set of defaults for engines to set on field creation but I'm not sure we need to be that opinionated here.

[rdblue]

I think we can be more specific here and call out the standard that we are referencing, like we do with IEEE 754. This is "Geometry features as WKB(link) stored in coordinate reference system C and edge type E (see Appendix G)"

I would also say that "If not specified, C is OGC:CRS84 and E is planar".

[RussellSpitzer]

Let's move these details out of the description and either into the footnotes ore another section for geometry.

[rdblue]

Could you remove the reformatting so we can more easily look at the changes?

[rdblue]

I would probably not specify how to hash geometry because we don't yet know how to do it correctly. The reason why we have the second table (hash requirements that are not part of bucket) is that we don't want anyone to forget that float and double should hash to the same value.

[rdblue]

@szehon-ho, I don't think we should specify this or allow geometry in bucket transforms because of issues with equality.

[szehon-ho]

Thanks @rdblue @RussellSpitzer addressed review comments.

• remove auto formatting
• added links to Appendix G
• optimized storage of lower_bounds and upper_bounds (thanks @rdblue for suggestion to skip WKB here)
• remove XZ2 partition transform (which we can add later, to unblock adding the type for V3)

[szehon-ho]

@rdblue am not entirely sure where this part of the spec is used. Should it also match the above (the more optimized serialization for stats)?

[rdblue]

This is used for default values and for encoding values in JSON expressions for filtering.

What about using WKT here instead of WKB?

[szehon-ho]

Good idea, added.

[rdblue]

I think we need to be a little more specific here. The way I read this is that you can take min and max values for each dimension in the point, but that isn't sufficient for spherical edges.

I think this needs to state that the lower and upper bounds must be less than or equal (or greater than or equal) to the values of any point that is located on an edge of the geometry object. In other words, the bounding box must contain all points that are in the geometry object.

If we don't have that requirement, then there could be a point that is outside of the bounding box. If that's the case, then a query that includes the point may not overlap the bounding box and we cannot use it for filtering.

[szehon-ho]

Per a previous conversation , it could be beneficial to have it in this form even for spherical edges. An engine could do some conversion from the bounding box to be useful for spherical edge.

Do you mean, you do not want this option at all ? (I suppose due to risk of mis-interpreting it)

[rdblue]

I'd prefer not to reference a PR.

[szehon-ho]

Removed these, can add the logical types once the pr is merged in Parquet

[rdblue]

Why big endian? Most of the time we use little endian in the format, with the only exception being the encoded decimal values.

[rdblue]

Also, what is the encoding for these values? 8-byte IEEE 754?

[szehon-ho]

Makes sense, added little-endian and 8-byte IEEE 754 (ie double type) for each coordinate

[szehon-ho]

@rdblue thanks for further review. Would appreciate a clarification for this comment #10981 (comment), otherwise everything else is addressed.

[Kontinuation]

Missing hash specification for geometry primitive type in the table above. We should add a new row for geometry and annotate it with [6].

[Kontinuation]

Is it always concatenated by 4 floating-point values? If it is not the case, we'll have a hard time figuring out if the point is in XYZ or XYM when there are 3 encoded dimensions. I suggest we use the WKB encoding of points here as well.

Enforcing the appearance of all 4 components and allow filling NaN for optional components also works, as it is more similar to the BoundingBox struct defined in the Parquet spec.

[wgtmac]

+1. This is why we have introduced a separate bounding box stats in the Parquet proposal to avoid the issue.