steveloughran commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3451821489
##########
core/src/main/java/org/apache/iceberg/puffin/PuffinReader.java:
##########
@@ -71,13 +71,10 @@ public FileMetadata fileMetadata() throws IOException {
PuffinCompressionCodec footerCompression = PuffinCompressionCodec.NONE;
for (Flag flag : decodeFlags(footer, footerStructOffset)) {
- switch (flag) {
- case FOOTER_PAYLOAD_COMPRESSED:
- footerCompression = PuffinFormat.FOOTER_COMPRESSION_CODEC;
- break;
- default:
- throw new IllegalStateException("Unsupported flag: " + flag);
- }
+ footerCompression =
+ switch (flag) {
+ case FOOTER_PAYLOAD_COMPRESSED ->
PuffinFormat.FOOTER_COMPRESSION_CODEC;
Review Comment:
this has moved the default: path to a compile-time check, which is fine *for
now*, but what if a puffin reader encounters a puffin file with an unknown
flag? I think the meaningful message of L79 should be restored, rather than
whatever gets raised by the jvm here, which may or may not be as informative
##########
core/src/main/java/org/apache/iceberg/puffin/PuffinFormat.java:
##########
@@ -126,16 +128,16 @@ private static ByteBuffer compress(Compressor compressor,
ByteBuffer input) {
static ByteBuffer decompress(PuffinCompressionCodec codec, ByteBuffer input)
{
switch (codec) {
- case NONE:
+ case NONE -> {
Review Comment:
this is the one where the move is harder to justify, but as it still
eliminates the risk of fall through's, valid
##########
core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java:
##########
@@ -122,16 +122,12 @@ private static class CommitErrorHandler extends
DefaultErrorHandler {
@Override
public void accept(ErrorResponse error) {
switch (error.code()) {
- case 404:
- throw new NoSuchTableException("%s", error.message());
- case 409:
- throw new CommitFailedException("Commit failed: %s",
error.message());
- case 500:
- case 502:
- case 503:
- case 504:
- throw new CommitStateUnknownException(
- new ServiceFailureException("Service failed: %s: %s",
error.code(), error.message()));
+ case 404 -> throw new NoSuchTableException("%s", error.message());
Review Comment:
FWIW I personally thing the project should pull out the http error codes as
constants into one class place so you've got constants you can find in the IDE
across the entire codebase. I'd say "not for this PR", though this class is
the place to add the constants 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]
