You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "wypoon (via GitHub)" <gi...@apache.org> on 2024/10/09 22:53:14 UTC
[PR] Arrow: Remove unused fixed width binary reader classes [iceberg]
wypoon opened a new pull request, #11292:
URL: https://github.com/apache/iceberg/pull/11292
For fixed width binary, the FixedSizeBinaryXXX readers are actually used.
This is a follow-up to https://github.com/apache/iceberg/pull/11276.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Arrow: Remove unused fixed width binary reader classes [iceberg]
Posted by "wypoon (via GitHub)" <gi...@apache.org>.
wypoon commented on PR #11292:
URL: https://github.com/apache/iceberg/pull/11292#issuecomment-2403628522
@nastra I missed noticing earlier that the fixed width binary reader classes are no longer used.
I think this is it, though.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Arrow: Remove unused fixed width binary reader classes [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11292:
URL: https://github.com/apache/iceberg/pull/11292#discussion_r1796754363
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedColumnIterator.java:
##########
@@ -214,20 +214,6 @@ protected int nextBatchOf(
}
}
- public class FixedWidthTypeBinaryBatchReader extends BatchReader {
Review Comment:
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 `fixedWidthTypeBinaryBatchReader`as it's possible that consumers of Iceberg could use this. See also #3249 on how we did the deprecation and #7987 later removed this
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Arrow: Deprecate unused fixed width binary reader classes [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #11292:
URL: https://github.com/apache/iceberg/pull/11292
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Arrow: Remove unused fixed width binary reader classes [iceberg]
Posted by "wypoon (via GitHub)" <gi...@apache.org>.
wypoon commented on code in PR #11292:
URL: https://github.com/apache/iceberg/pull/11292#discussion_r1794387844
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedColumnIterator.java:
##########
@@ -278,10 +264,6 @@ public VarWidthTypeBatchReader varWidthTypeBatchReader() {
return new VarWidthTypeBatchReader();
}
- public FixedWidthTypeBinaryBatchReader fixedWidthTypeBinaryBatchReader() {
- return new FixedWidthTypeBinaryBatchReader();
- }
Review Comment:
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Arrow: Remove unused fixed width binary reader classes [iceberg]
Posted by "wypoon (via GitHub)" <gi...@apache.org>.
wypoon commented on code in PR #11292:
URL: https://github.com/apache/iceberg/pull/11292#discussion_r1797487178
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedColumnIterator.java:
##########
@@ -214,20 +214,6 @@ protected int nextBatchOf(
}
}
- public class FixedWidthTypeBinaryBatchReader extends BatchReader {
Review Comment:
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Arrow: Remove unused fixed width binary reader classes [iceberg]
Posted by "wypoon (via GitHub)" <gi...@apache.org>.
wypoon commented on code in PR #11292:
URL: https://github.com/apache/iceberg/pull/11292#discussion_r1797487178
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/parquet/VectorizedColumnIterator.java:
##########
@@ -214,20 +214,6 @@ protected int nextBatchOf(
}
}
- public class FixedWidthTypeBinaryBatchReader extends BatchReader {
Review Comment:
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org