[Parquet] ProjectionMask::columns name handling is bug prone

[mapleFU]

Describe the bug

ProjectionMask::columns in parquet allowing selecting columns, this is created in #6871 . However, this method is bug prone.

To Reproduce

Assume schema:

{
  a: int,
  aa: int
}

Select "a" will also making "aa" selected.

Expected behavior

I think the interface should be deprecated. Outside should pass explicit the require path. Or inner should receive the path.

Parquet allows "a.b" column, so I don't think dot is a good way.

Additional context

No

[mapleFU]

cc @alamb @etseidl

[etseidl]

True, Parquet gives you much rope to hang yourself with, but I think most users would expected a dotted path to work. I think a fix that matches on path elements rather than the concatenated result would fix the 'a' vs 'aa' issue.

[alamb]

I think the interface should be deprecated. Outside should pass explicit the require path. Or inner should receive the path.

I think I am missing something. Per https://docs.rs/parquet/latest/parquet/arrow/struct.ProjectionMask.html#method.columns

The API is
pub fn columns<'a>(
schema: &SchemaDescriptor,
names: impl IntoIterator<Item = &'a str>,
) -> Self

How does this confuse a and aa?

Also, since it accepts an iterator of strings (it doesn't split the on .) it would faithfully follow the paths

[etseidl]

I believe the issue is this line. The paths are converted to single strings, and starts_with is used to find common roots. We should probably keep the paths as Vec<&str> and compare each node. I don't think this code is performance critical so a slower check should be ok.

[alamb]

Ah makes sense -- thank you

[mapleFU]

but I think most users would expected a dotted path to work

I think we cannot assuming this. The best way is to pass-in a ColumnPath, and searching this path rather than column.

[mapleFU]

Personally, I don't use "." in my code. But parquet might used to store metadata. Imagine:

1. a mysql or data analysis system uses "tablename.information_schema" or something else as column ( this is commonly used, right?)
2. a system might use special character to store "internal column" ( or hidden column )

So I don't think "." can be regard as unexists. Maybe we can:

1. Uses column path
2. Says this only works on root columns, leafs are not included.

[etseidl]

I think it's fine to add another function that takes a ColumnPath to handle this case, but I don't feel a need to deprecate the existing API. I already have a PR to address the bug you reported, so I could add this new method there.