Skip to content

Core: Enable column statistics filtering after planning #8803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aokolnychyi merged 10 commits into from
Nov 14, 2023
Merged

Core: Enable column statistics filtering after planning #8803

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5bb425d
Colstat
May 16, 2023
7a3effa
Stevent's comments
Oct 19, 2023
ae5975c
Steven's comments 2nd round
Oct 24, 2023
be9a11e
Remove flink conf value
Oct 24, 2023
c8061f3
Some more comments
Oct 25, 2023
d0fcac2
Use containsKey to check map availability
Oct 27, 2023
9872f1f
Ryan's and Anton's comments
Nov 6, 2023
f6279d0
Anton's comments
Nov 10, 2023
32d2645
Serialization change accepted
Nov 10, 2023
1922ae0
Anton's comments
Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Stevent's comments
  • Loading branch information
Peter Vary committed Oct 19, 2023
commit 7a3effab6cc535aafbf5bdf67de91f6e7396680d
3 changes: 2 additions & 1 deletion api/src/main/java/org/apache/iceberg/BatchScanAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.apache.iceberg;

import java.util.Collection;
import java.util.Set;
import java.util.concurrent.ExecutorService;
import org.apache.iceberg.expressions.Expression;
import org.apache.iceberg.io.CloseableIterable;
Expand Down Expand Up @@ -84,7 +85,7 @@ public BatchScan includeColumnStats() {
}

