You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/01/29 05:32:47 UTC
[GitHub] [iceberg] PiyushHurpadeDremio opened a new pull request #2182: Support for PartitionStatsFile in each snapshot
PiyushHurpadeDremio opened a new pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182
This PR is contains implementation PartitionStatsFile. issue : #1833
Within each snapshot we will have "partition-stats-files" map which points to partition stats file location for each partition spec in this snapshot.
Schema of partitionstatsfile :
1. partition: This is a list of values for each partition column. PartitionData tuple based on partition specification.
2. file_count (int): Number of data files in this partition
3. row_count(long): Number of rows in table in this partition
See TestPartitionStatsMap.java which tests possible scenarios
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] PiyushHurpadeDremio commented on a change in pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
PiyushHurpadeDremio commented on a change in pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#discussion_r567571695
##########
File path: core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
##########
@@ -494,6 +499,48 @@ private ManifestFile newDeleteFilesAsManifest() {
return cachedNewDeleteManifest;
}
+ @Override
+ protected Map<Integer, String> updatePartitionStats() {
+ Map<Integer, String> newPartitionStatsLocations = new HashMap<>();
+
+ int currentSpecID = ops.current().spec().specId();
+
+ /**
+ * Update Partition Stats for Delete operation
+ */
+ PartitionStatsMap deletePartitionStatsMap = filterManager.getPartitionStatsMap();
+ deletePartitionStatsMap.getAllUpdatedPartitionSpec().forEach(
+ specId -> {
+ if (specId != currentSpecID) {
+ List<PartitionStatsEntry> statsEntries = getOldPartitionStatsBySpec(specId);
+ PartitionsTable.PartitionMap parMap = deletePartitionStatsMap.getPartitionMap(specId);
+ parMap.subtractFromPartitionStatsEntries(statsEntries);
+ OutputFile outputFile = newPartitionStatsFile(specId);
+ newPartitionStatsLocations.put(specId, outputFile.location());
+ writePartitionStatsEntries(parMap.getAllPartitionStatsEntries(), outputFile, specId);
Review comment:
We are re-using partition stats from the previous snapshot only. this loop iterate over only specifications that got updated as a part of deleteFile(CharSequence path), deleteFile(DataFile file), deleteFromRowFilter(Expression expr) operation. deletePartitionStatsMap.getAllUpdatedPartitionSpec() this will return only partition specifications that got 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.
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
[GitHub] [iceberg] PiyushHurpadeDremio commented on pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
PiyushHurpadeDremio commented on pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#issuecomment-771579446
@rdblue Can you please review this PR. This PR is implementation of issue [#1833 ](https://github.com/apache/iceberg/issues/1833). Briefly, this PR implements support for partition stats. In each snapshot we will have partition stats file for each partition spec. If no partition stats are updated for partition spec in new snapshot, we will point it to older partition stats file for that partition_stats_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.
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
[GitHub] [iceberg] vvellanki commented on pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
vvellanki commented on pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#issuecomment-773063209
@rdblue I did some experimentation with the approach you outlined by invoking planFiles(). I found that the time taken to read the manifest list file and the manifest files was quite high.
Also, the time taken increases when tables have a large number of columns (> 1000) and there are per-column stats for each of the data files (string columns increase the size of the per-column stats). This increases the number of manifest files that need to be read as part of this exercise and also increases the amount of time taken.
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] vvellanki commented on a change in pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
vvellanki commented on a change in pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#discussion_r567738918
##########
File path: core/src/main/java/org/apache/iceberg/PartitionStatsMap.java
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.HashSet;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+
+/**
+ * @ParitionStatsMap Instance of this class holds updated delta information, this can be used for both delete and
+ * inserts.
+ */
+public class PartitionStatsMap {
+ private final Map<Integer, PartitionSpec> specsById;
+
+ private Map<Integer, PartitionsTable.PartitionMap> partitionSpecMap = Maps.newHashMap();
+ private Set<Integer> partitionSpecUpdated = new HashSet<>();
+
+ public PartitionStatsMap(Map<Integer, PartitionSpec> specsById) {
+ this.specsById = specsById;
+ }
+
+ void put(int partitionSpecId, StructLike pd, DataFile dataFile) {
Review comment:
Not synchronised?
##########
File path: core/src/main/java/org/apache/iceberg/SnapshotProducer.java
##########
@@ -150,6 +153,44 @@ protected void validate(TableMetadata currentMetadata) {
*/
protected abstract List<ManifestFile> apply(TableMetadata metadataToUpdate);
+ /**
+ Implementation of this method update stats for each partition and
+ returns Map which defines partition specification for which udpates happen
+ Map Key: Partition Specification Id Map Value: New location of partition Stats file
+ */
+ protected Map<Integer, String> updatePartitionStats() {
Review comment:
This looks wierd, why is an update method not doing anything?
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] rdblue commented on pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#issuecomment-772044013
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] rdblue commented on pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#issuecomment-773474731
@vvellanki, what I'm suggesting is to plan early and use the actual stats from the splits. You will need to do the planning at some point either way because you'll need to dispatch a task for each input split.
As for the time taken to plan a scan with more columns, you should take a look at the table properties to control metrics that are kept in table metadata. The [metrics properties start with `write.metadata`](http://iceberg.apache.org/configuration/#write-properties).
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] rdblue commented on pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#issuecomment-773474731
@vvellanki, what I'm suggesting is to plan early and use the actual stats from the splits. You will need to do the planning at some point either way because you'll need to dispatch a task for each input split.
As for the time taken to plan a scan with more columns, you should take a look at the table properties to control metrics that are kept in table metadata. The [metrics properties start with `write.metadata`](http://iceberg.apache.org/configuration/#write-properties).
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] vvellanki commented on a change in pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
vvellanki commented on a change in pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#discussion_r566744926
##########
File path: core/src/main/java/org/apache/iceberg/FastAppend.java
##########
@@ -147,6 +152,22 @@ private ManifestFile copyManifest(ManifestFile manifest) {
return newManifests;
}
+ @Override
+ protected Map<Integer, String> updatePartitionStats() {
+ Map<Integer, String> newPartitionStatsLocations = new HashMap<>();
+ int currentSpecID = ops.current().spec().specId();
+ List<PartitionStatsEntry> oldStatsEntries = getOldPartitionStatsBySpec(currentSpecID);
+ PartitionsTable.PartitionMap partitionMap = partitionStatsMap.getPartitionMap(currentSpecID);
+ if (Objects.isNull(partitionMap)) {
+ partitionMap = partitionStatsMap.getEmptyPartitionMap(currentSpecID);
Review comment:
The transaction might be a schema change transaction (like adding a new column). Does this require creating a new partition stats for the current specif?
##########
File path: core/src/main/java/org/apache/iceberg/GenericPartitionStatsEntry.java
##########
@@ -0,0 +1,121 @@
+/*
+ * 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;
+import org.apache.avro.Schema;
+import org.apache.avro.generic.IndexedRecord;
+import org.apache.avro.specific.SpecificData;
+import org.apache.iceberg.avro.AvroSchemaUtil;
+
+public class GenericPartitionStatsEntry implements PartitionStatsEntry, StructLike, IndexedRecord,
+ SpecificData.SchemaConstructable, Serializable {
+
+ private final org.apache.avro.Schema schema;
+ private PartitionData partitionData = null;
+ private int fileCount;
+ private long rowCount;
+
+ GenericPartitionStatsEntry(org.apache.avro.Schema schema) {
+ this.schema = schema;
+ }
+
+ public GenericPartitionStatsEntry(PartitionData partitionData, int fileCount, long rowCount) {
+ this.schema = AvroSchemaUtil.convert(PartitionStatsEntry.getSchema(partitionData.getPartitionType()),
+ "partitionStatsEntry");
+ this.partitionData = partitionData;
+ this.fileCount = fileCount;
+ this.rowCount = rowCount;
+ }
+
+ @Override
+ public void put(int i, Object v) {
+ switch (i) {
+ case 0:
Review comment:
Can you define enums for these instead of using 0, 1 and 2?
##########
File path: core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
##########
@@ -494,6 +499,48 @@ private ManifestFile newDeleteFilesAsManifest() {
return cachedNewDeleteManifest;
}
+ @Override
+ protected Map<Integer, String> updatePartitionStats() {
+ Map<Integer, String> newPartitionStatsLocations = new HashMap<>();
+
+ int currentSpecID = ops.current().spec().specId();
+
+ /**
+ * Update Partition Stats for Delete operation
+ */
+ PartitionStatsMap deletePartitionStatsMap = filterManager.getPartitionStatsMap();
+ deletePartitionStatsMap.getAllUpdatedPartitionSpec().forEach(
+ specId -> {
+ if (specId != currentSpecID) {
+ List<PartitionStatsEntry> statsEntries = getOldPartitionStatsBySpec(specId);
+ PartitionsTable.PartitionMap parMap = deletePartitionStatsMap.getPartitionMap(specId);
+ parMap.subtractFromPartitionStatsEntries(statsEntries);
+ OutputFile outputFile = newPartitionStatsFile(specId);
+ newPartitionStatsLocations.put(specId, outputFile.location());
+ writePartitionStatsEntries(parMap.getAllPartitionStatsEntries(), outputFile, specId);
Review comment:
Why are we not re-using the partition stats from the previous snapshot instead of creating a new one for each 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.
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
[GitHub] [iceberg] rdblue edited a comment on pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
rdblue edited a comment on pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#issuecomment-773474731
@vvellanki, what I'm suggesting is to plan early and use the actual stats from the splits. You will need to do the planning at some point either way because you'll need to dispatch a task for each input split.
As for the time taken to plan a scan with more columns, you should take a look at the table properties to control metrics that are kept in table metadata. The [metrics properties start with `write.metadata.metrics`](http://iceberg.apache.org/configuration/#write-properties).
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] rdblue edited a comment on pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
rdblue edited a comment on pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#issuecomment-773474731
@vvellanki, what I'm suggesting is to plan early and use the actual stats from the splits. You will need to do the planning at some point either way because you'll need to dispatch a task for each input split.
As for the time taken to plan a scan with more columns, you should take a look at the table properties to control metrics that are kept in table metadata. The [metrics properties start with `write.metadata.metrics`](http://iceberg.apache.org/configuration/#write-properties).
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] rdblue commented on pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#issuecomment-772044013
Thanks for pinging me, @PiyushHurpadeDremio. Looks like the next step is to agree on a design for tracking partition stats?
I see from the description that this is adding a file to each snapshot that has an entry per partition tuple that has file and record counts. Do you think that this is going to be sufficient for other partition stats as well? What is the underlying use case you're trying to solve with number of files and number of rows?
Would it also make sense to add partition-level stats like column lower/upper bounds, value counts, null counts, and NaN counts?
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] PiyushHurpadeDremio commented on a change in pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
PiyushHurpadeDremio commented on a change in pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#discussion_r567574823
##########
File path: core/src/main/java/org/apache/iceberg/FastAppend.java
##########
@@ -147,6 +152,22 @@ private ManifestFile copyManifest(ManifestFile manifest) {
return newManifests;
}
+ @Override
+ protected Map<Integer, String> updatePartitionStats() {
+ Map<Integer, String> newPartitionStatsLocations = new HashMap<>();
+ int currentSpecID = ops.current().spec().specId();
+ List<PartitionStatsEntry> oldStatsEntries = getOldPartitionStatsBySpec(currentSpecID);
+ PartitionsTable.PartitionMap partitionMap = partitionStatsMap.getPartitionMap(currentSpecID);
+ if (Objects.isNull(partitionMap)) {
+ partitionMap = partitionStatsMap.getEmptyPartitionMap(currentSpecID);
Review comment:
schema changes will be part of UpdateSchema.java interface. Above code is only for FastAppend operation. We don't require to create new stats file. This PR implementation will not create new partition stats file. as UpdateSchema only updates table metadata.
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] PiyushHurpadeDremio commented on pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
PiyushHurpadeDremio commented on pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#issuecomment-771579446
@rdblue Can you please review this PR. This PR is implementation of issue [#1833 ](https://github.com/apache/iceberg/issues/1833). Briefly, this PR implements support for partition stats. In each snapshot we will have partition stats file for each partition spec. If no partition stats are updated for partition spec in new snapshot, we will point it to older partition stats file for that partition_stats_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.
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
[GitHub] [iceberg] vvellanki commented on pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
vvellanki commented on pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#issuecomment-772380537
@rdblue The use case for tracking per-partition stats is mainly for planning purposes - estimating the number of rows (for join planning, and join ordering) and the number of files to read based on the dataset that needs to be read.
As of now, the submitted change rewrites only the required files. Are suggesting the following approach:
- Commit the transaction by including placeholders for the partition stats files
- Asynchronously write the partition stats files
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] PiyushHurpadeDremio commented on a change in pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
PiyushHurpadeDremio commented on a change in pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#discussion_r567849814
##########
File path: core/src/main/java/org/apache/iceberg/PartitionStatsMap.java
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.HashSet;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+
+/**
+ * @ParitionStatsMap Instance of this class holds updated delta information, this can be used for both delete and
+ * inserts.
+ */
+public class PartitionStatsMap {
+ private final Map<Integer, PartitionSpec> specsById;
+
+ private Map<Integer, PartitionsTable.PartitionMap> partitionSpecMap = Maps.newHashMap();
+ private Set<Integer> partitionSpecUpdated = new HashSet<>();
+
+ public PartitionStatsMap(Map<Integer, PartitionSpec> specsById) {
+ this.specsById = specsById;
+ }
+
+ void put(int partitionSpecId, StructLike pd, DataFile dataFile) {
Review comment:
Not used. deleted in updated code.
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] rdblue commented on pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#issuecomment-772786048
Thanks, @vvellanki. I think we will need to take a closer look at the plan for maintaining these files. I think we should track the last file and the snapshot it is based on, so we can apply diffs to it and update asynchronously.
For the use case, I'm curious why you aren't using the actual files? In Spark, we push filters down to Iceberg before producing stats. Then stats are based on the actual files that will be scanned, which can be significantly smaller than just partition-level stats. That allows a lot more joins to be converted to broadcast joins. Is it possible to push filters earlier in your job planning?
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] vvellanki commented on pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
vvellanki commented on pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#issuecomment-773063209
@rdblue I did some experimentation with the approach you outlined by invoking planFiles(). I found that the time taken to read the manifest list file and the manifest files was quite high.
Also, the time taken increases when tables have a large number of columns (> 1000) and there are per-column stats for each of the data files (string columns increase the size of the per-column stats). This increases the number of manifest files that need to be read as part of this exercise and also increases the amount of time taken.
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] rdblue commented on pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#issuecomment-772052442
I also think that we need to have a concrete plan for how these files are created and updated.
The number of partitions in a table could easily be a few million. That's quite a bit to write each time a snapshot is produced, since these files are immutable. And because these would be maintained by applying the changes from one to the next (rather than scanning all of the metadata) the cost would be to read an entire file and write an entire file. That's quite a bit of extra work that I don't think we want to require for each write to a table.
I think we would want to add a way to track a file and the snapshot for which it was produced, so that we can figure out what changed since and update it asynchronously.
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] PiyushHurpadeDremio commented on a change in pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
PiyushHurpadeDremio commented on a change in pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#discussion_r567574823
##########
File path: core/src/main/java/org/apache/iceberg/FastAppend.java
##########
@@ -147,6 +152,22 @@ private ManifestFile copyManifest(ManifestFile manifest) {
return newManifests;
}
+ @Override
+ protected Map<Integer, String> updatePartitionStats() {
+ Map<Integer, String> newPartitionStatsLocations = new HashMap<>();
+ int currentSpecID = ops.current().spec().specId();
+ List<PartitionStatsEntry> oldStatsEntries = getOldPartitionStatsBySpec(currentSpecID);
+ PartitionsTable.PartitionMap partitionMap = partitionStatsMap.getPartitionMap(currentSpecID);
+ if (Objects.isNull(partitionMap)) {
+ partitionMap = partitionStatsMap.getEmptyPartitionMap(currentSpecID);
Review comment:
schema changes will be part of UpdateSchema.java interface. Above code is only for FastAppend operation. We don't require to create new stats file in schema update operation. This PR implementation will not create new partition stats file. as UpdateSchema only updates table metadata.
----------------------------------------------------------------
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.
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
[GitHub] [iceberg] PiyushHurpadeDremio commented on a change in pull request #2182: Support for PartitionStatsFile in each snapshot
Posted by GitBox <gi...@apache.org>.
PiyushHurpadeDremio commented on a change in pull request #2182:
URL: https://github.com/apache/iceberg/pull/2182#discussion_r567853904
##########
File path: core/src/main/java/org/apache/iceberg/SnapshotProducer.java
##########
@@ -150,6 +153,44 @@ protected void validate(TableMetadata currentMetadata) {
*/
protected abstract List<ManifestFile> apply(TableMetadata metadataToUpdate);
+ /**
+ Implementation of this method update stats for each partition and
+ returns Map which defines partition specification for which udpates happen
+ Map Key: Partition Specification Id Map Value: New location of partition Stats file
+ */
+ protected Map<Integer, String> updatePartitionStats() {
Review comment:
Defined this method as "abstract". Any implementation of updatePartitionStats method in SnapshotProducer.java which does not want to updated stats. needs to return empty map.
----------------------------------------------------------------
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.
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