andygrove commented on issue #4994: URL: https://github.com/apache/datafusion-comet/issues/4994#issuecomment-6223160353
A note for whoever picks this up, so the next attempt doesn't hit the same wall. #4995 tried this with a process-wide registry of `FileMetadataCache`s keyed by object store URL. It also plumbed Spark's `modificationTime` through to `object_meta.last_modified`, so `(size, last_modified)` validation was real. It was closed because `spark-sql-sql_hive-1` failed on both Spark 3.5 and 4.1 ([3.5 job](https://github.com/apache/datafusion-comet/actions/runs/29850687435/job/88712647467), [4.1 job](https://github.com/apache/datafusion-comet/actions/runs/29850687435/job/88713139609)). @mbutrovich noted that @comphead's earlier attempt hit the same problem. A Spark SQL test creates a file, reads it, deletes it and reads it again, and the second read was answered from the cached metadata. The tests that match that shape are in `HiveMetadataCacheSuite`. It is untagged, so it runs in `sql_hive-1`. Its four tests are `SPARK-16337 temporary view refresh`, `view refresh`, and `partitioned table is cached when partition pruning is true` / `is false`. Each runs a `count(*)` over a Parquet table, deletes one data file behind Spark's back, runs the `count(*)` again, and expects `FAILED_READ_FILE.FILE_NOT_EXIST`. I couldn't pull the job logs to confirm the exact test name, but nothing else in that shard does this. Validation can't catch it, because the second query's file list comes from Spark's cached file index. That index still lists the deleted file with its old size and modification time, so a cached footer validates against it. A `count(*)` needs nothing but the footer's row counts, so on a hit the native scan never opens the file. The deletion goes unnoticed and the query returns the old count. Today every task opens the footer, and that open raises `FILE_NOT_EXIST` the way Spark does. sql/core's `MetadataCacheSuite` has the same three-step tests, but it writes ORC, which Comet doesn't scan natively. So `sql_core` won't catch a regression here. Only the Hive shard will. Two directions that avoid it: - **Share only within a stage.** Key entries by stage id (or SQL execution id) as well as path, size and modification time. That covers the first case in this issue, a large file split across one stage's tasks, while every new query still opens its files, which is what these tests check. Reuse across queries, the second case, is the part that conflicts with Spark's semantics. Keying by the serialized plan bytes, as #5615's base-plan cache does, wouldn't be enough: the two `count(*)` runs in these tests list the same files and can produce the same plan bytes. - **Keep the cache process-wide, but check that the file still exists** (a `stat` or `HEAD`) before using a cached entry. That still saves the footer fetch and the Thrift parse. On object stores it costs one metadata round trip per file per task. One more data point on why the first case matters. `EagerPageIndexReaderFactory` loads the page index on every metadata fetch, for all of the file's row groups and columns, and relies on the cache to absorb repeats. With a per-task cache, every task that opens a split pays for the whole file's page index. I measured a `count(*)` over one 474 MB lineitem file (98 row groups) split into 470 tasks with `spark.sql.files.maxPartitionBytes=1m` (macOS profile, release build). Footer and page-index decoding was about 9% of task and Tokio thread samples, and most of those tasks' splits held no row group at all. parquet-mr re-reads the footer per task too, but it reads column indexes only for the row groups in its split and the filtered columns, so that part is not parity with Spark. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
