You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ajantha-bhat (via GitHub)" <gi...@apache.org> on 2025/02/11 18:25:26 UTC
[PR] Core: Make totalRecordCount optional in PartitionStats [iceberg]
ajantha-bhat opened a new pull request, #12226:
URL: https://github.com/apache/iceberg/pull/12226
Spec was already optional for totalRecordCount.
https://iceberg.apache.org/spec/#partition-statistics-file
During the implementation, we decided to make all the counters to be initialized to zero.
Recently, we concluded that total record count counter can still be optional.
Based on the discussions from https://github.com/apache/iceberg/pull/12098#discussion_r1940551771
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r2002513794
##########
core/src/main/java/org/apache/iceberg/PartitionStats.java:
##########
@@ -78,7 +78,15 @@ public int equalityDeleteFileCount() {
return equalityDeleteFileCount;
}
+ /**
+ * @deprecated since 1.9.0, will be removed in 1.10.0, use {@link #totalRecords()} instead.
+ */
+ @Deprecated
public long totalRecordCount() {
+ return totalRecordCount == null ? 0 : totalRecordCount;
Review Comment:
```suggestion
return totalRecordCount == null ? 0L : totalRecordCount;
```
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r2000747104
##########
core/src/main/java/org/apache/iceberg/PartitionStats.java:
##########
@@ -78,7 +78,16 @@ public int equalityDeleteFileCount() {
return equalityDeleteFileCount;
}
+ /**
+ * @deprecated since 1.9.0, will be removed in 1.10.0, use {@link #totalRecordCountOptional()}
+ * instead.
+ */
+ @Deprecated
public long totalRecordCount() {
+ return totalRecordCount == null ? 0 : totalRecordCount;
+ }
+
+ public Long totalRecordCountOptional() {
Review Comment:
I like it. Updated.
Also, after rebased and observed test failure due to partition stats writer code. Handled them also.
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r2000699545
##########
core/src/main/java/org/apache/iceberg/PartitionStats.java:
##########
@@ -78,7 +78,16 @@ public int equalityDeleteFileCount() {
return equalityDeleteFileCount;
}
+ /**
+ * @deprecated since 1.9.0, will be removed in 1.10.0, use {@link #totalRecordCountOptional()}
+ * instead.
+ */
+ @Deprecated
public long totalRecordCount() {
+ return totalRecordCount == null ? 0 : totalRecordCount;
+ }
+
+ public Long totalRecordCountOptional() {
Review Comment:
maybe `totalRecords()`?
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r2002428277
##########
data/src/test/java/org/apache/iceberg/data/TestPartitionStatsHandler.java:
##########
@@ -591,7 +591,7 @@ private static boolean isEqual(
&& stats1.positionDeleteFileCount() == stats2.positionDeleteFileCount()
&& stats1.equalityDeleteRecordCount() == stats2.equalityDeleteRecordCount()
&& stats1.equalityDeleteFileCount() == stats2.equalityDeleteFileCount()
- && stats1.totalRecordCount() == stats2.totalRecordCount()
+ && Objects.equals(stats1.totalRecords(), stats2.totalRecords())
Review Comment:
`PartitionStats` has `StructLike` member for `partition`, it needs a comparator for equals(). But storing comparator for each `PartitionStats` is not efficient. Plus we also want to keep the members of this class same as partition stats spec.
It was discussed in one of the old 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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#issuecomment-2703264435
@aokolnychyi @Fokko wdyt ?
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#issuecomment-2728638740
@nastra, @pvary: I need your help to merge this. I am planning for 1.9.0 release this week. Will start a mailing list discussion tomorrow.
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #12226:
URL: https://github.com/apache/iceberg/pull/12226
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r2000690777
##########
core/src/main/java/org/apache/iceberg/PartitionStats.java:
##########
@@ -78,7 +78,16 @@ public int equalityDeleteFileCount() {
return equalityDeleteFileCount;
}
+ /**
+ * @deprecated since 1.9.0, will be removed in 1.10.0, use {@link #totalRecordCountOptional()}
+ * instead.
+ */
+ @Deprecated
Review Comment:
do we really need to deprecate the existing method and introduce a new one to handle this properly?
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r2000744858
##########
core/src/main/java/org/apache/iceberg/PartitionStats.java:
##########
@@ -78,7 +78,16 @@ public int equalityDeleteFileCount() {
return equalityDeleteFileCount;
}
+ /**
+ * @deprecated since 1.9.0, will be removed in 1.10.0, use {@link #totalRecordCountOptional()}
+ * instead.
+ */
+ @Deprecated
Review Comment:
> method's return type stays the same
We wanted to return null when not computed as per the discussion. We can't do that if the return type is primitive.
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r2000988187
##########
core/src/main/java/org/apache/iceberg/PartitionStats.java:
##########
@@ -150,7 +158,12 @@ public void appendStats(PartitionStats entry) {
this.positionDeleteFileCount += entry.positionDeleteFileCount;
this.equalityDeleteRecordCount += entry.equalityDeleteRecordCount;
this.equalityDeleteFileCount += entry.equalityDeleteFileCount;
- this.totalRecordCount += entry.totalRecordCount;
+
+ if (totalRecordCount == null) {
+ this.totalRecordCount = entry.totalRecordCount;
+ } else {
+ this.totalRecordCount += entry.totalRecordCount;
Review Comment:
Added unit tests.
##########
core/src/test/java/org/apache/iceberg/TestPartitionStatsUtil.java:
##########
@@ -103,7 +103,7 @@ public void testPartitionStats() throws Exception {
0,
0L,
0,
- 0L,
+ null,
Review Comment:
Added the unit 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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r1998445332
##########
core/src/test/java/org/apache/iceberg/TestPartitionStatsUtil.java:
##########
@@ -103,7 +103,7 @@ public void testPartitionStats() throws Exception {
0,
0L,
0,
- 0L,
+ null,
Review Comment:
The current implementation doesn't compute it. Hence, it is always null
https://github.com/apache/iceberg/blob/57ec405a651b99d5fce3f3b4bec217d24bc98d20/core/src/main/java/org/apache/iceberg/PartitionStats.java#L124
Do you mean have a mock test, that sets `PartitionStats` with some number for `total_record_count` (using StructLike's `set(pos, value)` and call `get(pos, value)`? I think it is an overkill.
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r2000690777
##########
core/src/main/java/org/apache/iceberg/PartitionStats.java:
##########
@@ -78,7 +78,16 @@ public int equalityDeleteFileCount() {
return equalityDeleteFileCount;
}
+ /**
+ * @deprecated since 1.9.0, will be removed in 1.10.0, use {@link #totalRecordCountOptional()}
+ * instead.
+ */
+ @Deprecated
Review Comment:
do we really need to deprecate the existing method and introduce a new one to handle this properly? I'm not a fan of having a method with `optional` in the name. Also what if the field itself becomes a `Long` but the method's return type stays the same?
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#issuecomment-2706657194
We have already finalized on this change by discussing on https://github.com/apache/iceberg/pull/12098#discussion_r1940551771
and https://lists.apache.org/thread/1dzcv7989q0hchjytyb8g219wo7nc1xm
But looks like Anton is not active. Anyone else can help in merging this?
cc: @RussellSpitzer, @nastra
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r2001508692
##########
data/src/test/java/org/apache/iceberg/data/TestPartitionStatsHandler.java:
##########
@@ -591,7 +591,7 @@ private static boolean isEqual(
&& stats1.positionDeleteFileCount() == stats2.positionDeleteFileCount()
&& stats1.equalityDeleteRecordCount() == stats2.equalityDeleteRecordCount()
&& stats1.equalityDeleteFileCount() == stats2.equalityDeleteFileCount()
- && stats1.totalRecordCount() == stats2.totalRecordCount()
+ && Objects.equals(stats1.totalRecords(), stats2.totalRecords())
Review Comment:
not directly related to this PR, but why can't we handle equals/hashcode in `PartitionStats` directly?
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r2002425079
##########
core/src/test/java/org/apache/iceberg/TestPartitionStats.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+
+public class TestPartitionStats {
+
+ private static final PartitionData PARTITION =
+ new PartitionData(
+ Types.StructType.of(Types.NestedField.required(1, "foo", Types.IntegerType.get())));
+
+ @Test
+ public void testAppendWithAllValues() {
+ PartitionStats stats1 =
+ createStats(100L, 15, 1000L, 2L, 500, 1L, 200, 15L, 1625077800000L, 12345L);
+ PartitionStats stats2 = createStats(200L, 7, 500L, 1L, 100, 0L, 50, 7L, 1625077900000L, 12346L);
+
+ stats1.appendStats(stats2);
+
+ validateStats(stats1, 300L, 22, 1500L, 3L, 600, 1L, 250, 22L, 1625077900000L, 12346L);
+ }
+
+ @Test
+ public void testAppendWithThisNullOptionalField() {
+ PartitionStats stats1 = createStats(100L, 15, 1000L, 2L, 500, 1L, 200, null, null, null);
+ PartitionStats stats2 = createStats(100L, 7, 500L, 1L, 100, 0L, 50, 7L, 1625077900000L, 12346L);
+
+ stats1.appendStats(stats2);
+
+ validateStats(stats1, 200L, 22, 1500L, 3L, 600, 1L, 250, 7L, 1625077900000L, 12346L);
+ }
+
+ @Test
+ public void testAppendWithBothNullOptionalFields() {
+ PartitionStats stats1 = createStats(100L, 15, 1000L, 2L, 500, 1L, 200, null, null, null);
+ PartitionStats stats2 = createStats(100L, 7, 500L, 1L, 100, 0L, 50, null, null, null);
+
+ stats1.appendStats(stats2);
+
+ validateStats(stats1, 200L, 22, 1500L, 3L, 600, 1L, 250, null, null, null);
+ }
+
+ @Test
+ public void testAppendWithOtherNullOptionalFields() {
+ PartitionStats stats1 =
+ createStats(100L, 15, 1000L, 2L, 500, 1L, 200, 15L, 1625077900000L, 12346L);
+ PartitionStats stats2 = createStats(100L, 7, 500L, 1L, 100, 0L, 50, null, null, null);
+
+ stats1.appendStats(stats2);
+
+ validateStats(stats1, 200L, 22, 1500L, 3L, 600, 1L, 250, 15L, 1625077900000L, 12346L);
+ }
+
+ @Test
+ public void testAppendWithDifferentSpec() {
+ PartitionStats stats1 = new PartitionStats(PARTITION, 1);
+ PartitionStats stats2 = new PartitionStats(PARTITION, 2);
+
+ assertThatThrownBy(() -> stats1.appendStats(stats2))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Spec IDs must match");
+ }
+
+ private PartitionStats createStats(
+ long dataRecordCount,
+ int dataFileCount,
+ long totalDataFileSizeInBytes,
+ long positionDeleteRecordCount,
+ int positionDeleteFileCount,
+ long equalityDeleteRecordCount,
+ int equalityDeleteFileCount,
+ Long totalRecordCount,
+ Long lastUpdatedAt,
+ Long lastUpdatedSnapshotId) {
+
+ PartitionStats stats = new PartitionStats(PARTITION, 1);
Review Comment:
Added
https://github.com/apache/iceberg/compare/e6c50140f8f915778c8cd120f81a40d643f72c2e..e4a60331b04ed2eabfe09b763387bdf67ff7021f
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r2000792522
##########
core/src/main/java/org/apache/iceberg/PartitionStats.java:
##########
@@ -150,7 +158,12 @@ public void appendStats(PartitionStats entry) {
this.positionDeleteFileCount += entry.positionDeleteFileCount;
this.equalityDeleteRecordCount += entry.equalityDeleteRecordCount;
this.equalityDeleteFileCount += entry.equalityDeleteFileCount;
- this.totalRecordCount += entry.totalRecordCount;
+
+ if (totalRecordCount == null) {
+ this.totalRecordCount = entry.totalRecordCount;
+ } else {
+ this.totalRecordCount += entry.totalRecordCount;
Review Comment:
what if `entry.totalRecordCount` is `null`?
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r1998298603
##########
core/src/test/java/org/apache/iceberg/TestPartitionStatsUtil.java:
##########
@@ -103,7 +103,7 @@ public void testPartitionStats() throws Exception {
0,
0L,
0,
- 0L,
+ null,
Review Comment:
Do we have test where this is actually not null? And maybe a valid 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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r2000708040
##########
core/src/main/java/org/apache/iceberg/PartitionStats.java:
##########
@@ -78,7 +78,16 @@ public int equalityDeleteFileCount() {
return equalityDeleteFileCount;
}
+ /**
+ * @deprecated since 1.9.0, will be removed in 1.10.0, use {@link #totalRecordCountOptional()}
+ * instead.
+ */
+ @Deprecated
Review Comment:
Maybe no one using it as the writer code is not present in previous version.
But just following the [guidelines](https://iceberg.apache.org/contribute/?h=contri#minor-version-deprecations-required) as it is a public interface and introduced in previous version.
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r2000801634
##########
core/src/main/java/org/apache/iceberg/PartitionStats.java:
##########
@@ -150,7 +158,12 @@ public void appendStats(PartitionStats entry) {
this.positionDeleteFileCount += entry.positionDeleteFileCount;
this.equalityDeleteRecordCount += entry.equalityDeleteRecordCount;
this.equalityDeleteFileCount += entry.equalityDeleteFileCount;
- this.totalRecordCount += entry.totalRecordCount;
+
+ if (totalRecordCount == null) {
+ this.totalRecordCount = entry.totalRecordCount;
+ } else {
+ this.totalRecordCount += entry.totalRecordCount;
Review Comment:
I feel like there should be some tests where `totalRecordCount` is null/non-null to make sure this is all properly handled
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r2001521866
##########
core/src/test/java/org/apache/iceberg/TestPartitionStats.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+
+public class TestPartitionStats {
+
+ private static final PartitionData PARTITION =
+ new PartitionData(
+ Types.StructType.of(Types.NestedField.required(1, "foo", Types.IntegerType.get())));
+
+ @Test
+ public void testAppendWithAllValues() {
+ PartitionStats stats1 =
+ createStats(100L, 15, 1000L, 2L, 500, 1L, 200, 15L, 1625077800000L, 12345L);
+ PartitionStats stats2 = createStats(200L, 7, 500L, 1L, 100, 0L, 50, 7L, 1625077900000L, 12346L);
+
+ stats1.appendStats(stats2);
+
+ validateStats(stats1, 300L, 22, 1500L, 3L, 600, 1L, 250, 22L, 1625077900000L, 12346L);
+ }
+
+ @Test
+ public void testAppendWithThisNullOptionalField() {
+ PartitionStats stats1 = createStats(100L, 15, 1000L, 2L, 500, 1L, 200, null, null, null);
+ PartitionStats stats2 = createStats(100L, 7, 500L, 1L, 100, 0L, 50, 7L, 1625077900000L, 12346L);
+
+ stats1.appendStats(stats2);
+
+ validateStats(stats1, 200L, 22, 1500L, 3L, 600, 1L, 250, 7L, 1625077900000L, 12346L);
+ }
+
+ @Test
+ public void testAppendWithBothNullOptionalFields() {
+ PartitionStats stats1 = createStats(100L, 15, 1000L, 2L, 500, 1L, 200, null, null, null);
+ PartitionStats stats2 = createStats(100L, 7, 500L, 1L, 100, 0L, 50, null, null, null);
+
+ stats1.appendStats(stats2);
+
+ validateStats(stats1, 200L, 22, 1500L, 3L, 600, 1L, 250, null, null, null);
+ }
+
+ @Test
+ public void testAppendWithOtherNullOptionalFields() {
+ PartitionStats stats1 =
+ createStats(100L, 15, 1000L, 2L, 500, 1L, 200, 15L, 1625077900000L, 12346L);
+ PartitionStats stats2 = createStats(100L, 7, 500L, 1L, 100, 0L, 50, null, null, null);
+
+ stats1.appendStats(stats2);
+
+ validateStats(stats1, 200L, 22, 1500L, 3L, 600, 1L, 250, 15L, 1625077900000L, 12346L);
+ }
+
+ @Test
+ public void testAppendWithDifferentSpec() {
+ PartitionStats stats1 = new PartitionStats(PARTITION, 1);
+ PartitionStats stats2 = new PartitionStats(PARTITION, 2);
+
+ assertThatThrownBy(() -> stats1.appendStats(stats2))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Spec IDs must match");
+ }
+
+ private PartitionStats createStats(
+ long dataRecordCount,
+ int dataFileCount,
+ long totalDataFileSizeInBytes,
+ long positionDeleteRecordCount,
+ int positionDeleteFileCount,
+ long equalityDeleteRecordCount,
+ int equalityDeleteFileCount,
+ Long totalRecordCount,
+ Long lastUpdatedAt,
+ Long lastUpdatedSnapshotId) {
+
+ PartitionStats stats = new PartitionStats(PARTITION, 1);
Review Comment:
I would also add a test where for both stats objects only the partition and spec id is set
--
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: Make totalRecordCount optional in PartitionStats [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12226:
URL: https://github.com/apache/iceberg/pull/12226#discussion_r2000690777
##########
core/src/main/java/org/apache/iceberg/PartitionStats.java:
##########
@@ -78,7 +78,16 @@ public int equalityDeleteFileCount() {
return equalityDeleteFileCount;
}
+ /**
+ * @deprecated since 1.9.0, will be removed in 1.10.0, use {@link #totalRecordCountOptional()}
+ * instead.
+ */
+ @Deprecated
Review Comment:
do we really need to deprecate the existing method and introduce a new one to handle this properly? I'm not a fan of having a method with `optional` in the name
--
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