@Override
public BatchScan includeColumnStats(Collection<Integer> columns) {
public BatchScan includeColumnStats(Set<Integer> columns) {
return new BatchScanAdapter(scan.includeColumnStats(columns));
}

Expand Down
31 changes: 5 additions & 26 deletions api/src/main/java/org/apache/iceberg/ContentFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@
package org.apache.iceberg;

import java.nio.ByteBuffer;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Set;

/**
* Superinterface of {@link DataFile} and {@link DeleteFile} that exposes common methods.
Expand Down Expand Up @@ -168,13 +168,14 @@ default Long fileSequenceNumber() {

/**
* Copies this file with only specific column stats. Manifest readers can reuse file instances;
Comment thread
pvary marked this conversation as resolved.
Outdated
* use this method to copy data and only copy specific stats when collecting files.
* use this method to copy data and only copy specific stats when collecting files. If the
Comment thread
pvary marked this conversation as resolved.
Outdated
* columnsToKeepStats set is empty or <code>null</code>, then all column stats will be kept.
*
* @param statsToKeep the collection of the column ids for the columns which stats are kept
* @param columnsToKeepStats the set of the column ids for the columns which stats are kept.
* @return a copy of this data file, with stats lower bounds, upper bounds, value counts, null
Comment thread
pvary marked this conversation as resolved.
Outdated
Comment thread
pvary marked this conversation as resolved.
Outdated
* value counts, and nan value counts for only specific columns.
*/
default F copyWithSpecificStats(Collection<Integer> statsToKeep) {
default F copyWithStats(Set<Integer> columnsToKeepStats) {

@aokolnychyi aokolnychyi Nov 4, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide to do this rename, we should probably be consistent here as well.

throw new UnsupportedOperationException(
this.getClass().getName() + " doesn't implement copyWithSpecificStats");
Comment thread
pvary marked this conversation as resolved.
Outdated
}
Expand All @@ -191,26 +192,4 @@ default F copyWithSpecificStats(Collection<Integer> statsToKeep) {
default F copy(boolean withStats) {
return withStats ? copy() : copyWithoutStats();
}

/**
* Copies this file (potentially with or without specific column stats). Manifest readers can
* reuse file instances; use this method to copy data when collecting files from tasks.
*
* @param withStats Will copy this file without file stats if set to <code>false</code>.
* @param statsToKeep Will keep stats only for these columns. Not used if <code>withStats</code>
* is set to <code>false</code>.
* @return a copy of this data file. If "withStats" is set to <code>false</code> the file will not
* contain lower bounds, upper bounds, value counts, null value counts, or nan value counts.
* If "withStats" is set to <code>true</code> and the "statsToKeep" is not empty then only
* specific column stats will be kept.
*/
default F copy(boolean withStats, Collection<Integer> statsToKeep) {
if (withStats) {
return statsToKeep != null && !statsToKeep.isEmpty()
? copyWithSpecificStats(statsToKeep)
: copy();
} else {
return copyWithoutStats();
}
}
}
6 changes: 4 additions & 2 deletions api/src/main/java/org/apache/iceberg/Scan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.apache.iceberg;

import java.util.Collection;
import java.util.Set;
import java.util.concurrent.ExecutorService;
import org.apache.iceberg.expressions.Expression;
import org.apache.iceberg.io.CloseableIterable;
Expand Down Expand Up @@ -79,14 +80,15 @@ public interface Scan<ThisT, T extends ScanTask, G extends ScanTaskGroup<T>> {

/**
* Create a new scan from this that loads the column stats for the specific columns with each data
* file.
* file. If the columns set is empty or <code>null</code> then all column stats will be kept, if
* {@link #includeColumnStats()} is set.
*
* <p>Column stats include: value count, null value count, lower bounds, and upper bounds.
*
* @param columns column ids from the table's schema
* @return a new scan based on this that loads column stats for specific columns.
*/
default ThisT includeColumnStats(Collection<Integer> columns) {
default ThisT includeColumnStats(Set<Integer> columns) {
throw new UnsupportedOperationException(
this.getClass().getName() + " doesn't implement includeColumnStats");
}
Expand Down
46 changes: 46 additions & 0 deletions api/src/main/java/org/apache/iceberg/util/ContentFileUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* 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.util;

import java.util.Set;
import org.apache.iceberg.ContentFile;

public class ContentFileUtil {
Comment thread
pvary marked this conversation as resolved.
private ContentFileUtil() {}

/**
* Copies the {@link ContentFile} with the specific stat settings.
*
* @param file a generic data file to copy.
* @param withStats whether to keep any stats
* @param statsToKeep a set of column ids to keep stats. If empty or <code>null</code> then every
* column stat is kept.
* @return The copied file
*/
public static <F extends ContentFile<K>, K> K copy(
F file, boolean withStats, Set<Integer> statsToKeep) {
Comment thread
pvary marked this conversation as resolved.
Outdated
if (withStats) {
return statsToKeep != null && !statsToKeep.isEmpty()
? file.copyWithStats(statsToKeep)
: file.copy();
} else {
return file.copyWithoutStats();
}
}
}
4 changes: 2 additions & 2 deletions api/src/test/java/org/apache/iceberg/TestHelpers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@
import java.lang.invoke.SerializedLambda;
import java.nio.ByteBuffer;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.IntStream;
import org.apache.iceberg.expressions.BoundPredicate;
import org.apache.iceberg.expressions.BoundSetPredicate;
Expand Down Expand Up @@ -664,7 +664,7 @@ public DataFile copyWithoutStats() {
}

@Override
public DataFile copyWithSpecificStats(Collection<Integer> statsToKeep) {
public DataFile copyWithStats(Set<Integer> statsToKeep) {
Comment thread
pvary marked this conversation as resolved.
Outdated
return this;
}

Expand Down
4 changes: 3 additions & 1 deletion core/src/main/java/org/apache/iceberg/BaseDistributedDataScan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.apache.iceberg.metrics.ScanMetricsUtil;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.util.ContentFileUtil;
import org.apache.iceberg.util.ParallelIterable;
import org.apache.iceberg.util.TableScanUtil;
import org.apache.iceberg.util.ThreadPools;
Expand Down Expand Up @@ -369,7 +370,8 @@ private CloseableIterable<ScanTask> toFileTasks(

return new BaseFileScanTask(
copyDataFiles
Comment thread
pvary marked this conversation as resolved.
Outdated
? dataFile.copy(shouldReturnColumnStats(), columnStatsToInclude())
? ContentFileUtil.copy(
dataFile, shouldReturnColumnStats(), columnsToIncludeStats())
: dataFile,
deleteFiles,
schemaString,
Expand Down
72 changes: 32 additions & 40 deletions core/src/main/java/org/apache/iceberg/BaseFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,12 @@
import java.io.Serializable;
import java.nio.ByteBuffer;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.function.Function;
import org.apache.avro.Schema;
import org.apache.avro.generic.IndexedRecord;
import org.apache.avro.specific.SpecificData;
Expand Down Expand Up @@ -176,9 +177,10 @@ public PartitionData copy() {
*
* @param toCopy a generic data file to copy.
* @param fullCopy whether to copy all fields or to drop column-level stats
* @param statsToKeep the collection of the column ids for the columns which stats are kept
* @param statsToKeep a set of column ids to keep stats. If empty or <code>null</code> then every
* column stat is kept.
*/
BaseFile(BaseFile<F> toCopy, boolean fullCopy, Collection<Integer> statsToKeep) {
BaseFile(BaseFile<F> toCopy, boolean fullCopy, Set<Integer> statsToKeep) {
Comment thread
pvary marked this conversation as resolved.
Outdated
this.fileOrdinal = toCopy.fileOrdinal;
this.partitionSpecId = toCopy.partitionSpecId;
this.content = toCopy.content;
Expand All @@ -189,23 +191,24 @@ public PartitionData copy() {
this.recordCount = toCopy.recordCount;
this.fileSizeInBytes = toCopy.fileSizeInBytes;
if (fullCopy) {
if (statsToKeep == null || statsToKeep.isEmpty()) {
this.columnSizes = SerializableMap.copyOf(toCopy.columnSizes);
this.valueCounts = SerializableMap.copyOf(toCopy.valueCounts);
this.nullValueCounts = SerializableMap.copyOf(toCopy.nullValueCounts);
this.nanValueCounts = SerializableMap.copyOf(toCopy.nanValueCounts);
this.lowerBounds =
SerializableByteBufferMap.wrap(SerializableMap.copyOf(toCopy.lowerBounds));
this.upperBounds =
SerializableByteBufferMap.wrap(SerializableMap.copyOf(toCopy.upperBounds));
} else {
this.columnSizes = filteredLongMap(toCopy.columnSizes, statsToKeep);
this.valueCounts = filteredLongMap(toCopy.valueCounts, statsToKeep);
this.nullValueCounts = filteredLongMap(toCopy.nullValueCounts, statsToKeep);
this.nanValueCounts = filteredLongMap(toCopy.nanValueCounts, statsToKeep);
this.lowerBounds = filteredByteBufferMap(toCopy.lowerBounds, statsToKeep);
this.upperBounds = filteredByteBufferMap(toCopy.upperBounds, statsToKeep);
}
this.columnSizes =
filterColumnsStats(toCopy.columnSizes, statsToKeep, SerializableMap::copyOf);
this.valueCounts =
filterColumnsStats(toCopy.valueCounts, statsToKeep, SerializableMap::copyOf);
this.nullValueCounts =
filterColumnsStats(toCopy.nullValueCounts, statsToKeep, SerializableMap::copyOf);
this.nanValueCounts =
filterColumnsStats(toCopy.nanValueCounts, statsToKeep, SerializableMap::copyOf);
this.lowerBounds =
filterColumnsStats(
toCopy.lowerBounds,
statsToKeep,
m -> SerializableByteBufferMap.wrap(SerializableMap.copyOf(m)));
this.upperBounds =
filterColumnsStats(
toCopy.upperBounds,
statsToKeep,
m -> SerializableByteBufferMap.wrap(SerializableMap.copyOf(m)));
} else {
this.columnSizes = null;
this.valueCounts = null;
Expand Down Expand Up @@ -518,38 +521,27 @@ private static Map<Integer, ByteBuffer> toReadableByteBufferMap(Map<Integer, Byt
}
}

private static Map<Integer, Long> filteredLongMap(
Map<Integer, Long> map, Collection<Integer> columnIds) {
if (map == null) {
return null;
private static <TypeT> Map<Integer, TypeT> filterColumnsStats(
Comment thread
stevenzwu marked this conversation as resolved.
Outdated
Map<Integer, TypeT> map,
Set<Integer> columnIds,
Function<Map<Integer, TypeT>, Map<Integer, TypeT>> copyFunction) {
if (columnIds == null || columnIds.isEmpty()) {
return copyFunction.apply(map);
}

Map<Integer, Long> filtered = Maps.newHashMapWithExpectedSize(columnIds.size());
for (Integer columnId : columnIds) {
Long value = map.get(columnId);
if (value != null) {
filtered.put(columnId, value);
}
}

return SerializableMap.copyOf(filtered);
}

private static Map<Integer, ByteBuffer> filteredByteBufferMap(
Map<Integer, ByteBuffer> map, Collection<Integer> columnIds) {
if (map == null) {
Comment thread
stevenzwu marked this conversation as resolved.
Outdated
return null;
}

Map<Integer, ByteBuffer> filtered = Maps.newHashMapWithExpectedSize(columnIds.size());
Map<Integer, TypeT> filtered = Maps.newHashMapWithExpectedSize(columnIds.size());
Comment thread
stevenzwu marked this conversation as resolved.
Outdated
for (Integer columnId : columnIds) {
ByteBuffer value = map.get(columnId);
TypeT value = map.get(columnId);
if (value != null) {

@stevenzwu stevenzwu Oct 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it necessary to check null value?

@pvary pvary Oct 24, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I found this in some tests, that the map could contain null values

@stevenzwu stevenzwu Oct 24, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are null values in lower_bound or upper_bound, why do we want to filter them out? shouldn't we keep the same behavior when copying stats for selected columns.

@pvary pvary Oct 25, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My previous answer was not clear enough. Let me rephrase: "we can have maps where the columnId is not contained in the key list". In this case the map.get(columnId); returns null. To return the correct map, we should not add the key to the list in this case.

@stevenzwu stevenzwu Oct 26, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. but could lower_bound and upper_bound have null value? how do we distinguish btw these two scenario?

e.g. here is the description for lower_bound

Lower bound for the non-null, non-NaN values in the partition field, or null if all values are null or NaN [2]

@pvary pvary Oct 26, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some testing with Parquet files:

  • Used TestParquet to check what Metrics do we generate when we have only null values:

  @Test
  public void testNullMetricsForEmptyColumns() throws IOException {
    Schema schema = new Schema(optional(1, "longCol", Types.LongType.get()),
        optional(2, "nullCol", Types.LongType.get()));

    File file = createTempFile(temp);

    List<GenericData.Record> records = Lists.newArrayListWithCapacity(1);
    org.apache.avro.Schema avroSchema = AvroSchemaUtil.convert(schema.asStruct());

    GenericData.Record smallRecord = new GenericData.Record(avroSchema);
    smallRecord.put("longCol", 1L);
    records.add(smallRecord);


    write(
        file,
        schema,
        Collections.emptyMap(),
        ParquetAvroWriter::buildWriter,
        records.toArray(new GenericData.Record[] {}));

    InputFile inputFile = Files.localInput(file);
    Metrics metrics = ParquetUtil.fileMetrics(inputFile, MetricsConfig.getDefault());
    assertThat(metrics.nullValueCounts()).hasSize(2);
    assertThat(metrics.lowerBounds()).hasSize(1).containsKey(1);
    assertThat(metrics.upperBounds()).hasSize(1).containsKey(1);
  }

Notice, that the size of the lowerBounds and the upperBounds map is 1. There is no null value for column '2'

  • Just to double check, I have tested how we read files with missing column metrics (like the ones created above). I have used TestManifestReaderStats for this:
  @Test
  public void testReadFileWithMissingStatsIncludesFullStats() throws IOException {
    Metrics missingStats =
        new Metrics(
            3L, null,
            ImmutableMap.of(3, 3L, 4, 4L),
            ImmutableMap.of(3, 3L, 4, 0L),
            ImmutableMap.of(3, 0L, 4, 1L),
            ImmutableMap.of(4, Conversions.toByteBuffer(Types.StringType.get(), "a")),
            ImmutableMap.of(4, Conversions.toByteBuffer(Types.StringType.get(), "a")));
    DataFile fileWithMissingStats =
        DataFiles.builder(SPEC)
            .withPath(FILE_PATH)
            .withFileSizeInBytes(10)
            .withPartitionPath("data_bucket=0") // easy way to set partition data for now
            .withRecordCount(3)
            .withMetrics(missingStats)
            .build();
    ManifestFile manifest = writeManifest(1000L, fileWithMissingStats);
    try (ManifestReader<DataFile> reader = ManifestFiles.read(manifest, FILE_IO)) {
      CloseableIterable<ManifestEntry<DataFile>> entries = reader.entries();
      ManifestEntry<DataFile> entry = entries.iterator().next();
      DataFile dataFile = entry.file();
      Assert.assertEquals(3, dataFile.recordCount());
      assertThat(dataFile.lowerBounds()).hasSize(1).containsKey(4);
      assertThat(dataFile.upperBounds()).hasSize(1).containsKey(4);
    }
  }

Notice, that the size of the lowerBounds and the upperBounds map is 1. There is no null value for column '3'

So I think the actual phrasing of the documentation is just not clean enough, as the map does not contain null values, it just does not contain the keys. While putting null value to a map is certainly possible, it should be used only in very specific cases, as it is very error-prone (see our discussion above 😄). If our goal would be to identify the case where all of the values in the given column are null, we can use the other stats to archive the result.

@stevenzwu stevenzwu Oct 26, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the unit test code. I am still not comfortable of making the assumption as the Iceberg spec clearly said null stats value is possible. what if Trino parquet writer produces min/max stats with null value.

Lower bound for the non-null, non-NaN values in the partition field, or null if all values are null or NaN [2]

As least, there is a mismatch in the Parquet writer implementation and the spec. Unless we are going to clarify/update the spec, I feel it is safer to do map.containsKey(key).

Maybe @RussellSpitzer and @rdblue can chime in here.

@pvary pvary Oct 27, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to ensure that even null values are copied if we accept the cost of possible double fetching the entries from the map.

Still not sure that this is necessary, but I will be on PTO next week, so wanted to have it here as a possibility

@stevenzwu stevenzwu Oct 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. double fetching is probably ok since the columnsToKeepStats is likely not big and Map get is a O(1) anyway.

The alternative is to start a discussion and revisit the spec or Parquet implementation.

filtered.put(columnId, value);
}
}

return SerializableMap.copyOf(filtered);
return copyFunction.apply(filtered);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/apache/iceberg/BaseIncrementalAppendScan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ private CloseableIterable<FileScanTask> appendFilesFromSnapshots(List<Snapshot>
&& manifestEntry.status() == ManifestEntry.Status.ADDED)
.specsById(table().specs())
.ignoreDeleted()
.columnStatsToKeep(context().returnColumnStatsToInclude());
.columnStatsToKeep(context().columnsToIncludeStats());

if (context().ignoreResiduals()) {
manifestGroup = manifestGroup.ignoreResiduals();
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/apache/iceberg/BaseIncrementalChangelogScan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ protected CloseableIterable<ChangelogScanTask> doPlanFiles(
.filterData(filter())
.filterManifestEntries(entry -> changelogSnapshotIds.contains(entry.snapshotId()))
.ignoreExisting()
.columnStatsToKeep(context().returnColumnStatsToInclude());
.columnStatsToKeep(context().columnsToIncludeStats());

if (shouldIgnoreResiduals()) {
manifestGroup = manifestGroup.ignoreResiduals();
Expand Down
10 changes: 4 additions & 6 deletions core/src/main/java/org/apache/iceberg/BaseScan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,8 @@ protected boolean shouldReturnColumnStats() {
return context().returnColumnStats();
}

protected Collection<Integer> columnStatsToInclude() {
return context().returnColumnStatsToInclude();
protected Set<Integer> columnsToIncludeStats() {
return context().columnsToIncludeStats();
}

protected boolean shouldIgnoreResiduals() {
Expand Down Expand Up @@ -170,11 +170,9 @@ public ThisT includeColumnStats() {
}

@Override
public ThisT includeColumnStats(Collection<Integer> statsNeeded) {
public ThisT includeColumnStats(Set<Integer> statsNeeded) {
Comment thread
pvary marked this conversation as resolved.
Outdated
return newRefinedScan(
table,
schema,
context.shouldReturnColumnStats(true).shouldReturnSpecificColumnStats(statsNeeded));
table, schema, context.shouldReturnColumnStats(true).columnsToIncludeStats(statsNeeded));
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/apache/iceberg/DataScan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ protected ManifestGroup newManifestGroup(
.specsById(table().specs())
.scanMetrics(scanMetrics())
.ignoreDeleted()
.columnStatsToKeep(context().returnColumnStatsToInclude());
.columnStatsToKeep(context().columnsToIncludeStats());

if (shouldIgnoreResiduals()) {
manifestGroup = manifestGroup.ignoreResiduals();
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/apache/iceberg/DataTableScan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public CloseableIterable<FileScanTask> doPlanFiles() {
.specsById(table().specs())
.scanMetrics(scanMetrics())
.ignoreDeleted()
.columnStatsToKeep(context().returnColumnStatsToInclude());
.columnStatsToKeep(context().columnsToIncludeStats());

if (shouldIgnoreResiduals()) {
manifestGroup = manifestGroup.ignoreResiduals();
Expand Down
12 changes: 6 additions & 6 deletions core/src/main/java/org/apache/iceberg/GenericDataFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@
package org.apache.iceberg;

import java.nio.ByteBuffer;
import java.util.Collection;
import java.util.List;
import java.util.Set;
import org.apache.avro.Schema;
import org.apache.iceberg.avro.AvroSchemaUtil;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -68,11 +68,11 @@ class GenericDataFile extends BaseFile<DataFile> implements DataFile {
* Copy constructor.
*
* @param toCopy a generic data file to copy.
* @param fullCopy whether to copy all fields or to drop column-level stats
* @param statsToKeep column ids columns where we need to keep the stats
* @param fullCopy whether to copy all fields or to drop column-level stats.
* @param statsToKeep a set of column ids to keep stats. If empty or <code>null</code> then every
* column stat is kept.
*/
private GenericDataFile(
GenericDataFile toCopy, boolean fullCopy, Collection<Integer> statsToKeep) {
private GenericDataFile(GenericDataFile toCopy, boolean fullCopy, Set<Integer> statsToKeep) {
Comment thread
pvary marked this conversation as resolved.
Outdated
super(toCopy, fullCopy, statsToKeep);
}

Expand All @@ -85,7 +85,7 @@ public DataFile copyWithoutStats() {
}

@Override
public DataFile copyWithSpecificStats(Collection<Integer> statsToKeep) {
public DataFile copyWithStats(Set<Integer> statsToKeep) {
return new GenericDataFile(this, true, statsToKeep);
}

Expand Down
Loading