feat: support _metadata constant columns in native Parquet scan - #5237
feat: support _metadata constant columns in native Parquet scan#5237mbutrovich wants to merge 10 commits into
Conversation
…e, file_block_start, file_block_length, file_modification_time)
andygrove
left a comment
There was a problem hiding this comment.
Thanks for this, and for the unusually detailed PR description. The insight that the six file-constant columns are exactly shaped like Hive partition columns, and so can ride the existing table_partition_cols machinery with no proto or DataFusion changes, is a nice one.
I built the branch locally and probed a number of query shapes beyond the seven new tests. Most of what I tried behaves correctly:
- The schema-ordering assumption holds.
FileSourceStrategybuildsoutputAttributesasreadDataColumns ++ generatedMetadataColumns ++ partitionColumns ++ constantMetadataColumns, andpartitionColumnsthere is the full partition schema rather than a pruned subset. Sofilter_schema = required_schema ++ partition_schemainplanner.rsreally does line up with Scala'sexprToProto(filter, scan.output)numbering, as long asgeneratedMetadataColumnsis empty. The narrowed gate guarantees that, and the_tmp_metadata_row_indexcheck is a second layer. - Non-UTC session timezone with
file_modification_timematches Spark. spark.comet.parquet.rowFilterPushdown.enabled=truewith a_metadata.file_sizepredicate matches Spark.- A metadata-only projection (empty
requiredSchema, e.g.SELECT _metadata.file_name FROM t) runs natively and returns correct values. - A directory with a space in it produces the correct URL-encoded
file_pathandfile_nameon Spark 4.1. - Selecting the whole
_metadatastruct still falls back to Spark, sincerow_indexis in the pruned struct.
I did find one correctness problem that I think needs addressing before merge, plus two coverage gaps.
1. Metadata column names can collide with real column names, giving wrong results
Reusing table_partition_cols also opts into DataFusion's partition-value literal substitution. In ParquetOpener::prepare, DataFusion builds a literal_columns: HashMap<String, ScalarValue> from table_partition_cols zipped with partition_values, then runs replace_columns_with_literals over both the projection and the predicate. That helper matches purely on Column::name() and ignores the index (datafusion-physical-expr-adapter/src/schema_rewriter.rs).
Hive partition column names can never collide with the data schema, so this has always been safe. But file_size, file_path, file_name, file_block_start, file_block_length and file_modification_time are ordinary identifiers a user's table can legitimately contain, and they now land in table_partition_cols.
Three failures I reproduced on this branch (Spark 4.1, ParquetReadV1Suite):
Silent wrong results in a plain projection.
Seq((1L, 10), (2L, 20)).toDF("file_size", "x").repartition(1).write.parquet(dir)
spark.read.parquet(dir).select($"file_size", $"x", $"_metadata.file_size".as("meta_size"))Spark: [1,10,719], [2,20,719]. Comet: [719,10,719], [719,20,719]. The real data column is silently overwritten with the file's byte size.
Silent wrong results in a filter. Same table, add .filter($"file_size" === 1L). Spark returns 1 row, Comet returns 0. The data column reference becomes the literal 719, so the predicate prunes the whole file.
Hard failure when a Hive partition column carries the name.
Seq((1, 100L), (2, 200L)).toDF("id", "file_size").repartition(1)
.write.partitionBy("file_size").parquet(dir)
spark.read.parquet(dir).select($"id", $"file_size", $"_metadata.file_size")fails with CometNativeException: Invalid argument error: column types must match schema types, expected Int32 but found Int64 at column index 1, because the Int32 partition column reference gets replaced by the Int64 metadata literal.
There are two ways out. The cheap one is a collision guard in CometScanRule.scala:151: only accept a constant metadata column when its name does not collide with relation.dataSchema or relation.partitionSchema, respecting spark.sql.caseSensitive, and fall back otherwise. The better one is to rename the fields on the wire, say _comet_metadata_file_size, in both CometNativeScan.scala:176 and operator/package.scala:77, so a collision is structurally impossible and native execution is retained. Everything downstream is positional (the projection vector and the Bound indices), so the rename should be contained.
Either way it would be good to have tests in both directions: a data column named file_size, and a partition column named file_size.
2. The Spark 3.4 shim is hand-replicated and has the thinnest CI coverage
spark/src/main/spark-3.4/org/apache/comet/shims/ShimFileFormat.scala:39 replicates Spark 3.5's BASE_METADATA_EXTRACTORS rather than Spark 3.4's own FileFormat.updateMetadataInternalRow. Those are close but not textually identical: 3.4 derives file_name from pf.filePath.toPath.toUri.getRawPath, because FileScanRDD passes currentFile.toPath (a Hadoop Path), whereas the shim uses pf.filePath.toUri.getRawPath (SparkPath's own URI, one fewer round trip through Path).
I expect those agree for ordinary paths, and the new tests do pass on 3.4. But every temp dir in them is free of characters that would exercise the difference, and the job that would catch it (Spark's metadata file path is url encoded and metadata file name is url encoded tests in FileMetadataStructSuite) lives in Spark SQL Tests (Spark 3.4), which does not run in PR CI.
Could you add a ParquetReadSuite test that writes into a directory with a space in the name and projects both file_path and file_name? That suite runs on all five versions in PR CI, so it would cover the one version where this logic is hand-written. I confirmed such a test passes on 4.1, so this is about pinning 3.4 down rather than a known break.
3. Nothing covers the fallback branch of the narrowed gate
The change narrows a fallback gate in CometScanRule.scala:154 and removes another one entirely, but none of the seven new tests assert that the remaining unsupported shape still falls back. Selecting the whole _metadata struct, or _metadata.row_index directly, is what has to keep going through Spark. I checked by hand that it does today. Since the gate is now a name-set subtraction rather than an unconditional check, could we pin that with a test?
| withFallbackReason( | ||
| scanExec, | ||
| "Native DataFusion scan is not compatible with input_file_name, " + | ||
| "Native Parquet scan is not compatible with input_file_name, " + |
There was a problem hiding this comment.
its not a problem of this specific PR, but prob we can fix the error message to
Native Parquet Scan doesn't support functions `input_file_name`, .....
There was a problem hiding this comment.
Do we use markdown semantics in error messages?
There was a problem hiding this comment.
I did a quick pass, and I don't see us ever using markdown formatting in errors.
…ve scan DataFusion's `table_partition_cols` literal substitution matches columns by name, not position, so a real data or Hive partition column sharing a name with one of the six constant metadata columns (e.g. `file_size`) could be silently overwritten with the metadata value. Spark itself has no such hazard: metadata attributes get a fresh ExprId regardless of name and are always resolved by ExprId/ordinal, never by name. Prefix the wire field names with `_comet_metadata_` so the collision is structurally impossible, matching Spark's guarantee instead of falling back. Also adds test coverage requested in review: fallback still applies to `_metadata` (whole struct) and `_metadata.row_index`, file_path/file_name are correctly url-encoded on Spark 3.4 for a directory containing a space, and drops redundant `includeClasses` assertions from `checkSparkAnswerAndOperator` calls now that `CometNativeScanExec` is the only native scan implementation.
|
Thanks @andygrove! I think I addressed all of the feedback in the latest commit. |
|
Thanks for addressing my feedback @mbutrovich. Could you take a look at the test failure in CI? |
Thanks @andygrove! Should be fixed now. |
andygrove
left a comment
There was a problem hiding this comment.
LGTM pending CI. Thanks @mbutrovich
Which issue does this PR close?
Closes #.
This is motivated by #3432 but does not close it. #3432 is specifically about
_metadata.row_index, which is generated per row by the Parquet reader and needs DataFusion's virtual-column plumbing (DataFusion 55, not yet picked up by Comet). This PR adds the other six_metadatacolumns, which turned out to be unblocked already and unrelated to that dependency.Rationale for this change
Spark's
FileSourceScanExecexposes six file-source constant_metadatacolumns:file_path,file_name,file_size,file_block_start,file_block_length,file_modification_time. Unlikerow_index, all six are known before opening the file and are constant for every row read from it, exactly like Hive partition columns.CometScanRulewas falling back to Spark unconditionally whenever any of these appeared in a query, with no distinction fromrow_index. The value-delivery mechanism for this already exists for partition columns (DataFusion'stable_partition_cols/PartitionedFile.partition_values, generic over field position), so these six columns can reuse it directly with no DataFusion or proto changes.What changes are included in this PR?
CometScanRule.scala: narrowed the metadata-column fallback gate to only reject columns that are not infileConstantMetadataColumns(i.e._metadata.row_index). Removed the now-redundantfileConstantMetadataColumns.nonEmptyfallback check below it, since the narrowed gate already covers that case. Kept the_tmp_metadata_row_indexschema check: that column is Spark's internal magic-named temporary column, not anisMetadataCol-flagged attribute, so the metadata-column gate never sees it and the scan needs its own check.CometNativeScan.scala: appends the constant metadata columns' schema after the real Hive partition schema, matching Spark's ownscan.outputordering (data columns, then partition columns, then constant metadata columns), and extends the projection vector and length assertion to match.operator/package.scala:partition2Prototakes the constant metadata attributes and the relation'sfileConstantMetadataExtractors, and derives each value via Spark's ownFileFormat.getFileConstantMetadataColumnValue(covers custom per-format extractor overrides), retyped against the attribute's declareddataTypethe same way Spark's ownupdateMetadataInternalRowdoes. Values are appended to the existingpartition_valuesproto field alongside real partition values.CometNativeScanExec.scala: threadsfileConstantMetadataColumnsandfileFormat.fileConstantMetadataExtractorsthrough topartition2Proto.ShimFileFormat.scala(per Spark version): addedfileConstantMetadataExtractors/getFileConstantMetadataColumnValue. Spark 3.4 predates per-format metadata extractors (SPARK-43868), so its shim replicatesFileFormat.BASE_METADATA_EXTRACTORSinline instead of delegating. Spark 4.2 backported SPARK-56931, which added a requireddataTypeargument togetFileConstantMetadataColumnValue; the shim signature is unified to 4 args across all five supported versions so the call site stays version-independent, with only the 4.2 shim using the extra argument. This required splitting the previously-sharedspark-4.x/ShimFileFormat.scalainto per-minor-version copies, matching how other Spark 4.x shims in this repo already handle behavior that differs by minor version.contraintExpressions.scala/QueryPlanSerde.scala: added a serde for Spark'sKnownNotNulltagging expression. Spark'sFileSourceStrategywraps the reconstructed_metadatastruct inKnownNotNullto force non-nullability on the schema; it is a runtime no-op, so the serde serializes the child and drops the tag (same approach as the existingCometKnownNullable; both now share one helper).native/core/src/execution/planner.rs:data_filterswere bound only againstrequired_schema, which excludes partition and metadata columns. A filter referencing one of these columns failed with a column-index-out-of-bounds error. Filters are now bound against the combinedrequired_schema+partition_schema, matching how Scala numbers columns when building the filter proto. This also fixes filtering on real Hive partition columns pushed down as a data filter, which shared the same latent bug (previously never triggered, since Spark's planner never routes a pure partition-column predicate throughdataFilters).docs/.../compatibility/scans.md: narrowed the "Spark metadata columns" limitation entry to_metadata.row_indexonly.CometNativeScan.scala: constant metadata columns are wired under a_comet_metadata_prefix rather than their bare Spark name. DataFusion'stable_partition_colsliteral substitution matches by name, not position, so a bare name likefile_sizecould collide with a real data or Hive partition column of the same name and get silently substituted. Spark itself has no such hazard (metadata attributes always resolve by ExprId/ordinal, never by name), so the prefix reproduces that guarantee instead of deviating from it.How are these changes tested?
Seven new tests in
ParquetReadSuite.scala, each checking both correct results (against Comet-disabled Spark) and that the native scan actually runs (no fallback):_metadata.file_sizealone. A pure metadata-column predicate can never become a Spark partition-pruning filter, since these columns are not part of the Hive partition schema, so this exercises per-file value correctness during the scan itself, not just projection._metadata.file_sizewith a range predicate (>), not just equality._metadata.file_sizecombined with an ordinary data column in one predicate._metadata.file_pathfor exact string equality.file_pathis derived to match Spark's own qualified-URI string exactly, including its single-slash local-path form, so an equality filter is a direct check on that derivation.CometNativeScan.scala'spartitionSchemaFields).planner.rs'sfilter_schema.The two-file tests (
writeTwoFilesAndDiscoverMetadata) assertrdd.getNumPartitions == 1to confirm both files land in the same Spark task, sopartition2Proto's per-file loop is exercised with more than one file, not just across separate tasks. Getting two tiny files into one partition isn't the default:spark.sql.files.minPartitionNumdefaults to the session's target parallelism, which pushesmaxSplitBytesdown tospark.sql.files.openCostInBytes, and a single file's own virtual open-cost surcharge already consumes that budget, so Spark schedules one file per task by default. These tests forcespark.sql.files.minPartitionNum=1to get both files packed into one partition instead.Three more tests cover review feedback:
file_path/file_nameare correctly url-encoded on Spark 3.4 for a directory containing a space (that logic is hand-replicated on 3.4 and untested elsewhere in PR CI), and two collision reproductions (a data column and, separately, a partition column both namedfile_size) confirming the rename fix above. ACometScanRuleSuitetest also pins that_metadata(whole struct) and_metadata.row_indexstill fall back to Spark.