Collect lower and upper bounds for nested struct fields in ParquetMetrics about iceberg HOT 8 CLOSED

@rdblue I see a comment in ParquetMetrics$fromMetadata, which says TODO: allow struct nesting, but not maps or arrays.

Could you describe why we need to exclude maps and arrays? We have proper statistics for keys/values in maps as well as for elements in lists, don't we? For example, we might support predicates on elements of lists at some point.

I am asking because the simplest option to enable statistics for nested structs is by replacing fileSchema.asStruct().field(fieldId) with fileSchema.findField(fieldId) in ParquetMetrics$fromMetadata. If we want to collect statistics only for nested structs, the logic will be more complicated.

from iceberg.

The reason for now is that this is how the stats and dictionary filters work. If a column has stats, then it is assumed to be at the top level. Structs are logical groupings and don't affect the meaning of a filter, so they are okay.

If we want to collect stats from maps and lists, then we need to have a plan for how to use them. That means implementing more filters like contains, containsKey, or containsValue. Since we don't have those yet, we don't need to store the data for them.

from iceberg.

Based on the discussion above we could add support for struct metrics and struct-based filtering. Filtering on structs would be a good incremental feature to add for complex schemas where often logical grouping can really help. Especially since there is a PR available in Spark now that one can cherry pick to push down struct-based filters. If we agree then I can work on this (unless @aokolnychyi has one on the way already).

from iceberg.

@prodeezy, sounds good to me! I'll review the PRs.

from iceberg.

Also, I'm updating the summary of this issue to lower/upper bounds instead of min/max. The spec makes no guarantee that these are min/max values so that variable-length types can truncate and save space.

from iceberg.

@rdblue am I right that valueCounts and nullValueCounts, which are persisted for keys/values in maps as well as for elements in lists, are not used later on? Shall we keep storing them?

from iceberg.

You're right that they aren't used right now. But let's keep them because we will probably want to add more predicates.

from iceberg.

@prodeezy @rdblue I'll submit a PR for this issue after PR#131, which introduces a new test suite that will be extended to cover nested struct fields once this issue is solved.

from iceberg.