Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* 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.spark.source;

import java.text.NumberFormat;
import org.apache.spark.sql.connector.metric.CustomMetric;

public class NumFiles implements CustomMetric {

@Override
public String name() {
return "numFiles";
}

@Override
public String description() {
return "number of files read";
}

@Override
public String aggregateTaskMetrics(long[] taskMetrics) {
long sum = initialValue;
for (int i = 0; i < taskMetrics.length; i++) {
sum += taskMetrics[i];
}
return NumberFormat.getIntegerInstance().format(sum);
}
Comment on lines +38 to +

@singhpk234 singhpk234 Mar 25, 2022

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.

[question] should we make this return "total(min, med, max)" we already have all the info required, this should give us even deeper insights on files distribution at task level. WDYT ?

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.

@singhpk234 thank you for your interesting suggesions!
I am open to presenting the min, median and max as well, but I wonder how useful those statistics really are. My impression is that Iceberg scan planning works well enough that the number of files to scan are usually very evenly distributed (except for cases like where there are two InputPartitions). Usually skew occurs with times (which can of course show great variation), or with number of records after a shuffle (rather than a BatchScan).
@rdblue, @aokolnychyi, @RussellSpitzer what do you think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I though the way these are shown in the UI are per task as well? Or am I remembering that wrong. IE we should have all these values (but not the calculated sum?) If not I think that would be good to report, I would suggest probably

min, 25percentile, median, 75percentile, max?

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.

@RussellSpitzer I think you may be thinking of task metrics that Spark itself collects (by default, Duration, GC Time, Input Size / Records, and Shuffle Size /Records are shown), and are shown with the min, 25th percentile, median, 75th percentile, and max.
For custom SQLMetrics, one implements the aggregateTaskMetrics and we'd need to do the calculation ourselves if we want such statistics. Spark doesn't do it for us. Spark sends all the values it collected from all the PartitionReaders for a given custom metric in a long[] to this aggregateTaskMetrics; that's all. It also doesn't separately calculate these statistics either.

}
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ class SparkBatchQueryScan extends SparkScan implements SupportsRuntimeFiltering
}
}

@Override
Comment thread
wypoon marked this conversation as resolved.
Outdated
public String description() {
return "IcebergScan " + super.description();
}

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.

Unfortunately the label for the node in the SQL DAG in the Spark UI just says "BatchScan" (that is the nodeName for a BatchScanExec node, and we can't affect it from Iceberg), but the tool tip for the node contains this description.
Also, the detailed Physical Plan shows this description. The toString method also uses "IcebergScan" (I followed its lead), but I don't see the toString being used in the Spark UI.

@RussellSpitzer RussellSpitzer Mar 26, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This sounds good to me even outside the scope of this PR, although maybe we should write Iceberg Batch Scan, since we will probably be generating different descriptions for SparkFileScan and any other Scans we build

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah I see we have the other descriptions below, well at least let's match that pattern :) "IcebergBatchScan"?

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 was matching the pattern in the toString method for each of the subclasses of SparkScan. If you prefer, we can use "IcebergBatchScan" in both description and toString for SparkBatchQueryScan.

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.

I am +1 on BatchScan -> IcebergBatchScan for UI purposes.

I think that updating the toString method should be fine as well if need be, provided it passes tests etc. IcebergBatchScan is closer to the name of the class anyway than plain IcebergScan.


Long snapshotId() {
return snapshotId;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ class SparkCopyOnWriteScan extends SparkScan implements SupportsRuntimeFiltering
}
}

@Override
public String description() {
return "IcebergCopyOnWriteScan " + super.description();
Comment thread
wypoon marked this conversation as resolved.
Outdated
}

