Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 29.4k
[SPARK-57205][SQL] Declare SCAN_MERGING on the built-in file sources#58340
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.
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
f5dd90387e94ee70204f97dd183360961feb6c4b7b019cda6File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -37,6 +37,18 @@ import org.apache.spark.sql.util.CaseInsensitiveStringMap | ||
| import org.apache.spark.sql.util.SchemaUtils | ||
| import org.apache.spark.util.ArrayImplicits._ | ||
| /** | ||
| * A [[Table]] backed by files. | ||
| * | ||
| * A subclass opts in to the `SCAN_MERGING` capability by overriding [[supportsScanMerging]], which | ||
| * holds it to this: with the scan options and the pushed filters held constant, widening the set of | ||
| * columns pruned on its builder must not change which rows the scan returns, nor the values it | ||
| * returns for the columns it was already asked for. It may at most surface a read error. A format | ||
| * whose parser decides what counts as a malformed record from the set of columns it was asked for | ||
| * does not meet that, and neither does one that resolves a column by its position in the | ||
| * projection. The capability is also withheld from a table whose reads are not strict, see | ||
| * `hasStrictFileReads`. | ||
| */ | ||
| abstract class FileTable( | ||
| sparkSession: SparkSession, | ||
| options: CaseInsensitiveStringMap, | ||
| @@ -111,7 +123,32 @@ abstract class FileTable( | ||
| override def properties: util.Map[String, String] = options.asCaseSensitiveMap | ||
| override def capabilities: java.util.Set[TableCapability] = FileTable.CAPABILITIES | ||
| override def capabilities: java.util.Set[TableCapability] = | ||
| if (supportsScanMerging && hasStrictFileReads) { | ||
| FileTable.CAPABILITIES_WITH_SCAN_MERGING | ||
| } else { | ||
| FileTable.CAPABILITIES | ||
| } | ||
| /** | ||
| * Whether this table meets the `SCAN_MERGING` contract described on this class. Defaults to | ||
| * false: a format that does not merge only misses an optimization, while a format that merges | ||
| * when its parser is projection-sensitive returns wrong rows. | ||
| */ | ||
| protected def supportsScanMerging: Boolean = false | ||
| /** | ||
| * Whether a read of this table is strict. Under `ignoreCorruptFiles`, a read failure in a column | ||
| * that only the other scan projects is swallowed and the remaining rows of that file are dropped, | ||
| * so the merged scan would not read a superset of either input's rows. `ignoreMissingFiles` drops | ||
| * the same rows whatever is projected, and is included to match `FileScanRDD.hasStrictFileReads`, | ||
| * the same predicate on the physical side. Evaluated per call rather than cached, so a table | ||
| * built before either configuration was set still answers for the read that is running. | ||
| */ | ||
| private def hasStrictFileReads: Boolean = { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finding 16.#58411 lifts this exact predicate onto privatedefhasStrictFileReads:Boolean=newFileSourceOptions(options.asCaseSensitiveMap.asScala.toMap).hasStrictFileReadsNothing to do now if this lands first - just worth a line in the description so the follow-up is not lost, since the scaladoc here already points at | ||
| val fileSourceOptions = new FileSourceOptions(options.asCaseSensitiveMap.asScala.toMap) | ||
| !fileSourceOptions.ignoreCorruptFiles && !fileSourceOptions.ignoreMissingFiles | ||
| } | ||
| /** | ||
| * When possible, this method should return the schema of the given `files`. When the format | ||
| @@ -182,4 +219,10 @@ abstract class FileTable( | ||
| object FileTable { | ||
| private val CAPABILITIES = util.EnumSet.of(BATCH_READ, BATCH_WRITE) | ||
| // For the formats that override supportsScanMerging. `fileIndex` is a lazy val, so every scan | ||
| // built from one table lists the same files, and `newScanBuilder` returns a fresh builder over | ||
| // `mergedOptions(options)`. | ||
| private val CAPABILITIES_WITH_SCAN_MERGING = | ||
| util.EnumSet.of(BATCH_READ, BATCH_WRITE, SCAN_MERGING) | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finding 13. This is the right criterion, and it is stricter than the one
TableCapability.SCAN_MERGINGstates. A third-party connector author reads only the javadoc.TableCapability.java:134-139:A CSV table satisfies that as written. Its rows are fully determined by the pruned column set - that is exactly the dependence, and re-pruning to the same set does yield an equivalent scan. So
CSVTablecould declare the capability without contradicting a word of it, and the next paragraph's "the merged scan reads ... a superset of their rows" then simply does not follow from what the table promised.What closes the gap is the monotonicity clause you wrote here: widening the pruned set must not change the rows or the values. Suggest adding it to the javadoc, right after the determinism sentence:
Different clause from the one at r3879483408 - there I said not to weaken "a superset of their rows", and this asks to strengthen the sentence above it so that claim actually follows. Fine as a follow-up on SPARK-40259 if you would rather not widen this diff, but the capability is
@since 4.3.0and unreleased, so it is cheaper now than after.