You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nssalian (via GitHub)" <gi...@apache.org> on 2026/06/19 19:07:26 UTC
[PR] Core: Migrate switch statements to switch expressions [iceberg]
nssalian opened a new pull request, #16881:
URL: https://github.com/apache/iceberg/pull/16881
Continues [#15494](https://github.com/apache/iceberg/pull/15494) (@mxm) and [#15176](https://github.com/apache/iceberg/pull/15176) (@manuzhang)
## Summary
Converts statement-form `switch` blocks in `iceberg-core` to expression-form (arrow syntax). Eliminates `StatementSwitchToExpressionSwitch` ErrorProne warnings and removes the risk of accidental fall-through in the converted code.
## Rationale for the change
Per the [[DISCUSS] Switching to new switches](https://lists.apache.org/thread/k2cg9v6rxs8lb74qbok166k68h1r0f3k) dev list thread, consensus was option 2
- merge sweeping syntax changes shortly before the release. Targeting 1.12.
## Changes
- `iceberg-core` only. Pattern-matching for `instanceof` - I'll create a separate follow-up PR.
- The diff was generated by ErrorProne's auto-patcher (`-XepPatchChecks:StatementSwitchToExpressionSwitch -XepPatchLocation:IN_PLACE`), then `spotlessApply`, plus a small manual cleanup in `PrimitiveWrapper.sizeInBytes()` to keep trailing comments inline.
## Test
- locally ran the core tests
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3457731906
##########
core/src/main/java/org/apache/iceberg/deletes/DeleteGranularity.java:
##########
@@ -48,14 +48,10 @@ public enum DeleteGranularity {
@Override
public String toString() {
- switch (this) {
- case FILE:
- return "file";
- case PARTITION:
- return "partition";
- default:
- throw new IllegalArgumentException("Unknown delete granularity: " + this);
- }
+ return switch (this) {
+ case FILE -> "file";
+ case PARTITION -> "partition";
Review Comment:
we should probably keep the default branch here
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#issuecomment-4755520830
I think its a good idea to add `'-Xep:StatementSwitchToExpressionSwitch:ERROR',` to `baseline.gradle` so that we can enforce this going forward and verify that all refactors are caught.
tested locally and found more places where we should refactor
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3450276632
##########
core/src/main/java/org/apache/iceberg/BaseFile.java:
##########
@@ -316,79 +316,102 @@ public void put(int i, Object value) {
@Override
protected <T> void internalSet(int pos, T value) {
switch (pos) {
- case 0:
+ case 0 -> {
this.content = value != null ? FileContent.fromId((Integer) value) : FileContent.DATA;
return;
Review Comment:
other places are GenericManifestEntry, GenericManifestFile, GenericPartitionFieldSummary, DeletionVectorStruct, StandardKeyMetadata
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3445366235
##########
core/src/main/java/org/apache/iceberg/avro/Avro.java:
##########
@@ -248,26 +248,20 @@ static Context deleteContext(Map<String, String> config) {
private static CodecFactory toCodec(String codecAsString, String compressionLevel) {
CodecFactory codecFactory;
try {
- switch (Codec.valueOf(codecAsString.toUpperCase(Locale.ENGLISH))) {
- case UNCOMPRESSED:
- codecFactory = CodecFactory.nullCodec();
- break;
- case SNAPPY:
- codecFactory = CodecFactory.snappyCodec();
- break;
- case ZSTD:
- codecFactory =
- CodecFactory.zstandardCodec(
- compressionLevelAsInt(compressionLevel, ZSTD_COMPRESSION_LEVEL_DEFAULT));
- break;
- case GZIP:
- codecFactory =
- CodecFactory.deflateCodec(
- compressionLevelAsInt(compressionLevel, GZIP_COMPRESSION_LEVEL_DEFAULT));
- break;
- default:
- throw new IllegalArgumentException("Unsupported compression codec: " + codecAsString);
- }
+ codecFactory =
+ switch (Codec.valueOf(codecAsString.toUpperCase(Locale.ENGLISH))) {
+ case UNCOMPRESSED -> CodecFactory.nullCodec();
+ case SNAPPY -> CodecFactory.snappyCodec();
+ case ZSTD ->
+ CodecFactory.zstandardCodec(
+ compressionLevelAsInt(compressionLevel, ZSTD_COMPRESSION_LEVEL_DEFAULT));
+ case GZIP ->
+ CodecFactory.deflateCodec(
+ compressionLevelAsInt(compressionLevel, GZIP_COMPRESSION_LEVEL_DEFAULT));
+ default ->
+ throw new IllegalArgumentException(
+ "Unsupported compression codec: " + codecAsString);
Review Comment:
Let me check for other such instances
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3457731906
##########
core/src/main/java/org/apache/iceberg/deletes/DeleteGranularity.java:
##########
@@ -48,14 +48,10 @@ public enum DeleteGranularity {
@Override
public String toString() {
- switch (this) {
- case FILE:
- return "file";
- case PARTITION:
- return "partition";
- default:
- throw new IllegalArgumentException("Unknown delete granularity: " + this);
- }
+ return switch (this) {
+ case FILE -> "file";
+ case PARTITION -> "partition";
Review Comment:
we should probably keep the default branch here
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3454428181
##########
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:
Will restore 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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3457767798
##########
core/src/main/java/org/apache/iceberg/puffin/PuffinFormat.java:
##########
@@ -105,14 +105,16 @@ static int readIntegerLittleEndian(byte[] data, int offset) {
static ByteBuffer compress(PuffinCompressionCodec codec, ByteBuffer input) {
switch (codec) {
- case NONE:
+ case NONE -> {
Review Comment:
please make sure to remove unnecessary {} in other places as well
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3457879585
##########
core/src/main/java/org/apache/iceberg/ScanTaskParser.java:
##########
@@ -113,19 +113,12 @@ private static FileScanTask fromJson(JsonNode jsonNode, boolean caseSensitive) {
taskType = TaskType.fromTypeName(taskTypeStr);
}
- switch (taskType) {
- case FILE_SCAN_TASK:
- return FileScanTaskParser.fromJson(jsonNode, caseSensitive);
- case DATA_TASK:
- return DataTaskParser.fromJson(jsonNode);
- case FILES_TABLE_TASK:
- return FilesTableTaskParser.fromJson(jsonNode);
- case ALL_MANIFESTS_TABLE_TASK:
- return AllManifestsTableTaskParser.fromJson(jsonNode);
- case MANIFEST_ENTRIES_TABLE_TASK:
- return ManifestEntriesTableTaskParser.fromJson(jsonNode);
- default:
- throw new UnsupportedOperationException("Unsupported task type: " + taskType.typeName());
- }
+ return switch (taskType) {
+ case FILE_SCAN_TASK -> FileScanTaskParser.fromJson(jsonNode, caseSensitive);
+ case DATA_TASK -> DataTaskParser.fromJson(jsonNode);
+ case FILES_TABLE_TASK -> FilesTableTaskParser.fromJson(jsonNode);
+ case ALL_MANIFESTS_TABLE_TASK -> AllManifestsTableTaskParser.fromJson(jsonNode);
+ case MANIFEST_ENTRIES_TABLE_TASK -> ManifestEntriesTableTaskParser.fromJson(jsonNode);
Review Comment:
I believe we need to keep the default branch here. Anything that we parse from a string should rather fail with `Unsupported task type` instead of seeing a weird JVM error.
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3457765852
##########
core/src/main/java/org/apache/iceberg/puffin/PuffinFormat.java:
##########
@@ -105,14 +105,16 @@ static int readIntegerLittleEndian(byte[] data, int offset) {
static ByteBuffer compress(PuffinCompressionCodec codec, ByteBuffer input) {
switch (codec) {
- case NONE:
+ case NONE -> {
Review Comment:
can be simplified to
```
case NONE -> input.duplicate();
case LZ4 -> {
// TODO requires LZ4 frame compressor, e.g.
// https://github.com/airlift/aircompressor/pull/142
}
case ZSTD -> compress(new ZstdCompressor(), input);
```
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3452404695
##########
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:
yes agree that we should keep this here and in a few other places that remove the `default` branch
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#issuecomment-4776834456
this should be good to go once the remaining comments have been addressed
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#issuecomment-4790598904
@nastra thanks for the review. Yes, I'll open follow up PRs for that and the instance of changes.
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3457785832
##########
core/src/main/java/org/apache/iceberg/variants/PrimitiveWrapper.java:
##########
@@ -83,44 +83,51 @@ public T get() {
@Override
public int sizeInBytes() {
switch (type()) {
- case NULL:
- case BOOLEAN_TRUE:
- case BOOLEAN_FALSE:
+ case NULL, BOOLEAN_TRUE, BOOLEAN_FALSE -> {
return 1; // 1 header only
- case INT8:
+ }
Review Comment:
most of these can be removed
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3457883650
##########
core/src/main/java/org/apache/iceberg/ScanTaskParser.java:
##########
@@ -113,19 +113,12 @@ private static FileScanTask fromJson(JsonNode jsonNode, boolean caseSensitive) {
taskType = TaskType.fromTypeName(taskTypeStr);
}
- switch (taskType) {
- case FILE_SCAN_TASK:
- return FileScanTaskParser.fromJson(jsonNode, caseSensitive);
- case DATA_TASK:
- return DataTaskParser.fromJson(jsonNode);
- case FILES_TABLE_TASK:
- return FilesTableTaskParser.fromJson(jsonNode);
- case ALL_MANIFESTS_TABLE_TASK:
- return AllManifestsTableTaskParser.fromJson(jsonNode);
- case MANIFEST_ENTRIES_TABLE_TASK:
- return ManifestEntriesTableTaskParser.fromJson(jsonNode);
- default:
- throw new UnsupportedOperationException("Unsupported task type: " + taskType.typeName());
- }
+ return switch (taskType) {
+ case FILE_SCAN_TASK -> FileScanTaskParser.fromJson(jsonNode, caseSensitive);
+ case DATA_TASK -> DataTaskParser.fromJson(jsonNode);
+ case FILES_TABLE_TASK -> FilesTableTaskParser.fromJson(jsonNode);
+ case ALL_MANIFESTS_TABLE_TASK -> AllManifestsTableTaskParser.fromJson(jsonNode);
+ case MANIFEST_ENTRIES_TABLE_TASK -> ManifestEntriesTableTaskParser.fromJson(jsonNode);
Review Comment:
hm nvm, in this case I think this would have already been handled inside `TaskType.fromTypeName()`
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3457743570
##########
core/src/main/java/org/apache/iceberg/deletes/DeleteGranularity.java:
##########
@@ -48,14 +48,10 @@ public enum DeleteGranularity {
@Override
public String toString() {
- switch (this) {
- case FILE:
- return "file";
- case PARTITION:
- return "partition";
- default:
- throw new IllegalArgumentException("Unknown delete granularity: " + this);
- }
+ return switch (this) {
+ case FILE -> "file";
+ case PARTITION -> "partition";
Review Comment:
generally speaking, I think we should keep the default branch everywhere, because we don't know what string values a client might pass, so it's not always safe to remove the default branch. Can you please make sure to restore all default branches across the entire PR?
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #16881:
URL: https://github.com/apache/iceberg/pull/16881
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3450313022
##########
core/src/main/java/org/apache/iceberg/V2Metadata.java:
##########
@@ -86,16 +86,20 @@ public <T> T get(int pos, Class<T> javaClass) {
private Object get(int pos) {
switch (pos) {
- case 0:
+ case 0 -> {
Review Comment:
same as above. please also fix other places that are affected
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3450106348
##########
core/src/main/java/org/apache/iceberg/avro/Avro.java:
##########
@@ -248,26 +248,17 @@ static Context deleteContext(Map<String, String> config) {
private static CodecFactory toCodec(String codecAsString, String compressionLevel) {
CodecFactory codecFactory;
try {
- switch (Codec.valueOf(codecAsString.toUpperCase(Locale.ENGLISH))) {
- case UNCOMPRESSED:
- codecFactory = CodecFactory.nullCodec();
- break;
- case SNAPPY:
- codecFactory = CodecFactory.snappyCodec();
- break;
- case ZSTD:
- codecFactory =
- CodecFactory.zstandardCodec(
- compressionLevelAsInt(compressionLevel, ZSTD_COMPRESSION_LEVEL_DEFAULT));
- break;
- case GZIP:
- codecFactory =
- CodecFactory.deflateCodec(
- compressionLevelAsInt(compressionLevel, GZIP_COMPRESSION_LEVEL_DEFAULT));
- break;
- default:
Review Comment:
we should probably keep the default branch
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3450106348
##########
core/src/main/java/org/apache/iceberg/avro/Avro.java:
##########
@@ -248,26 +248,17 @@ static Context deleteContext(Map<String, String> config) {
private static CodecFactory toCodec(String codecAsString, String compressionLevel) {
CodecFactory codecFactory;
try {
- switch (Codec.valueOf(codecAsString.toUpperCase(Locale.ENGLISH))) {
- case UNCOMPRESSED:
- codecFactory = CodecFactory.nullCodec();
- break;
- case SNAPPY:
- codecFactory = CodecFactory.snappyCodec();
- break;
- case ZSTD:
- codecFactory =
- CodecFactory.zstandardCodec(
- compressionLevelAsInt(compressionLevel, ZSTD_COMPRESSION_LEVEL_DEFAULT));
- break;
- case GZIP:
- codecFactory =
- CodecFactory.deflateCodec(
- compressionLevelAsInt(compressionLevel, GZIP_COMPRESSION_LEVEL_DEFAULT));
- break;
- default:
Review Comment:
we should probably keep the default branch
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3457708553
##########
core/src/main/java/org/apache/iceberg/avro/InternalReader.java:
##########
@@ -170,65 +170,48 @@ public ValueReader<?> variant(
public ValueReader<?> primitive(Pair<Integer, Type> partner, Schema primitive) {
LogicalType logicalType = primitive.getLogicalType();
if (logicalType != null) {
- switch (logicalType.getName()) {
- case "date":
- return ValueReaders.ints();
-
- case "time-micros":
- return ValueReaders.longs();
-
- case "timestamp-millis":
+ return switch (logicalType.getName()) {
+ case "date" -> ValueReaders.ints();
+ case "time-micros" -> ValueReaders.longs();
+ case "timestamp-millis" -> {
// adjust to microseconds
ValueReader<Long> longs = ValueReaders.longs();
- return (ValueReader<Long>) (decoder, ignored) -> longs.read(decoder, null) * 1000L;
Review Comment:
can be simplified the same way as in my other comment
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3457835744
##########
core/src/main/java/org/apache/iceberg/SingleValueParser.java:
##########
@@ -168,20 +181,25 @@ public static Object fromJson(Type type, JsonNode defaultValue) {
byte[] fixedBytes =
BaseEncoding.base16().decode(defaultValue.textValue().toUpperCase(Locale.ROOT));
return ByteBuffer.wrap(fixedBytes);
- case BINARY:
+ }
+ case BINARY -> {
Preconditions.checkArgument(
defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue);
byte[] binaryBytes =
BaseEncoding.base16().decode(defaultValue.textValue().toUpperCase(Locale.ROOT));
return ByteBuffer.wrap(binaryBytes);
- case LIST:
+ }
+ case LIST -> {
Review Comment:
can be simplified
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#issuecomment-4755543307
> I think its a good idea to add `'-Xep:StatementSwitchToExpressionSwitch:ERROR',` to `baseline.gradle` so that we can enforce this going forward and verify that all refactors are caught.
>
>
>
> tested locally and found more places where we should refactor
Makes sense. Did you find it core or other modules? I kept this scoped to core for now.
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3454424473
##########
core/src/main/java/org/apache/iceberg/BaseFile.java:
##########
@@ -316,79 +316,102 @@ public void put(int i, Object value) {
@Override
protected <T> void internalSet(int pos, T value) {
switch (pos) {
- case 0:
+ case 0 -> {
this.content = value != null ? FileContent.fromId((Integer) value) : FileContent.DATA;
return;
Review Comment:
Will find these and fix them.
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3457975076
##########
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:
> what if a puffin reader encounters a puffin file with an unknown flag
This is switch-over-enum, not switch-over-string - an unexpected value fails early in `PuffinReader#decodeFlags`, never reaching this switch.
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3457699903
##########
core/src/main/java/org/apache/iceberg/avro/GenericAvroReader.java:
##########
@@ -173,67 +173,50 @@ public ValueReader<?> variant(
public ValueReader<?> primitive(Type partner, Schema primitive) {
LogicalType logicalType = primitive.getLogicalType();
if (logicalType != null) {
- switch (logicalType.getName()) {
- case "date":
+ return switch (logicalType.getName()) {
// Spark uses the same representation
- return ValueReaders.ints();
-
- case "time-micros":
- return ValueReaders.longs();
-
- case "timestamp-millis":
+ case "date" -> ValueReaders.ints();
+ case "time-micros" -> ValueReaders.longs();
+ case "timestamp-millis" -> {
// adjust to microseconds
ValueReader<Long> longs = ValueReaders.longs();
- return (ValueReader<Long>) (decoder, ignored) -> longs.read(decoder, null) * 1000L;
-
- case "timestamp-micros":
- case "timestamp-nanos":
- // both are handled in memory as long values, using the type to track units
- return ValueReaders.longs();
-
- case "decimal":
- return ValueReaders.decimal(
- ValueReaders.decimalBytesReader(primitive),
- ((LogicalTypes.Decimal) logicalType).getScale());
-
- case "uuid":
- return ValueReaders.uuids();
-
- default:
- throw new IllegalArgumentException("Unknown logical type: " + logicalType);
- }
+ yield (ValueReader<Long>) (decoder, ignored) -> longs.read(decoder, null) * 1000L;
Review Comment:
can be simplified to
```
case "timestamp-millis" -> // adjust to microseconds
(decoder, ignored) -> ValueReaders.longs().read(decoder, null) * 1000L;
```
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3457770868
##########
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:
{} are not needed in this entire block
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3457789465
##########
core/src/main/java/org/apache/iceberg/variants/VariantVisitor.java:
##########
@@ -77,9 +77,10 @@ public static <R> R visit(VariantValue value, VariantVisitor<R> visitor) {
}
return visitor.object(object, fieldNames, fieldResults);
-
- default:
+ }
+ default -> {
Review Comment:
```suggestion
default ->
```
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3445095902
##########
core/src/main/java/org/apache/iceberg/avro/Avro.java:
##########
@@ -248,26 +248,20 @@ static Context deleteContext(Map<String, String> config) {
private static CodecFactory toCodec(String codecAsString, String compressionLevel) {
CodecFactory codecFactory;
try {
- switch (Codec.valueOf(codecAsString.toUpperCase(Locale.ENGLISH))) {
- case UNCOMPRESSED:
- codecFactory = CodecFactory.nullCodec();
- break;
- case SNAPPY:
- codecFactory = CodecFactory.snappyCodec();
- break;
- case ZSTD:
- codecFactory =
- CodecFactory.zstandardCodec(
- compressionLevelAsInt(compressionLevel, ZSTD_COMPRESSION_LEVEL_DEFAULT));
- break;
- case GZIP:
- codecFactory =
- CodecFactory.deflateCodec(
- compressionLevelAsInt(compressionLevel, GZIP_COMPRESSION_LEVEL_DEFAULT));
- break;
- default:
- throw new IllegalArgumentException("Unsupported compression codec: " + codecAsString);
- }
+ codecFactory =
+ switch (Codec.valueOf(codecAsString.toUpperCase(Locale.ENGLISH))) {
+ case UNCOMPRESSED -> CodecFactory.nullCodec();
+ case SNAPPY -> CodecFactory.snappyCodec();
+ case ZSTD ->
+ CodecFactory.zstandardCodec(
+ compressionLevelAsInt(compressionLevel, ZSTD_COMPRESSION_LEVEL_DEFAULT));
+ case GZIP ->
+ CodecFactory.deflateCodec(
+ compressionLevelAsInt(compressionLevel, GZIP_COMPRESSION_LEVEL_DEFAULT));
+ default ->
+ throw new IllegalArgumentException(
+ "Unsupported compression codec: " + codecAsString);
Review Comment:
I believe this default branch is no longer necessary in the new switch statement. The compilation will fail if the switch expression doesn't cover all possible input values.
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#issuecomment-4791520680
From the discussion in the community sync up, any similar changes going forward should be in individual PRs and would be reviewer's discretion to allow those in.
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3450308876
##########
core/src/main/java/org/apache/iceberg/V1Metadata.java:
##########
@@ -267,18 +254,20 @@ public <T> T get(int pos, Class<T> javaClass) {
private Object get(int pos) {
switch (pos) {
- case 0:
+ case 0 -> {
Review Comment:
I think { isn't needed here anywhere, right?
##########
core/src/main/java/org/apache/iceberg/V1Metadata.java:
##########
@@ -365,36 +354,51 @@ public <T> T get(int pos, Class<T> javaClass) {
private Object get(int pos) {
switch (pos) {
- case 0:
+ case 0 -> {
Review Comment:
same as above
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3461693482
##########
core/src/main/java/org/apache/iceberg/puffin/PuffinFormat.java:
##########
@@ -105,14 +105,16 @@ static int readIntegerLittleEndian(byte[] data, int offset) {
static ByteBuffer compress(PuffinCompressionCodec codec, ByteBuffer input) {
switch (codec) {
- case NONE:
+ case NONE -> {
Review Comment:
some have interleaved doc strings so I kept them. but yeah will clean up.
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3450272024
##########
core/src/main/java/org/apache/iceberg/BaseFile.java:
##########
@@ -316,79 +316,102 @@ public void put(int i, Object value) {
@Override
protected <T> void internalSet(int pos, T value) {
switch (pos) {
- case 0:
+ case 0 -> {
this.content = value != null ? FileContent.fromId((Integer) value) : FileContent.DATA;
return;
Review Comment:
all of these return statements can be removed now. Please also address the same thing in other places that were updated
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3450274476
##########
core/src/main/java/org/apache/iceberg/BaseFile.java:
##########
@@ -316,79 +316,102 @@ public void put(int i, Object value) {
@Override
protected <T> void internalSet(int pos, T value) {
switch (pos) {
- case 0:
+ case 0 -> {
this.content = value != null ? FileContent.fromId((Integer) value) : FileContent.DATA;
return;
Review Comment:
effectively this can be a single line now: `case 0 -> ...`
--
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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
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: 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] Core: Migrate switch statements to switch expressions [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16881:
URL: https://github.com/apache/iceberg/pull/16881#discussion_r3457743570
##########
core/src/main/java/org/apache/iceberg/deletes/DeleteGranularity.java:
##########
@@ -48,14 +48,10 @@ public enum DeleteGranularity {
@Override
public String toString() {
- switch (this) {
- case FILE:
- return "file";
- case PARTITION:
- return "partition";
- default:
- throw new IllegalArgumentException("Unknown delete granularity: " + this);
- }
+ return switch (this) {
+ case FILE -> "file";
+ case PARTITION -> "partition";
Review Comment:
generally speaking, I think we should keep the default branch everywhere, because we don't know what string values a client might pass, so it's not always safe to remove the default branch. Can you please make sure to restore all default branches across the entire PR?
--
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