rdblue commented on a change in pull request #830:
URL: https://github.com/apache/iceberg/pull/830#discussion_r441022967
##########
File path: parquet/src/main/java/org/apache/iceberg/parquet/PruneColumns.java
##########
@@ -45,13 +44,16 @@ public Type message(MessageType message, List<Type> fields)
{
for (int i = 0; i < fields.size(); i += 1) {
Type originalField = message.getType(i);
Type field = fields.get(i);
- if (selectedIds.contains(getId(originalField))) {
- builder.addField(originalField);
- fieldCount += 1;
- } else if (field != null) {
- builder.addField(field);
- fieldCount += 1;
- hasChange = true;
+ Integer fieldId = getId(originalField);
+ if (fieldId != null) {
+ if (selectedIds.contains(fieldId)) {
Review comment:
Why is this not `if (fieldId != null && selectedIds.contains(fieldId))`?
The else case is used when a sub-field is projected by ID. So the question
is whether a sub-field can be projected if its parents aren't mapped. I think
we should allow it because it would be confusing to have a value mapped, but
still get nulls because a parent is not.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
