Flink, Aliyun, MR, Delta-lake, Hive3, Parquet, Data: replaced .size() > 0 with isEmpty() #8819
Conversation
....17/flink/src/main/java/org/apache/iceberg/flink/sink/shuffle/DataStatisticsCoordinator.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Fokko Driesprong <fokko@apache.org>
|
@PickBas thanks for working on this. I think it would be great to combine all changes in a single PR rather than opening 1-file PRs |
@nastra Thank you for your feedback. I've done 1 PR per module so we could keep the changes traceable and manageable, as @Fokko requested in PR #8813. I may do it another way: keep PRs open for spark and core modules, since they include most of the changes, and combine the rest of the modules into a single PR. If that works for both you and @Fokko, I will surely do that. |
There was a problem hiding this comment.
Thanks @PickBas!
I may do it another way: keep PRs open for spark and core modules, since they include most of the changes, and combine the rest of the modules into a single PR.
Yeah l like this. Some of the changes for the modules are smaller than others, we can combine the small ones together and leave the bigger ones like Spark/Core in separate PRs. IMO it's just a balance between looking at Git history and then also easier PR review. With many smaller change PRs the commit history becomes less meaningful, but with too big PRs it becomes hard to review and easy to miss things
[ISSUE apache#8810] data/*: replaced .size() > 0 with isEmpty()
[ISSUE apache#8810] parquet/*: replaced .size() > 0 with isEmpty()
Fix 8810 aws
[ISSUE apache#8810] api/*: replaced .size() > 0 with isEmpty()
[ISSUE apache#8810] hive3/*: replaced .size() > 0 with isEmpty()
[ISSUE apache#8810] delta-lake/*: replaced .size() > 0 with isEmpty()
[ISSUE apache#8810] mr/*: replaced .size() > 0 with isEmpty()
[ISSUE apache#8810] aliyun/*: replaced .size() > 0 with isEmpty()
PickBas
changed the title
|
@amogh-jahagirdar @nastra @Fokko Done. Now the rest of the modules are included in this pull request. |
I'm open to everything. |
| @@ -105,7 +105,7 @@ public void testNamespaceExists() { | |||
| public void testListNamespace() { | |||
| String namespace = createNamespace(); | |||
| List<Namespace> namespaceList = glueCatalog.listNamespaces(); | |||
| Assert.assertTrue(namespaceList.size() > 0); | |||
| Assert.assertFalse(namespaceList.isEmpty()); | |||
There was a problem hiding this comment.
nit: we might want to switch all Junit4-style assertions to AssertJ in the AWS integration test module (in a separate PR)
|
Thanks @PickBas for picking this up 🙌 and @nastra and @ajantha-bhat for the review 👍 |
From issue #8810.