StructTypes in a VirtualTableScan

[Blizzara]

Hey! I've been working on a Spark to Substrait (and back) converter, forked from https://github.com/apache/incubator-gluten/tree/v1.1.0/substrait/substrait-spark / https://github.com/substrait-io/substrait-java/pull/90/files (currently the fork is private but I hope to open source it too)

While aiming to support struct types in LocalRelations/VirtualTables, I ran into the check here:

substrait-java/core/src/main/java/io/substrait/relation/VirtualTableScan.java

Line 33 in 72bab63

&& rows.stream().noneMatch(r -> r == null || r.fields().size() != names.size());

If I've understood right, the "names" here should be a flattened list of field names, including column names but also recursively all names from struct types. This also aligns with the code in Isthmus. Overall, that means the check r.fields().size() != names.size() will trigger since there will be more names than top-level fields.

I'm pretty new to Substrait still so I may also be mistaken, but if my understanding is right, would it make sense to either:
a) remove the check,
b) change the check to confirm that names.size() >= r.fields().size(), or
c) iterate through fields to count the sub-fields as well before comparing the sizes?

[vbarua]

If I've understood right, the "names" here should be a flattened list of field names, including column names but also recursively all names from struct types.

I think that's correct, and

Overall, that means the check r.fields().size() != names.size() will trigger since there will be more names than top-level fields.

this is probably a bug.

Weakening the check like you suggest in b makes sense or making it more comprehensive like in c both sound reasonable to me. It would be good to add a test for this case as well to avoid regressing to this behaviour.