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/06/17 07:23:31 UTC
[PR] Core: Fix schema id of partition stats file [iceberg]
ajantha-bhat opened a new pull request, #13329:
URL: https://github.com/apache/iceberg/pull/13329
(no 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: Fix filed ids of partition stats file [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2151637230
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -65,29 +65,44 @@ private PartitionStatsHandler() {}
private static final Logger LOG = LoggerFactory.getLogger(PartitionStatsHandler.class);
- public static final int PARTITION_FIELD_ID = 0;
+ // schema of the partition stats file as per spec
+ public static final int PARTITION_FIELD_ID = 1;
Review Comment:
These are binary breaking changes in 1.9.2 compared to 1.9.0.
What is our policy here?
Do we announce that the 1.9.0 partition stats implementation is broken, and we don't support it?
In this case we might be ok to do incompatible changes (making it compatible with the spec)
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2151620580
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -239,30 +263,30 @@ private static OutputFile newPartitionStatsFile(
private static PartitionStats recordToPartitionStats(StructLike record) {
PartitionStats stats =
new PartitionStats(
- record.get(PARTITION_FIELD_ID, StructLike.class),
- record.get(SPEC_ID.fieldId(), Integer.class));
- stats.set(DATA_RECORD_COUNT.fieldId(), record.get(DATA_RECORD_COUNT.fieldId(), Long.class));
- stats.set(DATA_FILE_COUNT.fieldId(), record.get(DATA_FILE_COUNT.fieldId(), Integer.class));
+ record.get(PARTITION_POSITION, StructLike.class),
+ record.get(SPEC_ID_POSITION, Integer.class));
+ stats.set(DATA_RECORD_COUNT_POSITION, record.get(DATA_RECORD_COUNT_POSITION, Long.class));
+ stats.set(DATA_FILE_COUNT_POSITION, record.get(DATA_FILE_COUNT_POSITION, Integer.class));
stats.set(
- TOTAL_DATA_FILE_SIZE_IN_BYTES.fieldId(),
- record.get(TOTAL_DATA_FILE_SIZE_IN_BYTES.fieldId(), Long.class));
+ TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION,
+ record.get(TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION, Long.class));
stats.set(
- POSITION_DELETE_RECORD_COUNT.fieldId(),
- record.get(POSITION_DELETE_RECORD_COUNT.fieldId(), Long.class));
+ POSITION_DELETE_RECORD_COUNT_POSITION,
+ record.get(POSITION_DELETE_RECORD_COUNT_POSITION, Long.class));
stats.set(
- POSITION_DELETE_FILE_COUNT.fieldId(),
- record.get(POSITION_DELETE_FILE_COUNT.fieldId(), Integer.class));
+ POSITION_DELETE_FILE_COUNT_POSITION,
+ record.get(POSITION_DELETE_FILE_COUNT_POSITION, Integer.class));
stats.set(
- EQUALITY_DELETE_RECORD_COUNT.fieldId(),
- record.get(EQUALITY_DELETE_RECORD_COUNT.fieldId(), Long.class));
+ EQUALITY_DELETE_RECORD_COUNT_POSITION,
+ record.get(EQUALITY_DELETE_RECORD_COUNT_POSITION, Long.class));
stats.set(
- EQUALITY_DELETE_FILE_COUNT.fieldId(),
- record.get(EQUALITY_DELETE_FILE_COUNT.fieldId(), Integer.class));
- stats.set(TOTAL_RECORD_COUNT.fieldId(), record.get(TOTAL_RECORD_COUNT.fieldId(), Long.class));
- stats.set(LAST_UPDATED_AT.fieldId(), record.get(LAST_UPDATED_AT.fieldId(), Long.class));
+ EQUALITY_DELETE_FILE_COUNT_POSITION,
+ record.get(EQUALITY_DELETE_FILE_COUNT_POSITION, Integer.class));
+ stats.set(TOTAL_RECORD_COUNT_POSITION, record.get(TOTAL_RECORD_COUNT_POSITION, Long.class));
+ stats.set(LAST_UPDATED_AT_POSITION, record.get(LAST_UPDATED_AT_POSITION, Long.class));
stats.set(
- LAST_UPDATED_SNAPSHOT_ID.fieldId(),
- record.get(LAST_UPDATED_SNAPSHOT_ID.fieldId(), Long.class));
+ LAST_UPDATED_SNAPSHOT_ID_POSITION,
+ record.get(LAST_UPDATED_SNAPSHOT_ID_POSITION, Long.class));
Review Comment:
Could we just do this?
```
PartitionStats stats =
new PartitionStats(
record.get(PARTITION_POSITION, StructLike.class),
record.get(SPEC_ID_POSITION, Integer.class));
for(int i = 0; i<record.size(); ++i) {
stats.set(i, record.get(i, Object.class));
}
```
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#issuecomment-2980438943
Thanks @ajantha-bhat for the heads up, we've just upgraded the Hive to Iceberg 1.9.1
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2158245627
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
Maybe we can have a wrapper like this and let the returned Iterable return this wrapper?
```java
public static class CatchingWrapper implements CloseableIterator<PartitionStats> {
private final CloseableIterator<PartitionStats> delegate;
public CatchingWrapper(CloseableIterator<PartitionStats> delegate) {
this.delegate = delegate;
}
@Override
public boolean hasNext() {
try {
return delegate.hasNext();
} catch (IllegalArgumentException | IllegalStateException e) {
if (isInvalidStats(e)) {
throw new InvalidStatsFileException();
}
throw e;
}
}
@Override
public PartitionStats next() {
try {
return delegate.next();
} catch (IllegalArgumentException | IllegalStateException e) {
if (isInvalidStats(e)) {
throw new InvalidStatsFileException();
}
throw e;
}
}
}
```
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2151926459
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -239,30 +263,30 @@ private static OutputFile newPartitionStatsFile(
private static PartitionStats recordToPartitionStats(StructLike record) {
PartitionStats stats =
new PartitionStats(
- record.get(PARTITION_FIELD_ID, StructLike.class),
- record.get(SPEC_ID.fieldId(), Integer.class));
- stats.set(DATA_RECORD_COUNT.fieldId(), record.get(DATA_RECORD_COUNT.fieldId(), Long.class));
- stats.set(DATA_FILE_COUNT.fieldId(), record.get(DATA_FILE_COUNT.fieldId(), Integer.class));
+ record.get(PARTITION_POSITION, StructLike.class),
+ record.get(SPEC_ID_POSITION, Integer.class));
+ stats.set(DATA_RECORD_COUNT_POSITION, record.get(DATA_RECORD_COUNT_POSITION, Long.class));
+ stats.set(DATA_FILE_COUNT_POSITION, record.get(DATA_FILE_COUNT_POSITION, Integer.class));
stats.set(
- TOTAL_DATA_FILE_SIZE_IN_BYTES.fieldId(),
- record.get(TOTAL_DATA_FILE_SIZE_IN_BYTES.fieldId(), Long.class));
+ TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION,
+ record.get(TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION, Long.class));
stats.set(
- POSITION_DELETE_RECORD_COUNT.fieldId(),
- record.get(POSITION_DELETE_RECORD_COUNT.fieldId(), Long.class));
+ POSITION_DELETE_RECORD_COUNT_POSITION,
+ record.get(POSITION_DELETE_RECORD_COUNT_POSITION, Long.class));
stats.set(
- POSITION_DELETE_FILE_COUNT.fieldId(),
- record.get(POSITION_DELETE_FILE_COUNT.fieldId(), Integer.class));
+ POSITION_DELETE_FILE_COUNT_POSITION,
+ record.get(POSITION_DELETE_FILE_COUNT_POSITION, Integer.class));
stats.set(
- EQUALITY_DELETE_RECORD_COUNT.fieldId(),
- record.get(EQUALITY_DELETE_RECORD_COUNT.fieldId(), Long.class));
+ EQUALITY_DELETE_RECORD_COUNT_POSITION,
+ record.get(EQUALITY_DELETE_RECORD_COUNT_POSITION, Long.class));
stats.set(
- EQUALITY_DELETE_FILE_COUNT.fieldId(),
- record.get(EQUALITY_DELETE_FILE_COUNT.fieldId(), Integer.class));
- stats.set(TOTAL_RECORD_COUNT.fieldId(), record.get(TOTAL_RECORD_COUNT.fieldId(), Long.class));
- stats.set(LAST_UPDATED_AT.fieldId(), record.get(LAST_UPDATED_AT.fieldId(), Long.class));
+ EQUALITY_DELETE_FILE_COUNT_POSITION,
+ record.get(EQUALITY_DELETE_FILE_COUNT_POSITION, Integer.class));
+ stats.set(TOTAL_RECORD_COUNT_POSITION, record.get(TOTAL_RECORD_COUNT_POSITION, Long.class));
+ stats.set(LAST_UPDATED_AT_POSITION, record.get(LAST_UPDATED_AT_POSITION, Long.class));
stats.set(
- LAST_UPDATED_SNAPSHOT_ID.fieldId(),
- record.get(LAST_UPDATED_SNAPSHOT_ID.fieldId(), Long.class));
+ LAST_UPDATED_SNAPSHOT_ID_POSITION,
+ record.get(LAST_UPDATED_SNAPSHOT_ID_POSITION, Long.class));
Review Comment:
Since, both are same datatype, it should be possible. Let me check.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2152362678
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -239,30 +263,30 @@ private static OutputFile newPartitionStatsFile(
private static PartitionStats recordToPartitionStats(StructLike record) {
PartitionStats stats =
new PartitionStats(
- record.get(PARTITION_FIELD_ID, StructLike.class),
- record.get(SPEC_ID.fieldId(), Integer.class));
- stats.set(DATA_RECORD_COUNT.fieldId(), record.get(DATA_RECORD_COUNT.fieldId(), Long.class));
- stats.set(DATA_FILE_COUNT.fieldId(), record.get(DATA_FILE_COUNT.fieldId(), Integer.class));
+ record.get(PARTITION_POSITION, StructLike.class),
+ record.get(SPEC_ID_POSITION, Integer.class));
+ stats.set(DATA_RECORD_COUNT_POSITION, record.get(DATA_RECORD_COUNT_POSITION, Long.class));
+ stats.set(DATA_FILE_COUNT_POSITION, record.get(DATA_FILE_COUNT_POSITION, Integer.class));
stats.set(
- TOTAL_DATA_FILE_SIZE_IN_BYTES.fieldId(),
- record.get(TOTAL_DATA_FILE_SIZE_IN_BYTES.fieldId(), Long.class));
+ TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION,
+ record.get(TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION, Long.class));
stats.set(
- POSITION_DELETE_RECORD_COUNT.fieldId(),
- record.get(POSITION_DELETE_RECORD_COUNT.fieldId(), Long.class));
+ POSITION_DELETE_RECORD_COUNT_POSITION,
+ record.get(POSITION_DELETE_RECORD_COUNT_POSITION, Long.class));
stats.set(
- POSITION_DELETE_FILE_COUNT.fieldId(),
- record.get(POSITION_DELETE_FILE_COUNT.fieldId(), Integer.class));
+ POSITION_DELETE_FILE_COUNT_POSITION,
+ record.get(POSITION_DELETE_FILE_COUNT_POSITION, Integer.class));
stats.set(
- EQUALITY_DELETE_RECORD_COUNT.fieldId(),
- record.get(EQUALITY_DELETE_RECORD_COUNT.fieldId(), Long.class));
+ EQUALITY_DELETE_RECORD_COUNT_POSITION,
+ record.get(EQUALITY_DELETE_RECORD_COUNT_POSITION, Long.class));
stats.set(
- EQUALITY_DELETE_FILE_COUNT.fieldId(),
- record.get(EQUALITY_DELETE_FILE_COUNT.fieldId(), Integer.class));
- stats.set(TOTAL_RECORD_COUNT.fieldId(), record.get(TOTAL_RECORD_COUNT.fieldId(), Long.class));
- stats.set(LAST_UPDATED_AT.fieldId(), record.get(LAST_UPDATED_AT.fieldId(), Long.class));
+ EQUALITY_DELETE_FILE_COUNT_POSITION,
+ record.get(EQUALITY_DELETE_FILE_COUNT_POSITION, Integer.class));
+ stats.set(TOTAL_RECORD_COUNT_POSITION, record.get(TOTAL_RECORD_COUNT_POSITION, Long.class));
+ stats.set(LAST_UPDATED_AT_POSITION, record.get(LAST_UPDATED_AT_POSITION, Long.class));
stats.set(
- LAST_UPDATED_SNAPSHOT_ID.fieldId(),
- record.get(LAST_UPDATED_SNAPSHOT_ID.fieldId(), Long.class));
+ LAST_UPDATED_SNAPSHOT_ID_POSITION,
+ record.get(LAST_UPDATED_SNAPSHOT_ID_POSITION, Long.class));
Review Comment:
`SPEC_ID_POSITION + 1`
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2158168420
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +304,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
Can I just catch `IOException` and `RuntimeException` here? As all the error file handling errors falls into this category?
I saw that many places, iceberg just catches whole exception `catch (Exception`
I will go with that.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2157445956
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +304,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
What happens, if we have a temporary file access issue here, and not able to read the old stat file?
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2151646228
##########
parquet/src/test/java/org/apache/iceberg/TestParquetPartitionStatsHandler.java:
##########
@@ -18,9 +18,36 @@
*/
package org.apache.iceberg;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.jupiter.api.Test;
+
public class TestParquetPartitionStatsHandler extends PartitionStatsHandlerTestBase {
public FileFormat format() {
return FileFormat.PARQUET;
}
+
+ @Test
+ public void testReadingStatsWithInvalidSchema() throws Exception {
+ Table testTable =
+ TestTables.create(tempDir("old_schema"), "old_schema", SCHEMA, SPEC, 2, fileFormatProperty);
+ Schema schema = PartitionStatsHandler.schema(Partitioning.partitionType(testTable));
+
+ String invalidSchema =
+ getClass()
+ .getClassLoader()
+ .getResource("org/apache/iceberg/PartitionStatsInvalidSchema.parquet")
Review Comment:
Could we just generate the file on the fly instead of checking in the binary?
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2165890885
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
I see, so it helps ignore all exceptions when reading the old stats, but not when doing the compute. +1 to the current change.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2157959956
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
In that case, is it possible to wrap this logic in the iterable returned by readPartitionStatsFile, so that callers don't have to check exceptions themselves? Besides, IllegalArgumentException and IllegalStateException seem too common to me. Perhaps we should further check the exception messages, or even check the field IDs in the file, to make sure exceptions are really caused by the old schema?
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2163628462
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
@lirui-apache: Let me know your final thoughts on this. If you still think wrapping the `IllegalArgumentException`, `IllegalStateException` to `InvalidStatsFileException` is useful for the users I can wrap it. But I feel users can figure out it is invalid based on callstack.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2151951117
##########
parquet/src/test/java/org/apache/iceberg/TestParquetPartitionStatsHandler.java:
##########
@@ -18,9 +18,36 @@
*/
package org.apache.iceberg;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.jupiter.api.Test;
+
public class TestParquetPartitionStatsHandler extends PartitionStatsHandlerTestBase {
public FileFormat format() {
return FileFormat.PARQUET;
}
+
+ @Test
+ public void testReadingStatsWithInvalidSchema() throws Exception {
+ Table testTable =
+ TestTables.create(tempDir("old_schema"), "old_schema", SCHEMA, SPEC, 2, fileFormatProperty);
+ Schema schema = PartitionStatsHandler.schema(Partitioning.partitionType(testTable));
+
+ String invalidSchema =
+ getClass()
+ .getClassLoader()
+ .getResource("org/apache/iceberg/PartitionStatsInvalidSchema.parquet")
Review Comment:
We don't have old schema in code now.
So, creating a parquet file as per old schema, adding rows etc will be a big chunk of code.
I observed that we do maintain puffin files, delete files and metadata files in resource folder
https://github.com/apache/iceberg/tree/main/core/src/test/resources/org/apache/iceberg
Hence, I kept it to reduce PR code size.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#issuecomment-2983541535
> > off-topic: partition stats file format is highly coupled with table write.format. If CU is using ORC, he automatically loses the partition stats.
>
> Yes. In the initial proposal document jack-ye has brought this up. Spec was hardcoded that it should be in table default format. Then we updated that it can be any of the format (https://iceberg.apache.org/spec/#partition-statistics-file).
>
> So, we can have a table property to configure a different format stats than data format. But I recommend supporting InternalData for ORC, I can help on it too if needed (I did it for parquet and avro). So, that ORC can be used for writing table metadata in v4. Than changing the format of partition stats here.
@ajantha-bhat, that would be awesome if we add an InternalData reader/writer for ORC. However, I am not sure how big that effort would be since we'll need to implement support for a few missing features like default values, timestamp(9), variant
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2151951117
##########
parquet/src/test/java/org/apache/iceberg/TestParquetPartitionStatsHandler.java:
##########
@@ -18,9 +18,36 @@
*/
package org.apache.iceberg;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.jupiter.api.Test;
+
public class TestParquetPartitionStatsHandler extends PartitionStatsHandlerTestBase {
public FileFormat format() {
return FileFormat.PARQUET;
}
+
+ @Test
+ public void testReadingStatsWithInvalidSchema() throws Exception {
+ Table testTable =
+ TestTables.create(tempDir("old_schema"), "old_schema", SCHEMA, SPEC, 2, fileFormatProperty);
+ Schema schema = PartitionStatsHandler.schema(Partitioning.partitionType(testTable));
+
+ String invalidSchema =
+ getClass()
+ .getClassLoader()
+ .getResource("org/apache/iceberg/PartitionStatsInvalidSchema.parquet")
Review Comment:
We don't have old schema in code now.
So, creating a parquet file as per old schema, adding rows etc will be a big chunk of code.
I observed that we do maintain puffin files, delete files and metadata files in resource folder
https://github.com/apache/iceberg/tree/main/core/src/test/resources/org/apache/iceberg
Hence, I kept it to avoid PR code size.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2155128558
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -263,30 +249,12 @@ private static OutputFile newPartitionStatsFile(
private static PartitionStats recordToPartitionStats(StructLike record) {
PartitionStats stats =
new PartitionStats(
- record.get(PARTITION_POSITION, StructLike.class),
- record.get(SPEC_ID_POSITION, Integer.class));
- stats.set(DATA_RECORD_COUNT_POSITION, record.get(DATA_RECORD_COUNT_POSITION, Long.class));
- stats.set(DATA_FILE_COUNT_POSITION, record.get(DATA_FILE_COUNT_POSITION, Integer.class));
- stats.set(
- TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION,
- record.get(TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION, Long.class));
- stats.set(
- POSITION_DELETE_RECORD_COUNT_POSITION,
- record.get(POSITION_DELETE_RECORD_COUNT_POSITION, Long.class));
- stats.set(
- POSITION_DELETE_FILE_COUNT_POSITION,
- record.get(POSITION_DELETE_FILE_COUNT_POSITION, Integer.class));
- stats.set(
- EQUALITY_DELETE_RECORD_COUNT_POSITION,
- record.get(EQUALITY_DELETE_RECORD_COUNT_POSITION, Long.class));
- stats.set(
- EQUALITY_DELETE_FILE_COUNT_POSITION,
- record.get(EQUALITY_DELETE_FILE_COUNT_POSITION, Integer.class));
- stats.set(TOTAL_RECORD_COUNT_POSITION, record.get(TOTAL_RECORD_COUNT_POSITION, Long.class));
- stats.set(LAST_UPDATED_AT_POSITION, record.get(LAST_UPDATED_AT_POSITION, Long.class));
- stats.set(
- LAST_UPDATED_SNAPSHOT_ID_POSITION,
- record.get(LAST_UPDATED_SNAPSHOT_ID_POSITION, Long.class));
+ record.get(0, StructLike.class), // partition
Review Comment:
done
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2161376184
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
I checked and `hasNext()` didn't throw exception. Only accessing element with `next()` has thrown exception.
So, in my opinion. Current changes are enough as we are treating any exception as `InvalidStatsFileException` in the place where we are accessing this iterator.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2165518201
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
@pvary @ajantha-bhat We haven't released the refactor causing the issue in our internal production so this won't affect our users.
But I wonder what our messages will be to users of the `readPartitionStatsFile` API in 1.10.0. Should they also treat any exception as invalid file and re-generate the stats? If that's the case, then I think we don't even need the `InvalidStatsFileException` class because it's essentially same as `Exception`.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#issuecomment-2982309570
Flink new flaky test: https://github.com/apache/iceberg/issues/13338
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2158168420
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +304,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
Can I just catch `IOException` and `RuntimeException` here? As all the error file handling errors falls into this category?
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2161134653
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
My thoughts:
- I would like to see this in 1.10.0 - I care about his more than anything else. I would accept really any compromise, but we need this out in 1.10.0, so no more new corrupted stats files are generated.
- I would prefer the extra code which handles corrupted stats files as simple as possible, as I would like to remove it in 1.11.0, and don't keep it any longer
- Catching any exception and assuming that is caused by the wrong schema seems problematic. Almost as problematic as parsing the exception messages.
- I don't see any fool-proof solution to detect this corruption than trying to read the stat file with the corrupted schema and successfully reading through it. OTOH, I'm not sure that we want to go this deep with the temp fix. Also, in this case we might just return this stats instead of throwing an exception.
@ajantha-bhat: Do we have an exception when we call `hasNext()`? This way we could test the iterator without changing the state.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2163442630
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
I would love to hear from @lirui-apache: Seems like he has users on this feature already
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#issuecomment-3003581490
Merged to main.
Thanks for the fix @ajantha-bhat and @lirui-apache and @deniskuzZ for the review.
This is an important change which we have to highlight in the release notes. @ajantha-bhat: Please create those.
CC: @stevenzwu - Please don't forget to add this to the 1.10.0 release notes. In nutshell: The schema of partition stats files generated by the 1.9.x Iceberg releases were incorrect (wrong fieldIds), and this is fixed starting from 1.10.0. The old stats files need to be recalculated, otherwise they will be unusable.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2151928078
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -65,29 +65,44 @@ private PartitionStatsHandler() {}
private static final Logger LOG = LoggerFactory.getLogger(PartitionStatsHandler.class);
- public static final int PARTITION_FIELD_ID = 0;
+ // schema of the partition stats file as per spec
+ public static final int PARTITION_FIELD_ID = 1;
Review Comment:
@RussellSpitzer: thoughts? Should be part of only 1.10.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: Fix filed ids of partition stats file [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2154222042
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -263,30 +249,12 @@ private static OutputFile newPartitionStatsFile(
private static PartitionStats recordToPartitionStats(StructLike record) {
PartitionStats stats =
new PartitionStats(
- record.get(PARTITION_POSITION, StructLike.class),
- record.get(SPEC_ID_POSITION, Integer.class));
- stats.set(DATA_RECORD_COUNT_POSITION, record.get(DATA_RECORD_COUNT_POSITION, Long.class));
- stats.set(DATA_FILE_COUNT_POSITION, record.get(DATA_FILE_COUNT_POSITION, Integer.class));
- stats.set(
- TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION,
- record.get(TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION, Long.class));
- stats.set(
- POSITION_DELETE_RECORD_COUNT_POSITION,
- record.get(POSITION_DELETE_RECORD_COUNT_POSITION, Long.class));
- stats.set(
- POSITION_DELETE_FILE_COUNT_POSITION,
- record.get(POSITION_DELETE_FILE_COUNT_POSITION, Integer.class));
- stats.set(
- EQUALITY_DELETE_RECORD_COUNT_POSITION,
- record.get(EQUALITY_DELETE_RECORD_COUNT_POSITION, Long.class));
- stats.set(
- EQUALITY_DELETE_FILE_COUNT_POSITION,
- record.get(EQUALITY_DELETE_FILE_COUNT_POSITION, Integer.class));
- stats.set(TOTAL_RECORD_COUNT_POSITION, record.get(TOTAL_RECORD_COUNT_POSITION, Long.class));
- stats.set(LAST_UPDATED_AT_POSITION, record.get(LAST_UPDATED_AT_POSITION, Long.class));
- stats.set(
- LAST_UPDATED_SNAPSHOT_ID_POSITION,
- record.get(LAST_UPDATED_SNAPSHOT_ID_POSITION, Long.class));
+ record.get(0, StructLike.class), // partition
Review Comment:
Using raw numbers directly in code is considered bad practice. Static analysis tools flag them as magic numbers and recommend replacing them with named constants. This enhances code readability and makes maintenance easier.
Alternatively, to adapt easily if the schema changes, we can introduce an index variable.
````
int pos = 0;
PartitionStats stats =
new PartitionStats(
record.get(pos++, StructLike.class),
record.get(pos++, Integer.class)
);
for (; pos < record.size(); pos++) {
stats.set(pos, record.get(pos, Object.class));
}
````
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2154222042
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -263,30 +249,12 @@ private static OutputFile newPartitionStatsFile(
private static PartitionStats recordToPartitionStats(StructLike record) {
PartitionStats stats =
new PartitionStats(
- record.get(PARTITION_POSITION, StructLike.class),
- record.get(SPEC_ID_POSITION, Integer.class));
- stats.set(DATA_RECORD_COUNT_POSITION, record.get(DATA_RECORD_COUNT_POSITION, Long.class));
- stats.set(DATA_FILE_COUNT_POSITION, record.get(DATA_FILE_COUNT_POSITION, Integer.class));
- stats.set(
- TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION,
- record.get(TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION, Long.class));
- stats.set(
- POSITION_DELETE_RECORD_COUNT_POSITION,
- record.get(POSITION_DELETE_RECORD_COUNT_POSITION, Long.class));
- stats.set(
- POSITION_DELETE_FILE_COUNT_POSITION,
- record.get(POSITION_DELETE_FILE_COUNT_POSITION, Integer.class));
- stats.set(
- EQUALITY_DELETE_RECORD_COUNT_POSITION,
- record.get(EQUALITY_DELETE_RECORD_COUNT_POSITION, Long.class));
- stats.set(
- EQUALITY_DELETE_FILE_COUNT_POSITION,
- record.get(EQUALITY_DELETE_FILE_COUNT_POSITION, Integer.class));
- stats.set(TOTAL_RECORD_COUNT_POSITION, record.get(TOTAL_RECORD_COUNT_POSITION, Long.class));
- stats.set(LAST_UPDATED_AT_POSITION, record.get(LAST_UPDATED_AT_POSITION, Long.class));
- stats.set(
- LAST_UPDATED_SNAPSHOT_ID_POSITION,
- record.get(LAST_UPDATED_SNAPSHOT_ID_POSITION, Long.class));
+ record.get(0, StructLike.class), // partition
Review Comment:
Using raw numbers directly in code is considered bad practice. Static analysis tools flag them as magic numbers and recommend replacing them with named constants. This enhances code readability and makes maintenance easier.
````
int i = 0;
PartitionStats stats =
new PartitionStats(
record.get(i++, StructLike.class),
record.get(i++, Integer.class)
);
for (; i < record.size(); i++) {
stats.set(i, record.get(i, Object.class));
}
````
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2151646748
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroPartitionStatsHandler.java:
##########
@@ -18,12 +18,44 @@
*/
package org.apache.iceberg.avro;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.PartitionStats;
+import org.apache.iceberg.PartitionStatsHandler;
import org.apache.iceberg.PartitionStatsHandlerTestBase;
+import org.apache.iceberg.Partitioning;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.TestTables;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.jupiter.api.Test;
public class TestAvroPartitionStatsHandler extends PartitionStatsHandlerTestBase {
public FileFormat format() {
return FileFormat.AVRO;
}
+
+ @Test
+ public void testReadingStatsWithInvalidSchema() throws Exception {
+ Table testTable =
+ TestTables.create(tempDir("old_schema"), "old_schema", SCHEMA, SPEC, 2, fileFormatProperty);
+ Schema schema = PartitionStatsHandler.schema(Partitioning.partitionType(testTable));
+
+ String invalidSchema =
+ getClass()
+ .getClassLoader()
+ .getResource("org/apache/iceberg/PartitionStatsInvalidSchema.avro")
Review Comment:
Could we just generate the file on the fly instead of checking in the binary?
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2152834762
##########
parquet/src/test/java/org/apache/iceberg/TestParquetPartitionStatsHandler.java:
##########
@@ -18,9 +18,36 @@
*/
package org.apache.iceberg;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.jupiter.api.Test;
+
public class TestParquetPartitionStatsHandler extends PartitionStatsHandlerTestBase {
public FileFormat format() {
return FileFormat.PARQUET;
}
+
+ @Test
+ public void testReadingStatsWithInvalidSchema() throws Exception {
+ Table testTable =
+ TestTables.create(tempDir("old_schema"), "old_schema", SCHEMA, SPEC, 2, fileFormatProperty);
+ Schema schema = PartitionStatsHandler.schema(Partitioning.partitionType(testTable));
+
+ String invalidSchema =
+ getClass()
+ .getClassLoader()
+ .getResource("org/apache/iceberg/PartitionStatsInvalidSchema.parquet")
Review Comment:
Found a way to generate stats with old schema with few lines of code and added it instead of binaries.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2161052987
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
The idea was to make it easier for users to tell whether they are trying to read an invalid stats file. If we decide to treat any exception as `InvalidStatsFileException`, then it's indeed redundant.
Maybe this doesn't matter for computing stats files, as we can always fall back to a full compute. But when reading stats files via the `readPartitionStatsFile` API, I think it's useful to tell if the failure is due to invalid file, or e.g. temporary IO issues, so that users can take different measures on these exceptions.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2158224710
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
I think I didn't clearly get what you mean. Here `readPartitionStatsFile` will not throw the exception. It always return the lazy iterable. Only the caller who access items from this iterable will get the exception. Accessing the iterable in `readPartitionStatsFile` will void the laziness. Which is not good.
Are you suggesting to provide a new public API wrapper just to catch the exception and throw `InvalidStatsFileException`? I feel it is redundant. Lets see what others think.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2154222042
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -263,30 +249,12 @@ private static OutputFile newPartitionStatsFile(
private static PartitionStats recordToPartitionStats(StructLike record) {
PartitionStats stats =
new PartitionStats(
- record.get(PARTITION_POSITION, StructLike.class),
- record.get(SPEC_ID_POSITION, Integer.class));
- stats.set(DATA_RECORD_COUNT_POSITION, record.get(DATA_RECORD_COUNT_POSITION, Long.class));
- stats.set(DATA_FILE_COUNT_POSITION, record.get(DATA_FILE_COUNT_POSITION, Integer.class));
- stats.set(
- TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION,
- record.get(TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION, Long.class));
- stats.set(
- POSITION_DELETE_RECORD_COUNT_POSITION,
- record.get(POSITION_DELETE_RECORD_COUNT_POSITION, Long.class));
- stats.set(
- POSITION_DELETE_FILE_COUNT_POSITION,
- record.get(POSITION_DELETE_FILE_COUNT_POSITION, Integer.class));
- stats.set(
- EQUALITY_DELETE_RECORD_COUNT_POSITION,
- record.get(EQUALITY_DELETE_RECORD_COUNT_POSITION, Long.class));
- stats.set(
- EQUALITY_DELETE_FILE_COUNT_POSITION,
- record.get(EQUALITY_DELETE_FILE_COUNT_POSITION, Integer.class));
- stats.set(TOTAL_RECORD_COUNT_POSITION, record.get(TOTAL_RECORD_COUNT_POSITION, Long.class));
- stats.set(LAST_UPDATED_AT_POSITION, record.get(LAST_UPDATED_AT_POSITION, Long.class));
- stats.set(
- LAST_UPDATED_SNAPSHOT_ID_POSITION,
- record.get(LAST_UPDATED_SNAPSHOT_ID_POSITION, Long.class));
+ record.get(0, StructLike.class), // partition
Review Comment:
Using raw numbers directly in code is considered bad practice. Static analysis tools flag them as magic numbers and recommend replacing them with named constants. This enhances code readability and makes maintenance easier.
Alternatively, we can introduce an index variable.
````
int pos = 0;
PartitionStats stats =
new PartitionStats(
record.get(pos++, StructLike.class),
record.get(pos++, Integer.class)
);
for (; pos < record.size(); pos++) {
stats.set(pos, record.get(pos, Object.class));
}
````
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2156630946
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
Does this mean that being able to read an old stats file is not a goal 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: Fix filed ids of partition stats file [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2154222042
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -263,30 +249,12 @@ private static OutputFile newPartitionStatsFile(
private static PartitionStats recordToPartitionStats(StructLike record) {
PartitionStats stats =
new PartitionStats(
- record.get(PARTITION_POSITION, StructLike.class),
- record.get(SPEC_ID_POSITION, Integer.class));
- stats.set(DATA_RECORD_COUNT_POSITION, record.get(DATA_RECORD_COUNT_POSITION, Long.class));
- stats.set(DATA_FILE_COUNT_POSITION, record.get(DATA_FILE_COUNT_POSITION, Integer.class));
- stats.set(
- TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION,
- record.get(TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION, Long.class));
- stats.set(
- POSITION_DELETE_RECORD_COUNT_POSITION,
- record.get(POSITION_DELETE_RECORD_COUNT_POSITION, Long.class));
- stats.set(
- POSITION_DELETE_FILE_COUNT_POSITION,
- record.get(POSITION_DELETE_FILE_COUNT_POSITION, Integer.class));
- stats.set(
- EQUALITY_DELETE_RECORD_COUNT_POSITION,
- record.get(EQUALITY_DELETE_RECORD_COUNT_POSITION, Long.class));
- stats.set(
- EQUALITY_DELETE_FILE_COUNT_POSITION,
- record.get(EQUALITY_DELETE_FILE_COUNT_POSITION, Integer.class));
- stats.set(TOTAL_RECORD_COUNT_POSITION, record.get(TOTAL_RECORD_COUNT_POSITION, Long.class));
- stats.set(LAST_UPDATED_AT_POSITION, record.get(LAST_UPDATED_AT_POSITION, Long.class));
- stats.set(
- LAST_UPDATED_SNAPSHOT_ID_POSITION,
- record.get(LAST_UPDATED_SNAPSHOT_ID_POSITION, Long.class));
+ record.get(0, StructLike.class), // partition
Review Comment:
Using raw numbers directly in code is considered bad practice. Static analysis tools flag them as magic numbers and recommend replacing them with named constants. This enhances code readability and makes maintenance easier.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat closed pull request #13329: Core: Fix filed ids of partition stats file
URL: https://github.com/apache/iceberg/pull/13329
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2157113145
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
The intention is to handle this as a bug, and we don't provide backward compatibility.
The goal is to make the stats recalculation resilient, but we don't want to read the old stats, and keep code "indefinitely" just to fix this issue.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2165745191
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
> But I wonder what our messages will be to users of the readPartitionStatsFile API in 1.10.0. Should they also treat any exception as invalid file and re-generate the stats? If that's the case, then I think we don't even need the InvalidStatsFileException class because it's essentially same as Exception.
`InvalidStatsFileException` is for the callers of `computeAndMergeStatsIncremental` to know that it needs to be a fallback.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2163131803
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
@deniskuzZ, @lirui-apache, @pvary: If no more comments, please approve. So, we can go ahead with this 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: Fix filed ids of partition stats file [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#issuecomment-2980502271
off-topic: partition stats file format is highly coupled with table `write.format`. If CU is using `ORC`, he automatically loses the partition stats.
Do you think we can decouple the file formats? If yes, I can raise a PR for that.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#issuecomment-2981014331
> off-topic: partition stats file format is highly coupled with table write.format. If CU is using ORC, he automatically loses the partition stats.
Yes. In the initial proposal document jack-ye has brought this up. Spec was hardcoded that it should be in table default format. Then we updated that it can be any of the format (https://iceberg.apache.org/spec/#partition-statistics-file).
So, we can have a table property to configure a different format stats than data format. But I recommend supporting InternalData for ORC, I can help on it too if needed (I did it for parquet and avro). So, that ORC can be used for writing table metadata in v4. Than changing the format of partition stats 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: Fix filed ids of partition stats file [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#issuecomment-2979901787
> @lirui-apache, @deniskuzZ: Just a heads up as you guys are using this feature as per my knowledge.
Thanks @ajantha-bhat , we are also looking at the issue internally. Guess I can incorporate your fix when it's done.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2151925151
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -65,29 +65,44 @@ private PartitionStatsHandler() {}
private static final Logger LOG = LoggerFactory.getLogger(PartitionStatsHandler.class);
- public static final int PARTITION_FIELD_ID = 0;
+ // schema of the partition stats file as per spec
+ public static final int PARTITION_FIELD_ID = 1;
public static final String PARTITION_FIELD_NAME = "partition";
- public static final NestedField SPEC_ID = NestedField.required(1, "spec_id", IntegerType.get());
+ public static final NestedField SPEC_ID = NestedField.required(2, "spec_id", IntegerType.get());
public static final NestedField DATA_RECORD_COUNT =
- NestedField.required(2, "data_record_count", LongType.get());
+ NestedField.required(3, "data_record_count", LongType.get());
public static final NestedField DATA_FILE_COUNT =
- NestedField.required(3, "data_file_count", IntegerType.get());
+ NestedField.required(4, "data_file_count", IntegerType.get());
public static final NestedField TOTAL_DATA_FILE_SIZE_IN_BYTES =
- NestedField.required(4, "total_data_file_size_in_bytes", LongType.get());
+ NestedField.required(5, "total_data_file_size_in_bytes", LongType.get());
public static final NestedField POSITION_DELETE_RECORD_COUNT =
- NestedField.optional(5, "position_delete_record_count", LongType.get());
+ NestedField.optional(6, "position_delete_record_count", LongType.get());
public static final NestedField POSITION_DELETE_FILE_COUNT =
- NestedField.optional(6, "position_delete_file_count", IntegerType.get());
+ NestedField.optional(7, "position_delete_file_count", IntegerType.get());
public static final NestedField EQUALITY_DELETE_RECORD_COUNT =
- NestedField.optional(7, "equality_delete_record_count", LongType.get());
+ NestedField.optional(8, "equality_delete_record_count", LongType.get());
public static final NestedField EQUALITY_DELETE_FILE_COUNT =
- NestedField.optional(8, "equality_delete_file_count", IntegerType.get());
+ NestedField.optional(9, "equality_delete_file_count", IntegerType.get());
public static final NestedField TOTAL_RECORD_COUNT =
- NestedField.optional(9, "total_record_count", LongType.get());
+ NestedField.optional(10, "total_record_count", LongType.get());
public static final NestedField LAST_UPDATED_AT =
- NestedField.optional(10, "last_updated_at", LongType.get());
+ NestedField.optional(11, "last_updated_at", LongType.get());
public static final NestedField LAST_UPDATED_SNAPSHOT_ID =
- NestedField.optional(11, "last_updated_snapshot_id", LongType.get());
+ NestedField.optional(12, "last_updated_snapshot_id", LongType.get());
+
+ // position in StructLike
+ static final int PARTITION_POSITION = 0;
+ static final int SPEC_ID_POSITION = 1;
+ static final int DATA_RECORD_COUNT_POSITION = 2;
+ static final int DATA_FILE_COUNT_POSITION = 3;
+ static final int TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION = 4;
+ static final int POSITION_DELETE_RECORD_COUNT_POSITION = 5;
+ static final int POSITION_DELETE_FILE_COUNT_POSITION = 6;
+ static final int EQUALITY_DELETE_RECORD_COUNT_POSITION = 7;
+ static final int EQUALITY_DELETE_FILE_COUNT_POSITION = 8;
+ static final int TOTAL_RECORD_COUNT_POSITION = 9;
+ static final int LAST_UPDATED_AT_POSITION = 10;
+ static final int LAST_UPDATED_SNAPSHOT_ID_POSITION = 11;
Review Comment:
are you suggesting to use the hardcoded value like 0,1,2 instead?
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2152339760
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -239,30 +263,30 @@ private static OutputFile newPartitionStatsFile(
private static PartitionStats recordToPartitionStats(StructLike record) {
PartitionStats stats =
new PartitionStats(
- record.get(PARTITION_FIELD_ID, StructLike.class),
- record.get(SPEC_ID.fieldId(), Integer.class));
- stats.set(DATA_RECORD_COUNT.fieldId(), record.get(DATA_RECORD_COUNT.fieldId(), Long.class));
- stats.set(DATA_FILE_COUNT.fieldId(), record.get(DATA_FILE_COUNT.fieldId(), Integer.class));
+ record.get(PARTITION_POSITION, StructLike.class),
+ record.get(SPEC_ID_POSITION, Integer.class));
+ stats.set(DATA_RECORD_COUNT_POSITION, record.get(DATA_RECORD_COUNT_POSITION, Long.class));
+ stats.set(DATA_FILE_COUNT_POSITION, record.get(DATA_FILE_COUNT_POSITION, Integer.class));
stats.set(
- TOTAL_DATA_FILE_SIZE_IN_BYTES.fieldId(),
- record.get(TOTAL_DATA_FILE_SIZE_IN_BYTES.fieldId(), Long.class));
+ TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION,
+ record.get(TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION, Long.class));
stats.set(
- POSITION_DELETE_RECORD_COUNT.fieldId(),
- record.get(POSITION_DELETE_RECORD_COUNT.fieldId(), Long.class));
+ POSITION_DELETE_RECORD_COUNT_POSITION,
+ record.get(POSITION_DELETE_RECORD_COUNT_POSITION, Long.class));
stats.set(
- POSITION_DELETE_FILE_COUNT.fieldId(),
- record.get(POSITION_DELETE_FILE_COUNT.fieldId(), Integer.class));
+ POSITION_DELETE_FILE_COUNT_POSITION,
+ record.get(POSITION_DELETE_FILE_COUNT_POSITION, Integer.class));
stats.set(
- EQUALITY_DELETE_RECORD_COUNT.fieldId(),
- record.get(EQUALITY_DELETE_RECORD_COUNT.fieldId(), Long.class));
+ EQUALITY_DELETE_RECORD_COUNT_POSITION,
+ record.get(EQUALITY_DELETE_RECORD_COUNT_POSITION, Long.class));
stats.set(
- EQUALITY_DELETE_FILE_COUNT.fieldId(),
- record.get(EQUALITY_DELETE_FILE_COUNT.fieldId(), Integer.class));
- stats.set(TOTAL_RECORD_COUNT.fieldId(), record.get(TOTAL_RECORD_COUNT.fieldId(), Long.class));
- stats.set(LAST_UPDATED_AT.fieldId(), record.get(LAST_UPDATED_AT.fieldId(), Long.class));
+ EQUALITY_DELETE_FILE_COUNT_POSITION,
+ record.get(EQUALITY_DELETE_FILE_COUNT_POSITION, Integer.class));
+ stats.set(TOTAL_RECORD_COUNT_POSITION, record.get(TOTAL_RECORD_COUNT_POSITION, Long.class));
+ stats.set(LAST_UPDATED_AT_POSITION, record.get(LAST_UPDATED_AT_POSITION, Long.class));
stats.set(
- LAST_UPDATED_SNAPSHOT_ID.fieldId(),
- record.get(LAST_UPDATED_SNAPSHOT_ID.fieldId(), Long.class));
+ LAST_UPDATED_SNAPSHOT_ID_POSITION,
+ record.get(LAST_UPDATED_SNAPSHOT_ID_POSITION, Long.class));
Review Comment:
`i` should start from `2`?
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2160952515
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
I tried to update the code like below
```
public static CloseableIterable<PartitionStats> readPartitionStatsFile(
Schema schema, InputFile inputFile) {
Preconditions.checkArgument(schema != null, "Invalid schema: null");
Preconditions.checkArgument(inputFile != null, "Invalid input file: null");
FileFormat fileFormat = FileFormat.fromFileName(inputFile.location());
Preconditions.checkArgument(
fileFormat != null, "Unable to determine format of file: %s", inputFile.location());
CloseableIterable<StructLike> records =
InternalData.read(fileFormat, inputFile).project(schema).build();
CloseableIterable<PartitionStats> transformed = CloseableIterable.transform(records, PartitionStatsHandler::recordToPartitionStats);
return new CloseableIterable<>() {
@Override
public CloseableIterator<PartitionStats> iterator() {
return new InvalidStatsCatchingIterator(transformed.iterator());
}
@Override
public void close() throws IOException {
transformed.close();
}
};
}
private static class InvalidStatsCatchingIterator implements CloseableIterator<PartitionStats> {
private final CloseableIterator<PartitionStats> delegate;
InvalidStatsCatchingIterator(CloseableIterator<PartitionStats> delegate) {
this.delegate = delegate;
}
@Override
public boolean hasNext() {
return runWithCatch(delegate::hasNext);
}
@Override
public PartitionStats next() {
return runWithCatch(delegate::next);
}
@Override
public void close() throws IOException {
delegate.close();
}
private static <T> T runWithCatch(Supplier<T> action) {
try {
return action.get();
} catch (Exception exception) {
throw new InvalidStatsFileException(exception);
}
}
}
```
But still the exception will be thrown when the iterator is accessed. Not when `readPartitionStatsFile` is called. So, I don't see much value in wrapping this as now we are catching any exception as `InvalidStatsFileException`.
So, the current code look good to me and I don't think we need to do this. Thoughts?
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2158117893
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
`readPartitionStatsFile`, returns an Iterable. The exception happens only when the first element is accessed. Collecting elements in `readPartitionStatsFile` just for the sake of catching exception can be expensive operation.
Are you not using the exposed API, `computeAndWriteStatsFile` for your integration?
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary merged PR #13329:
URL: https://github.com/apache/iceberg/pull/13329
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "lirui-apache (via GitHub)" <gi...@apache.org>.
lirui-apache commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2158197890
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
I think `readPartitionStatsFile` is also a public API, and it's not friendly to ask users to check low level exceptions to determine invalid stats files. The returned Iterable can stay lazy, and is only a wrapper of an underlying Iterable reading the file. When the underlying iterable/iterator throws exceptions, the wrapper catches and determines if this is due to invalid stats file and throws `InvalidStatsFileException` accordingly. What do you think?
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2151623764
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -65,29 +65,44 @@ private PartitionStatsHandler() {}
private static final Logger LOG = LoggerFactory.getLogger(PartitionStatsHandler.class);
- public static final int PARTITION_FIELD_ID = 0;
+ // schema of the partition stats file as per spec
+ public static final int PARTITION_FIELD_ID = 1;
public static final String PARTITION_FIELD_NAME = "partition";
- public static final NestedField SPEC_ID = NestedField.required(1, "spec_id", IntegerType.get());
+ public static final NestedField SPEC_ID = NestedField.required(2, "spec_id", IntegerType.get());
public static final NestedField DATA_RECORD_COUNT =
- NestedField.required(2, "data_record_count", LongType.get());
+ NestedField.required(3, "data_record_count", LongType.get());
public static final NestedField DATA_FILE_COUNT =
- NestedField.required(3, "data_file_count", IntegerType.get());
+ NestedField.required(4, "data_file_count", IntegerType.get());
public static final NestedField TOTAL_DATA_FILE_SIZE_IN_BYTES =
- NestedField.required(4, "total_data_file_size_in_bytes", LongType.get());
+ NestedField.required(5, "total_data_file_size_in_bytes", LongType.get());
public static final NestedField POSITION_DELETE_RECORD_COUNT =
- NestedField.optional(5, "position_delete_record_count", LongType.get());
+ NestedField.optional(6, "position_delete_record_count", LongType.get());
public static final NestedField POSITION_DELETE_FILE_COUNT =
- NestedField.optional(6, "position_delete_file_count", IntegerType.get());
+ NestedField.optional(7, "position_delete_file_count", IntegerType.get());
public static final NestedField EQUALITY_DELETE_RECORD_COUNT =
- NestedField.optional(7, "equality_delete_record_count", LongType.get());
+ NestedField.optional(8, "equality_delete_record_count", LongType.get());
public static final NestedField EQUALITY_DELETE_FILE_COUNT =
- NestedField.optional(8, "equality_delete_file_count", IntegerType.get());
+ NestedField.optional(9, "equality_delete_file_count", IntegerType.get());
public static final NestedField TOTAL_RECORD_COUNT =
- NestedField.optional(9, "total_record_count", LongType.get());
+ NestedField.optional(10, "total_record_count", LongType.get());
public static final NestedField LAST_UPDATED_AT =
- NestedField.optional(10, "last_updated_at", LongType.get());
+ NestedField.optional(11, "last_updated_at", LongType.get());
public static final NestedField LAST_UPDATED_SNAPSHOT_ID =
- NestedField.optional(11, "last_updated_snapshot_id", LongType.get());
+ NestedField.optional(12, "last_updated_snapshot_id", LongType.get());
+
+ // position in StructLike
+ static final int PARTITION_POSITION = 0;
+ static final int SPEC_ID_POSITION = 1;
+ static final int DATA_RECORD_COUNT_POSITION = 2;
+ static final int DATA_FILE_COUNT_POSITION = 3;
+ static final int TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION = 4;
+ static final int POSITION_DELETE_RECORD_COUNT_POSITION = 5;
+ static final int POSITION_DELETE_FILE_COUNT_POSITION = 6;
+ static final int EQUALITY_DELETE_RECORD_COUNT_POSITION = 7;
+ static final int EQUALITY_DELETE_FILE_COUNT_POSITION = 8;
+ static final int TOTAL_RECORD_COUNT_POSITION = 9;
+ static final int LAST_UPDATED_AT_POSITION = 10;
+ static final int LAST_UPDATED_SNAPSHOT_ID_POSITION = 11;
Review Comment:
Do we really need these?
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2151920350
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +304,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
callstack for parquet
```
Not a primitive type: struct<1000: c2: optional string, 1001: c3: optional string>
java.lang.IllegalArgumentException: Not a primitive type: struct<1000: c2: optional string, 1001: c3: optional string>
at org.apache.iceberg.types.Type.asPrimitiveType(Type.java:73)
at org.apache.iceberg.parquet.TypeWithSchemaVisitor.visit(TypeWithSchemaVisitor.java:53)
at org.apache.iceberg.parquet.TypeWithSchemaVisitor.visitField(TypeWithSchemaVisitor.java:192)
at org.apache.iceberg.parquet.TypeWithSchemaVisitor.visitFields(TypeWithSchemaVisitor.java:207)
at org.apache.iceberg.parquet.TypeWithSchemaVisitor.visit(TypeWithSchemaVisitor.java:49)
at org.apache.iceberg.parquet.ParquetSchemaUtil.pruneColumns(ParquetSchemaUtil.java:134)
at org.apache.iceberg.parquet.ReadConf.<init>(ReadConf.java:82)
at org.apache.iceberg.parquet.ParquetReader.init(ParquetReader.java:74)
at org.apache.iceberg.parquet.ParquetReader.iterator(ParquetReader.java:94)
at org.apache.iceberg.io.CloseableIterable$7$1.<init>(CloseableIterable.java:205)
at org.apache.iceberg.io.CloseableIterable$7.iterator(CloseableIterable.java:204)
at org.apache.iceberg.io.CloseableIterable$7.iterator(CloseableIterable.java:196)
at org.apache.iceberg.relocated.com.google.common.collect.Lists.newArrayList(Lists.java:139)
at org.apache.iceberg.TestParquetPartitionStatsHandler.testStatsWithInvalidSchema(TestParquetPartitionStatsHandler.java:45)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
```
callstack for avro
```
java.lang.IllegalStateException: Not an instance of org.apache.iceberg.StructLike: 0
at org.apache.iceberg.data.GenericRecord.get(GenericRecord.java:138)
at org.apache.iceberg.PartitionStatsHandler.recordToPartitionStats(PartitionStatsHandler.java:266)
at org.apache.iceberg.io.CloseableIterable$7$1.next(CloseableIterable.java:219)
...(99 remaining lines not displayed - this can be changed with Assertions.setMaxStackTraceElementsDisplayed)
at org.apache.iceberg.avro.TestAvroPartitionStatsHandler.testReadingStatsWithInvalidSchema(TestAvroPartitionStatsHandler.java:57)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
```
or should I just catch `RuntimeException` 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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#issuecomment-2979834949
@lirui-apache, @deniskuzZ: Just a heads up as you guys are using this feature as per my knowledge.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2157446396
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +304,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
Also what happens if the file is corrupted? Not a parquet file, or something like missing footer?
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2152348806
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -239,30 +263,30 @@ private static OutputFile newPartitionStatsFile(
private static PartitionStats recordToPartitionStats(StructLike record) {
PartitionStats stats =
new PartitionStats(
- record.get(PARTITION_FIELD_ID, StructLike.class),
- record.get(SPEC_ID.fieldId(), Integer.class));
- stats.set(DATA_RECORD_COUNT.fieldId(), record.get(DATA_RECORD_COUNT.fieldId(), Long.class));
- stats.set(DATA_FILE_COUNT.fieldId(), record.get(DATA_FILE_COUNT.fieldId(), Integer.class));
+ record.get(PARTITION_POSITION, StructLike.class),
+ record.get(SPEC_ID_POSITION, Integer.class));
+ stats.set(DATA_RECORD_COUNT_POSITION, record.get(DATA_RECORD_COUNT_POSITION, Long.class));
+ stats.set(DATA_FILE_COUNT_POSITION, record.get(DATA_FILE_COUNT_POSITION, Integer.class));
stats.set(
- TOTAL_DATA_FILE_SIZE_IN_BYTES.fieldId(),
- record.get(TOTAL_DATA_FILE_SIZE_IN_BYTES.fieldId(), Long.class));
+ TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION,
+ record.get(TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION, Long.class));
stats.set(
- POSITION_DELETE_RECORD_COUNT.fieldId(),
- record.get(POSITION_DELETE_RECORD_COUNT.fieldId(), Long.class));
+ POSITION_DELETE_RECORD_COUNT_POSITION,
+ record.get(POSITION_DELETE_RECORD_COUNT_POSITION, Long.class));
stats.set(
- POSITION_DELETE_FILE_COUNT.fieldId(),
- record.get(POSITION_DELETE_FILE_COUNT.fieldId(), Integer.class));
+ POSITION_DELETE_FILE_COUNT_POSITION,
+ record.get(POSITION_DELETE_FILE_COUNT_POSITION, Integer.class));
stats.set(
- EQUALITY_DELETE_RECORD_COUNT.fieldId(),
- record.get(EQUALITY_DELETE_RECORD_COUNT.fieldId(), Long.class));
+ EQUALITY_DELETE_RECORD_COUNT_POSITION,
+ record.get(EQUALITY_DELETE_RECORD_COUNT_POSITION, Long.class));
stats.set(
- EQUALITY_DELETE_FILE_COUNT.fieldId(),
- record.get(EQUALITY_DELETE_FILE_COUNT.fieldId(), Integer.class));
- stats.set(TOTAL_RECORD_COUNT.fieldId(), record.get(TOTAL_RECORD_COUNT.fieldId(), Long.class));
- stats.set(LAST_UPDATED_AT.fieldId(), record.get(LAST_UPDATED_AT.fieldId(), Long.class));
+ EQUALITY_DELETE_FILE_COUNT_POSITION,
+ record.get(EQUALITY_DELETE_FILE_COUNT_POSITION, Integer.class));
+ stats.set(TOTAL_RECORD_COUNT_POSITION, record.get(TOTAL_RECORD_COUNT_POSITION, Long.class));
+ stats.set(LAST_UPDATED_AT_POSITION, record.get(LAST_UPDATED_AT_POSITION, Long.class));
stats.set(
- LAST_UPDATED_SNAPSHOT_ID.fieldId(),
- record.get(LAST_UPDATED_SNAPSHOT_ID.fieldId(), Long.class));
+ LAST_UPDATED_SNAPSHOT_ID_POSITION,
+ record.get(LAST_UPDATED_SNAPSHOT_ID_POSITION, Long.class));
Review Comment:
yes, for sure. But I got what he mean and updated locally.
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2154222042
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -263,30 +249,12 @@ private static OutputFile newPartitionStatsFile(
private static PartitionStats recordToPartitionStats(StructLike record) {
PartitionStats stats =
new PartitionStats(
- record.get(PARTITION_POSITION, StructLike.class),
- record.get(SPEC_ID_POSITION, Integer.class));
- stats.set(DATA_RECORD_COUNT_POSITION, record.get(DATA_RECORD_COUNT_POSITION, Long.class));
- stats.set(DATA_FILE_COUNT_POSITION, record.get(DATA_FILE_COUNT_POSITION, Integer.class));
- stats.set(
- TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION,
- record.get(TOTAL_DATA_FILE_SIZE_IN_BYTES_POSITION, Long.class));
- stats.set(
- POSITION_DELETE_RECORD_COUNT_POSITION,
- record.get(POSITION_DELETE_RECORD_COUNT_POSITION, Long.class));
- stats.set(
- POSITION_DELETE_FILE_COUNT_POSITION,
- record.get(POSITION_DELETE_FILE_COUNT_POSITION, Integer.class));
- stats.set(
- EQUALITY_DELETE_RECORD_COUNT_POSITION,
- record.get(EQUALITY_DELETE_RECORD_COUNT_POSITION, Long.class));
- stats.set(
- EQUALITY_DELETE_FILE_COUNT_POSITION,
- record.get(EQUALITY_DELETE_FILE_COUNT_POSITION, Integer.class));
- stats.set(TOTAL_RECORD_COUNT_POSITION, record.get(TOTAL_RECORD_COUNT_POSITION, Long.class));
- stats.set(LAST_UPDATED_AT_POSITION, record.get(LAST_UPDATED_AT_POSITION, Long.class));
- stats.set(
- LAST_UPDATED_SNAPSHOT_ID_POSITION,
- record.get(LAST_UPDATED_SNAPSHOT_ID_POSITION, Long.class));
+ record.get(0, StructLike.class), // partition
Review Comment:
Using raw numbers directly in code is considered bad practice. Static analysis tools flag them as magic numbers and recommend replacing them with named constants. This enhances code readability and makes maintenance easier.
````
int pos = 0;
PartitionStats stats =
new PartitionStats(
record.get(pos++, StructLike.class),
record.get(pos++, Integer.class)
);
for (; pos < record.size(); pos++) {
stats.set(pos, record.get(pos, Object.class));
}
````
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2151928078
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -65,29 +65,44 @@ private PartitionStatsHandler() {}
private static final Logger LOG = LoggerFactory.getLogger(PartitionStatsHandler.class);
- public static final int PARTITION_FIELD_ID = 0;
+ // schema of the partition stats file as per spec
+ public static final int PARTITION_FIELD_ID = 1;
Review Comment:
@RussellSpitzer: thoughts?
--
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: Fix filed ids of partition stats file [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #13329:
URL: https://github.com/apache/iceberg/pull/13329#discussion_r2165745191
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -280,6 +273,8 @@ private static Collection<PartitionStats> computeAndMergeStatsIncremental(
oldStats.forEach(
partitionStats ->
statsMap.put(partitionStats.specId(), partitionStats.partition(), partitionStats));
+ } catch (IllegalArgumentException | IllegalStateException exception) {
Review Comment:
> But I wonder what our messages will be to users of the readPartitionStatsFile API in 1.10.0. Should they also treat any exception as invalid file and re-generate the stats? If that's the case, then I think we don't even need the InvalidStatsFileException class because it's essentially same as Exception.
`InvalidStatsFileException` is for the callers of `computeAndMergeStatsIncremental` to know that it needs to be a fallback when old stats cannot be read. I didn't fallback on any exception for `computeAndMergeStatsIncremental` is because, if manifest reading failed for network issues, it can be retired and can succeed the incremental compute after network is repaired.
--
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