nssalian commented on PR #18434: URL: https://github.com/apache/iceberg/pull/18434#issuecomment-6355379546
Thanks @randreucetti for the fix, and thanks @amogh-jahagirdar for helping review this while I was out. I finally got to look at this, and it looks right to me. A few review comments: - The fix is in the shared arrow module, so could you add the Spark test to v4.0 and v4.1 too? #16087 did the same for an earlier fix in this builder. - For `vectorized = true`, assert that the scan ran columnar. Without that, the test still passes if the scan ever falls back to the row reader. - The description still uses the old test names. Could you update it? On your question about field lookup: `VectorizedSparkParquetReaders.buildReader` (3.5 to 4.2) and `ArrowReader.buildReader` both pass the same file schema to `TypeWithSchemaVisitor.visit` and to the builder, so `getType(currentPath())` resolves the variant group. An unmapped variant in a name-mapped file has no id and is skipped, the same as `primitive()`. This should also fix the same crash in `ArrowReader`, which builds over the full file schema. I'll follow up with PRs for #18601 (projected shredded variants) on top of this one. They will flip `projectingShreddedVariantUnsupported` to expect a row-based reader. Let's get this fix in first. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
