peterxcli commented on PR #5161: URL: https://github.com/apache/datafusion-comet/pull/5161#issuecomment-5443932242
Thanks for the pass — point by point: **Match arm ordering** — agreed it read as "exempt from everything" rather than "supported." Moved the arm to just above the `super.isTypeSupported` fallthrough (it has to stay above the super call, since the shared default rejects intervals) with a comment. No behavior change: an interval can never match the `ShortType`, collation, or struct guards. **V2 coverage** — the V2 Parquet path doesn't go through `CometScanTypeChecker`: `transformV2Scan` gates on `CometBatchScanExec.isSchemaSupported`, which uses the shared `DataTypeSupport` default and still rejects intervals, so a V2 interval scan falls back to Spark exactly as before this PR. (`CometScanTypeChecker` is shared with the V2 Iceberg path, but Iceberg has no interval type.) The fixture's `Config:` line only pins Spark's default — parquet is already in the default `useV1SourceList` — and I've added a comment in the fixture saying so. **Native predicate coverage** — `ParquetReadSuite`'s "ANSI interval types" test already filters on `i2 = INTERVAL '1-2' YEAR TO MONTH AND i9 = INTERVAL '1 02:03:04.5' DAY TO SECOND` under `checkSparkAnswerAndOperator`, asserts `dataFilters` is non-empty, and checks `output_rows` == 333 with pushdown vs 1000 without, so both the literal encoding and row-group pruning are pinned against Spark's answer. I've additionally added `ym >` / `dt >` predicate queries to `ansi_interval_scan.sql` so the SQL golden harness covers it across Spark versions too. **Prettier** — `npx prettier --check docs/source/user-guide/latest/datatypes.md` passes on the branch as-is, and the markdown CI check is green on the latest push, so no realignment is pending. Separately, the one failing check (`PR Build (Linux) / Spark 4.1, JDK 17 [exec]`) was an HTTP 429 fetching the Maven distribution during setup — infra flake; a re-run should clear it. Both targeted suites pass locally on Spark 4.1 / JDK 17 after the change: `ParquetReadV1Suite "ANSI interval types"` and `CometSqlFileTestSuite "sql-file: expressions/datetime/ansi_interval_scan.sql"`. -- 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]
