Skip to content

Conversation

@wypoon
Copy link
Contributor

@wypoon wypoon commented Oct 9, 2024

For fixed width binary, the FixedSizeBinaryXXX readers are actually used.
In VectorizedArrowReader::read:

          case UUID:
          case FIXED_WIDTH_BINARY:
          case FIXED_LENGTH_DECIMAL:
            vectorizedColumnIterator
                .fixedSizeBinaryBatchReader()
                .nextBatch(vec, typeWidth, nullabilityHolder);
            break;

This is a follow-up to #11276.

@github-actions github-actions bot added the arrow label Oct 9, 2024
Comment on lines 281 to 283
public FixedWidthTypeBinaryBatchReader fixedWidthTypeBinaryBatchReader() {
return new FixedWidthTypeBinaryBatchReader();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw in my IDE that this method is never called, which means that the chain of fixed width binary reader classes are never used.

@wypoon
Copy link
Contributor Author

wypoon commented Oct 9, 2024

@nastra I missed noticing earlier that the fixed width binary reader classes are no longer used.
I think this is it, though.

}
}

public class FixedWidthTypeBinaryBatchReader extends BatchReader {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can just remove this. We should add a deprecation message (@deprecated since 1.7.0, will be removed in 1.8.0) here and to fixedWidthTypeBinaryBatchReaderas it's possible that consumers of Iceberg could use this. See also #3249 on how we did the deprecation and #7987 later removed this

Copy link
Contributor Author

@wypoon wypoon Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will deprecate rather than remove the unused code.
There was no revapi failure, so I thought the removal was ok.
In any case, it's a useful exercise to show that with the code removed, the build and all tests still pass.

@wypoon wypoon force-pushed the remove_unused_binary_reader branch from 6f7dbd6 to c2e1ce4 Compare October 11, 2024 23:25
@wypoon wypoon changed the title Arrow: Remove unused fixed width binary reader classes Arrow: Deprecate unused fixed width binary reader classes Oct 11, 2024
@nastra nastra merged commit 12ff959 into apache:main Oct 14, 2024
zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants