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 2023/09/05 17:07:07 UTC
[GitHub] [iceberg] ajantha-bhat opened a new pull request, #8502: Api: Track partition statistics via TableMetadata
ajantha-bhat opened a new pull request, #8502:
URL: https://github.com/apache/iceberg/pull/8502
Tracking `PartitionStatisticsFile` in a same way as how `StatisticsFile` is already tracked.
Derived from #8488
Fixes #8457
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#issuecomment-1856508439
@ajantha-bhat, I'll take a look at other PRs once this is in. I feel this one is almost ready to go.
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#issuecomment-1845791782
@aokolnychyi: Thanks for the review.
- I see that most of the questions or comments related to why puffin stats followed that style. We can fix in this PR for partition stats and later back port to puffin too.
- I didn't add expire snapshots and remove orphan files test code (but have test case of RemoveSnapshots API) as I was thinking to keep the scope of PR to non-Spark. I will have a PR that depends on this PR which tests these function. So, this PR can be merged.
Got a little busy week. I will finish addressing comments and the follow up Spark PR for expire snapshots and remove orphan files by Monday.
Meanwhile you can also review the independent PR (Util for partition stats reading and writing) : https://github.com/apache/iceberg/pull/9170
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#discussion_r1427920868
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -946,15 +955,25 @@ private Builder(TableMetadata base) {
this.previousFileLocation = base.metadataFileLocation;
this.previousFiles = base.previousFiles;
this.refs = Maps.newHashMap(base.refs);
- this.statisticsFiles =
Review Comment:
Currently one stats file per snapshot. But I think in future it may track multiple files too.
I followed the same pattern as existing puffin files. I want to keep the interfaces consistent.
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#issuecomment-1861945339
@aokolnychyi: Thanks for the detailed review and merge.
The next small PR to review will be https://github.com/apache/iceberg/pull/9284 (Spark 3.5: Ensure that partition stats files are considered for GC procedures)
> I had only one comment about the access modifier for the base implementation class.
I will check this in a followup. Again this follows the style from puffin files.
I will also handle the applicable comments from this PR to stats files code (puffin files) in a follow up 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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#discussion_r1654742829
##########
core/src/main/java/org/apache/iceberg/TableMetadataParser.java:
##########
@@ -481,6 +488,13 @@ public static TableMetadata fromJson(String metadataLocation, JsonNode node) {
statisticsFiles = ImmutableList.of();
}
+ List<PartitionStatisticsFile> partitionStatisticsFiles;
+ if (node.has(PARTITION_STATISTICS)) {
+ partitionStatisticsFiles = partitionStatsFilesFromJson(node.get(PARTITION_STATISTICS));
+ } else {
+ partitionStatisticsFiles = ImmutableList.of();
+ }
+
Review Comment:
The way partition stats tracked and added to table metadata is same as puffin files right now.
The stats are optional, so even if we lose it. Planner won't return wrong query results.
However these stats can be helpful to improve query performance. We are planning to provide a call procedure, compute partition stats. Which will check the last snapshot that had partition stats and incrementally compute the stats for remaining snapshots.
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#discussion_r1427180249
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -946,15 +955,25 @@ private Builder(TableMetadata base) {
this.previousFileLocation = base.metadataFileLocation;
this.previousFiles = base.previousFiles;
this.refs = Maps.newHashMap(base.refs);
- this.statisticsFiles =
Review Comment:
@ajantha-bhat, just to make sure I understand. We try to replace the partition stats for each snapshot but it is not required by the spec so it is technically possible to have multiple files for one snapshot?
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1288,6 +1307,24 @@ public Builder suppressHistoricalSnapshots() {
return this;
}
+ public Builder setPartitionStatistics(PartitionStatisticsFile partitionStatisticsFile) {
+ Preconditions.checkNotNull(partitionStatisticsFile, "partition statistics file is null");
+ partitionStatisticsFiles.put(
+ partitionStatisticsFile.snapshotId(), ImmutableList.of(partitionStatisticsFile));
+ changes.add(new MetadataUpdate.SetPartitionStatistics(partitionStatisticsFile));
+ return this;
+ }
+
+ public Builder removePartitionStatistics(long snapshotId) {
+ Preconditions.checkNotNull(snapshotId, "snapshotId is null");
Review Comment:
How can it be null if it is primitive?
##########
core/src/main/java/org/apache/iceberg/TableMetadataParser.java:
##########
@@ -561,4 +576,21 @@ private static List<StatisticsFile> statisticsFilesFromJson(JsonNode statisticsF
return statisticsFilesBuilder.build();
}
+
+ private static List<PartitionStatisticsFile> partitionStatisticsFilesFromJson(
+ JsonNode partitionStatisticsFilesList) {
Review Comment:
I don't mind using longer variables but I wonder whether a shorter version and staying on one line in some places would be more readable in this method.
##########
core/src/main/java/org/apache/iceberg/SetPartitionStatistics.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Set;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+public class SetPartitionStatistics implements UpdatePartitionStatistics {
+ private final TableOperations ops;
+ private final Set<PartitionStatisticsFile> partitionStatisticsToSet = Sets.newHashSet();
+ private final Set<Long> partitionStatisticsToRemove = Sets.newHashSet();
+
+ public SetPartitionStatistics(TableOperations ops) {
+ this.ops = ops;
+ }
+
+ @Override
+ public UpdatePartitionStatistics setPartitionStatistics(
+ PartitionStatisticsFile partitionStatisticsFile) {
Review Comment:
I feel the context in this class is pretty clear and we can call it `stats`, `partitionStats` or `file` to shorten the lines. Then we can also call other variables `partitionStatsToSet` or `statsToSet`.
##########
core/src/main/java/org/apache/iceberg/SetPartitionStatistics.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Set;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+public class SetPartitionStatistics implements UpdatePartitionStatistics {
+ private final TableOperations ops;
+ private final Set<PartitionStatisticsFile> partitionStatisticsToSet = Sets.newHashSet();
Review Comment:
What about using `Map<Long, PartitionStatisticsFile>` to make sure the files are overridden if multiple files are passed for the same snapshot? We can then simply call `statsToSet.values().forEach(...)` below?
##########
api/src/main/java/org/apache/iceberg/PartitionStatisticsFile.java:
##########
@@ -0,0 +1,38 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.io.Serializable;
+
+/**
+ * Represents a partition statistics file that can be used to read table data more efficiently.
+ *
+ * <p>Statistics are informational. A reader can choose to ignore statistics information. Statistics
+ * support is not required to read the table correctly.
+ */
+public interface PartitionStatisticsFile extends Serializable {
Review Comment:
Are we sure it is a good idea to make `PartitionStatisticsFile` serializable? I would probably not do that unless there is a good reason right now. None of our existing files are serializable by contract (they may be in practice but not by API).
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -946,15 +955,25 @@ private Builder(TableMetadata base) {
this.previousFileLocation = base.metadataFileLocation;
this.previousFiles = base.previousFiles;
this.refs = Maps.newHashMap(base.refs);
- this.statisticsFiles =
- base.statisticsFiles.stream().collect(Collectors.groupingBy(StatisticsFile::snapshotId));
-
+ this.statisticsFiles = statsFileBySnapshotID(base);
+ this.partitionStatisticsFiles = partitionStatsFileBySnapshotID(base);
this.snapshotsById = Maps.newHashMap(base.snapshotsById);
this.schemasById = Maps.newHashMap(base.schemasById);
this.specsById = Maps.newHashMap(base.specsById);
this.sortOrdersById = Maps.newHashMap(base.sortOrdersById);
}
+ private static Map<Long, List<StatisticsFile>> statsFileBySnapshotID(TableMetadata base) {
Review Comment:
Question: Is there a pattern in this class to have static methods at the end? If so, can we put these methods together with other static methods? If not, it is OK to keep them here.
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -946,15 +955,25 @@ private Builder(TableMetadata base) {
this.previousFileLocation = base.metadataFileLocation;
this.previousFiles = base.previousFiles;
this.refs = Maps.newHashMap(base.refs);
- this.statisticsFiles =
- base.statisticsFiles.stream().collect(Collectors.groupingBy(StatisticsFile::snapshotId));
-
+ this.statisticsFiles = statsFileBySnapshotID(base);
+ this.partitionStatisticsFiles = partitionStatsFileBySnapshotID(base);
this.snapshotsById = Maps.newHashMap(base.snapshotsById);
this.schemasById = Maps.newHashMap(base.schemasById);
this.specsById = Maps.newHashMap(base.specsById);
this.sortOrdersById = Maps.newHashMap(base.sortOrdersById);
}
+ private static Map<Long, List<StatisticsFile>> statsFileBySnapshotID(TableMetadata base) {
+ return base.statisticsFiles.stream()
+ .collect(Collectors.groupingBy(StatisticsFile::snapshotId));
+ }
+
+ private static Map<Long, List<PartitionStatisticsFile>> partitionStatsFileBySnapshotID(
Review Comment:
I think it is common for this class to call such methods `indexSmth` and pass actual elements. If so, what about something like below? We can use shorter variables to stay on line but that's totally optional. Up to you, method names are examples.
```
private static Map<Long, List<StatisticsFile>> indexStatistics(List<StatisticsFile> files) {
return files.stream().collect(Collectors.groupingBy(StatisticsFile::snapshotId));
}
private static Map<Long, List<PartitionStatisticsFile>> indexPartitionStatistics(
List<PartitionStatisticsFile> files) {
return files.stream().collect(Collectors.groupingBy(PartitionStatisticsFile::snapshotId));
}
```
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1288,6 +1307,24 @@ public Builder suppressHistoricalSnapshots() {
return this;
}
+ public Builder setPartitionStatistics(PartitionStatisticsFile partitionStatisticsFile) {
Review Comment:
Question: I wonder whether using just `file` or `partitionStats` to shorten the lines would make sense.
Completely up to you.
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#issuecomment-1830900823
Ack, will review this week.
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#issuecomment-1834166805
> Ack, will review this week.
Thanks.
Also, please include https://github.com/apache/iceberg/pull/9170 in the review
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#discussion_r1407512055
##########
api/src/main/java/org/apache/iceberg/PartitionStatisticsFile.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.io.Serializable;
+
+/**
+ * Represents a partition statistics file in the table default format, that can be used to read
+ * table data more efficiently.
+ *
+ * <p>Statistics are informational. A reader can choose to ignore statistics information. Statistics
+ * support is not required to read the table correctly.
+ */
+public interface PartitionStatisticsFile extends Serializable {
Review Comment:
As per the spec
https://github.com/apache/iceberg/blob/main/format/spec.md#partition-statistics
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#discussion_r1427926385
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1288,6 +1307,24 @@ public Builder suppressHistoricalSnapshots() {
return this;
}
+ public Builder setPartitionStatistics(PartitionStatisticsFile partitionStatisticsFile) {
+ Preconditions.checkNotNull(partitionStatisticsFile, "partition statistics file is null");
+ partitionStatisticsFiles.put(
+ partitionStatisticsFile.snapshotId(), ImmutableList.of(partitionStatisticsFile));
+ changes.add(new MetadataUpdate.SetPartitionStatistics(partitionStatisticsFile));
+ return this;
+ }
+
+ public Builder removePartitionStatistics(long snapshotId) {
+ Preconditions.checkNotNull(snapshotId, "snapshotId is null");
Review Comment:
True. I copy pasted from existing `removeStatistics` which has this problem. I overlooked or assumed things are correct. I will be careful next time.
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#discussion_r1650705615
##########
core/src/main/java/org/apache/iceberg/TableMetadataParser.java:
##########
@@ -481,6 +488,13 @@ public static TableMetadata fromJson(String metadataLocation, JsonNode node) {
statisticsFiles = ImmutableList.of();
}
+ List<PartitionStatisticsFile> partitionStatisticsFiles;
+ if (node.has(PARTITION_STATISTICS)) {
+ partitionStatisticsFiles = partitionStatsFilesFromJson(node.get(PARTITION_STATISTICS));
+ } else {
+ partitionStatisticsFiles = ImmutableList.of();
+ }
+
Review Comment:
Hi @ajantha-bhat and @aokolnychyi, I have a question about this implementation as I'm exploring to add new fields into TableMetadata. Suppose the table `db.table`'s partition stats is updated by the new version of Iceberg via UpdatePartitionStatistics. After that, some old version of Iceberg library or the PyIceberg client produces a new commit to this table. Per my understanding, that writer will produce TableMetadata without `PARTITION_STATISTICS` since it knows nothing about `PARTITION_STATISTICS`, which effectively loses that info for the table.
Do you have any solutions or ideas on how to prevent such cases? I can think of some potential ideas, such as:
1. upgrade the format_version to a new one whenever we need to add new fields to table metadata, all the old clients will be rejected by the version check then.
2. define a writer_version field, old client can read metadata produced by new client, but it will reject writers with old versions.
3. move the check to the REST catalog service?
I feel it's too heavy to do a format upgrade when only adding new fields in TableMetadata.
Do you have any other ideas? Really appreciate your inputs.
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#discussion_r1427954785
##########
api/src/main/java/org/apache/iceberg/PartitionStatisticsFile.java:
##########
@@ -0,0 +1,38 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.io.Serializable;
+
+/**
+ * Represents a partition statistics file that can be used to read table data more efficiently.
+ *
+ * <p>Statistics are informational. A reader can choose to ignore statistics information. Statistics
+ * support is not required to read the table correctly.
+ */
+public interface PartitionStatisticsFile extends Serializable {
Review Comment:
Ack. We can add it back if required during an end to end implementation.
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -946,15 +955,25 @@ private Builder(TableMetadata base) {
this.previousFileLocation = base.metadataFileLocation;
this.previousFiles = base.previousFiles;
this.refs = Maps.newHashMap(base.refs);
- this.statisticsFiles =
- base.statisticsFiles.stream().collect(Collectors.groupingBy(StatisticsFile::snapshotId));
-
+ this.statisticsFiles = statsFileBySnapshotID(base);
+ this.partitionStatisticsFiles = partitionStatsFileBySnapshotID(base);
this.snapshotsById = Maps.newHashMap(base.snapshotsById);
this.schemasById = Maps.newHashMap(base.schemasById);
this.specsById = Maps.newHashMap(base.specsById);
this.sortOrdersById = Maps.newHashMap(base.sortOrdersById);
}
+ private static Map<Long, List<StatisticsFile>> statsFileBySnapshotID(TableMetadata base) {
Review Comment:
moved
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat closed pull request #8502: Api: Track partition statistics via TableMetadata
URL: https://github.com/apache/iceberg/pull/8502
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#discussion_r1427955084
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -946,15 +955,25 @@ private Builder(TableMetadata base) {
this.previousFileLocation = base.metadataFileLocation;
this.previousFiles = base.previousFiles;
this.refs = Maps.newHashMap(base.refs);
- this.statisticsFiles =
- base.statisticsFiles.stream().collect(Collectors.groupingBy(StatisticsFile::snapshotId));
-
+ this.statisticsFiles = statsFileBySnapshotID(base);
+ this.partitionStatisticsFiles = partitionStatsFileBySnapshotID(base);
this.snapshotsById = Maps.newHashMap(base.snapshotsById);
this.schemasById = Maps.newHashMap(base.schemasById);
this.specsById = Maps.newHashMap(base.specsById);
this.sortOrdersById = Maps.newHashMap(base.sortOrdersById);
}
+ private static Map<Long, List<StatisticsFile>> statsFileBySnapshotID(TableMetadata base) {
+ return base.statisticsFiles.stream()
+ .collect(Collectors.groupingBy(StatisticsFile::snapshotId));
+ }
+
+ private static Map<Long, List<PartitionStatisticsFile>> partitionStatsFileBySnapshotID(
Review Comment:
updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#issuecomment-1857853254
@aokolnychyi: I have handled all the new comments. Thanks again for the review.
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#discussion_r1422541957
##########
core/src/main/java/org/apache/iceberg/ReachableFileUtil.java:
##########
@@ -137,7 +137,9 @@ public static List<String> manifestListLocations(Table table, Set<Long> snapshot
*
* @param table table for which statistics files needs to be listed
* @return the location of statistics files
+ * @deprecated use the {@code allStatisticsFilesLocations(table)} instead.
*/
+ @Deprecated
Review Comment:
removed the changes in this file as the new APIs added was not called from anywhere (this is needed for spark action).
I will raise a follow up PR for Spark actions to consider partition stats file for remove orphan files and expire snapshots action.
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#discussion_r1422543077
##########
.palantir/revapi.yml:
##########
@@ -873,6 +873,10 @@ acceptedBreaks:
new: "method void org.apache.iceberg.encryption.Ciphers::<init>()"
justification: "Static utility class - should not have public constructor"
"1.4.0":
+ org.apache.iceberg:iceberg-api:
+ - code: "java.method.addedToInterface"
Review Comment:
added as default
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#issuecomment-1851876373
@aokolnychyi: Fixed all the comments and also opened a new Spark module PR (which is dependent on this) to ensure partition stats are considered for GC (expire snapshots and remove orphan files). I will rebase that PR once this is merged.
https://github.com/apache/iceberg/pull/9284
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#discussion_r1412672873
##########
.palantir/revapi.yml:
##########
@@ -873,6 +873,10 @@ acceptedBreaks:
new: "method void org.apache.iceberg.encryption.Ciphers::<init>()"
justification: "Static utility class - should not have public constructor"
"1.4.0":
+ org.apache.iceberg:iceberg-api:
+ - code: "java.method.addedToInterface"
Review Comment:
I know we did a similar breaking change when adding table stats. I wonder whether that was the correct decision, however. Why not return an empty list by default?
##########
api/src/main/java/org/apache/iceberg/PartitionStatisticsFile.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.io.Serializable;
+
+/**
+ * Represents a partition statistics file in the table default format, that can be used to read
Review Comment:
I wonder whether `in the table default format` will always be accurate. What about just this?
```
Represents a partition statistics file that can be used to read table data more efficiently.
```
##########
core/src/main/java/org/apache/iceberg/SetPartitionStatistics.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+
+public class SetPartitionStatistics implements UpdatePartitionStatistics {
+ private final TableOperations ops;
+ private final Map<Long, Optional<PartitionStatisticsFile>> partitionStatisticsToSet =
Review Comment:
I am not sure how I feel about using a map and giving `Optional` a special meaning. Would having a map of stats to set and a separate set of snapshot IDs to remove easier to read/understand? What do you think, @ajantha-bhat?
```
private TableMetadata internalApply(TableMetadata base) {
TableMetadata.Builder builder = TableMetadata.buildFrom(base);
toSet.forEach(builder::setPartitionStatistics);
toRemove.forEach(builder::removePartitionStatistics);
return builder.build();
}
```
##########
api/src/main/java/org/apache/iceberg/PartitionStatisticsFile.java:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.io.Serializable;
+
+/**
+ * Represents a partition statistics file in the table default format, that can be used to read
+ * table data more efficiently.
+ *
+ * <p>Statistics are informational. A reader can choose to ignore statistics information. Statistics
+ * support is not required to read the table correctly.
+ */
+public interface PartitionStatisticsFile extends Serializable {
+ /** ID of the Iceberg table's snapshot the partition statistics file is associated with. */
+ long snapshotId();
+
+ /**
+ * Returns fully qualified path to the file, suitable for constructing a Hadoop Path. Never null.
Review Comment:
I wouldn't encourage using Hadoop `Path` given that we have our own `FileIO` and `InputFile`.
What about dropping `suitable for constructing a Hadoop Path`?
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -948,6 +957,9 @@ private Builder(TableMetadata base) {
this.refs = Maps.newHashMap(base.refs);
this.statisticsFiles =
base.statisticsFiles.stream().collect(Collectors.groupingBy(StatisticsFile::snapshotId));
+ this.partitionStatisticsFiles =
+ base.partitionStatisticsFiles.stream()
Review Comment:
I wonder whether a helper method would make it easier to read.
##########
core/src/test/resources/TableMetadataPartitionStatisticsFiles.json:
##########
@@ -0,0 +1,61 @@
+{
+ "format-version": 2,
+ "table-uuid": "9c12d441-03fe-4693-9a96-a0705ddf69c1",
+ "location": "s3://bucket/test/location",
+ "last-sequence-number": 34,
+ "last-updated-ms": 1602638573590,
+ "last-column-id": 3,
+ "current-schema-id": 0,
+ "schemas": [
+ {
+ "type": "struct",
+ "schema-id": 0,
+ "fields": [
+ {
+ "id": 1,
+ "name": "x",
+ "required": true,
+ "type": "long"
+ }
+ ]
+ }
+ ],
+ "default-spec-id": 0,
+ "partition-specs": [
+ {
+ "spec-id": 0,
+ "fields": []
+ }
+ ],
+ "last-partition-id": 1000,
+ "default-sort-order-id": 0,
+ "sort-orders": [
+ {
+ "order-id": 0,
+ "fields": []
+ }
+ ],
+ "properties": {},
+ "current-snapshot-id": 3055729675574597004,
+ "snapshots": [
+ {
+ "snapshot-id": 3055729675574597004,
+ "timestamp-ms": 1555100955770,
+ "sequence-number": 1,
+ "summary": {
+ "operation": "append"
+ },
+ "manifest-list": "s3://a/b/2.avro",
+ "schema-id": 0
+ }
+ ],
+ "partition-statistics": [
+ {
+ "snapshot-id": 3055729675574597004,
+ "statistics-path": "s3://a/b/partition-stats.parquet",
+ "file-size-in-bytes": 43
+ }
+ ],
+ "snapshot-log": [],
+ "metadata-log": []
+}
Review Comment:
Missing empty line?
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -262,7 +262,8 @@ public void cleanFiles(TableMetadata beforeExpiration, TableMetadata afterExpira
deleteFiles(manifestsToDelete, "manifest");
deleteFiles(manifestListsToDelete, "manifest list");
- if (!beforeExpiration.statisticsFiles().isEmpty()) {
+ if (!beforeExpiration.statisticsFiles().isEmpty()
Review Comment:
What about a helper method like `hasStatsFiles` or something to simplify the condition and stay on 1 line?
##########
core/src/main/java/org/apache/iceberg/ReachableFileCleanup.java:
##########
@@ -82,7 +82,8 @@ public void cleanFiles(TableMetadata beforeExpiration, TableMetadata afterExpira
deleteFiles(manifestListsToDelete, "manifest list");
- if (!beforeExpiration.statisticsFiles().isEmpty()) {
+ if (!beforeExpiration.statisticsFiles().isEmpty()
Review Comment:
A helper method here too?
##########
api/src/main/java/org/apache/iceberg/UpdatePartitionStatistics.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.List;
+
+/** API for updating partition statistics files in a table. */
+public interface UpdatePartitionStatistics extends PendingUpdate<List<PartitionStatisticsFile>> {
+ /**
+ * Set the table's partition statistics file for given snapshot, replacing the previous partition
+ * statistics file for the snapshot if any exists.
+ *
+ * @return this for method chaining
+ */
+ UpdatePartitionStatistics setPartitionStatistics(
+ long snapshotId, PartitionStatisticsFile partitionStatisticsFile);
Review Comment:
Hm, `PartitionStatisticsFile` already returns `snapshotId()`.
Why also pass it explicitly and validate they are equal in the implementation?
Seems to match what we do for `UpdateStatistics`, though.
##########
core/src/main/java/org/apache/iceberg/ReachableFileUtil.java:
##########
@@ -137,7 +137,9 @@ public static List<String> manifestListLocations(Table table, Set<Long> snapshot
*
* @param table table for which statistics files needs to be listed
* @return the location of statistics files
+ * @deprecated use the {@code allStatisticsFilesLocations(table)} instead.
*/
+ @Deprecated
Review Comment:
Given the purpose of this method and how it is used, what about simply extending it to return all statistics files? Then we won't need changes in consumers and won't need another method. We can clarify that this returns all stats files in Javadoc.
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#issuecomment-1841336236
I started looking on Friday but got distracted. I will try to finish by end of Wed.
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi merged PR #8502:
URL: https://github.com/apache/iceberg/pull/8502
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#discussion_r1423873333
##########
core/src/main/java/org/apache/iceberg/SetPartitionStatistics.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+
+public class SetPartitionStatistics implements UpdatePartitionStatistics {
+ private final TableOperations ops;
+ private final Map<Long, Optional<PartitionStatisticsFile>> partitionStatisticsToSet =
Review Comment:
updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#discussion_r1423873054
##########
core/src/main/java/org/apache/iceberg/ReachableFileUtil.java:
##########
@@ -137,7 +137,9 @@ public static List<String> manifestListLocations(Table table, Set<Long> snapshot
*
* @param table table for which statistics files needs to be listed
* @return the location of statistics files
+ * @deprecated use the {@code allStatisticsFilesLocations(table)} instead.
*/
+ @Deprecated
Review Comment:
Since, it was needed only for spark action, moved these changes into a separate PR with the tests for expire snapshots and remove orphan files.
https://github.com/apache/iceberg/pull/9284
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#issuecomment-1858140592
Re-triggering build due to flaky test in Flink
https://github.com/apache/iceberg/actions/runs/7222400549/job/19679307743?pr=8502
--
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] Api: Track partition statistics via TableMetadata [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #8502:
URL: https://github.com/apache/iceberg/pull/8502#discussion_r1430591144
##########
core/src/main/java/org/apache/iceberg/SetPartitionStatistics.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+
+public class SetPartitionStatistics implements UpdatePartitionStatistics {
Review Comment:
Does this have to be public?
--
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