Long snapshotId() {
return snapshot != null ? snapshot.snapshotId() : null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ class SparkFilesScan extends SparkScan {
this.splitOpenFileCost = readConf.splitOpenFileCost();
}

@Override
public String description() {
Comment thread
wypoon marked this conversation as resolved.
Outdated
return "IcebergFilesScan " + super.description();
}

@Override
protected List<CombinedScanTask> tasks() {
if (tasks == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@
import org.apache.spark.broadcast.Broadcast;
import org.apache.spark.sql.SparkSession;
import org.apache.spark.sql.catalyst.InternalRow;
import org.apache.spark.sql.connector.metric.CustomMetric;
import org.apache.spark.sql.connector.metric.CustomTaskMetric;
import org.apache.spark.sql.connector.read.Batch;
import org.apache.spark.sql.connector.read.InputPartition;
import org.apache.spark.sql.connector.read.PartitionReader;
Expand Down Expand Up @@ -162,6 +164,11 @@ public String description() {
return String.format("%s [filters=%s]", table, filters);
}

@Override
public CustomMetric[] supportedCustomMetrics() {
return new CustomMetric[] { new NumFiles() };
}

static class ReaderFactory implements PartitionReaderFactory {
private final int batchSize;

Expand Down Expand Up @@ -193,15 +200,41 @@ public boolean supportColumnarReads(InputPartition partition) {
}
}

static long numFilesToScan(CombinedScanTask scanTask) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

numSplitsToScan?

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.

Since this is now a one-liner, I have removed the function altogether.

long fileCount = 0L;
for (FileScanTask file : scanTask.files()) {
fileCount += 1L;

@singhpk234 singhpk234 Mar 25, 2022

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.

[quetion] should we incorporate delete's as well assosiated with the FileScanTask

Suggested change
fileCount += 1L;
fileCount += 1L + file.deletes().size();

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.

We could have a separate metric for number of delete files to read. Or rather, a sum of the number of delete files to apply, over the data files to be read; we won't actually track what the delete files are, if there is any overlap among them (can there be overlap?).
@rdblue @aokolnychyi what do you think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's fair to count all the delete files, but definitely in another metric.

There will be duplicates in two cases

  1. It's an equality delete
  2. There is a position delete file for a data file which is split into being read in two different tasks (this is also a problem for duplicate files being read)

}
return fileCount;
Comment thread
wypoon marked this conversation as resolved.
Outdated
}

private static class RowReader extends RowDataReader implements PartitionReader<InternalRow> {
private long numFilesToRead;

RowReader(ReadTask task) {
super(task.task, task.table(), task.expectedSchema(), task.isCaseSensitive());
numFilesToRead = numFilesToScan(task.task);
LOG.debug("reading {} files for table {}", numFilesToRead, task.table().name());
}

@Override
public CustomTaskMetric[] currentMetricsValues() {
return new CustomTaskMetric[] { new TaskNumFiles(numFilesToRead) };
}
Comment on lines +219 to 217

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.

Note: Spark calls this every 100 rows for each PartitionReader. The numFilesToRead can be computed up front and stored, and is. I wonder if it is worth caching the CustomTaskMetric[] as well.

@singhpk234 singhpk234 Mar 25, 2022

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.

can it be a property of this RowReader then as numFilesToScan is already computed in the constructor, and we could just return that when ever asked.

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, this is what I meant, is it worth caching the CustomTaskMetric[], so it can be constructed just once.
I'm undecided.

}

private static class BatchReader extends BatchDataReader implements PartitionReader<ColumnarBatch> {
private long numFilesToRead;

BatchReader(ReadTask task, int batchSize) {
super(task.task, task.table(), task.expectedSchema(), task.isCaseSensitive(), batchSize);
numFilesToRead = numFilesToScan(task.task);
LOG.debug("reading {} files for table {}", numFilesToRead, task.table().name());
}

@Override
public CustomTaskMetric[] currentMetricsValues() {
return new CustomTaskMetric[] { new TaskNumFiles(numFilesToRead) };
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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.spark.source;

import org.apache.spark.sql.connector.metric.CustomTaskMetric;

public class TaskNumFiles implements CustomTaskMetric {
private long value;

TaskNumFiles(long value) {
this.value = value;
}

@Override
public String name() {
return "numFiles";
}

@Override
public long value() {
return value;
}
}