andygrove commented on PR #54: URL: https://github.com/apache/datafusion-spark/pull/54#issuecomment-6857794361
Thanks for the review @mbutrovich. All four points are addressed: - **Packaging:** the runner moved to `crates/sqllogictest/scripts/` (`publish = false`), so it no longer ships in the `datafusion-spark` crate. The repo-root path still resolves correctly from there. - **`except TypeError`:** the DataFrame is built outside the `try`, and only `.collect()` is wrapped, so a `TypeError` from the runner's own code is now reported as `runner-error`. - **Expected errors:** the README now says, next to the outcome table, that a record expecting an error passes on any Spark error and that the message is not compared. - **CI:** `.github/workflows/pyspark-runner.yml` runs `python3 -m unittest discover -s crates/sqllogictest/scripts`. It only triggers on changes to the runner or the workflow. Running Spark stays manual. I also did two rounds of self-review and a cleanup pass. Fixes from that: - Skip `arrow_cast` to nested date/time/timestamp types (e.g. `List(Date32)`) and of a timestamp to an integer, where Arrow and Spark convert differently. - Keep `DROP ... IF EXISTS` / `CREATE OR REPLACE` semantics instead of always rewriting to `DROP VIEW IF EXISTS` / `CREATE OR REPLACE`, so records that expect an error aren't hidden. `CREATE TABLE IF NOT EXISTS` is skipped because Spark doesn't allow it on a temporary view. - Time zones: UTC names match in any case, hour-only offsets like `+05` work, and unknown zones are skipped instead of counting as runner failures. - Maps nested in arrays, structs and maps are collected as entries, so they keep Spark's order and can have array keys. - Doubles round half to even, matching `BigDecimal::round` in DataFusion's `float_to_str`. - Double-quoted identifiers are translated to backticks, since Spark reads `"x"` as a string by default. - Dates and timestamps outside Python's range are a convert skip, not a runner error. - Zoned timestamps in expected results are localized only in timestamp columns, not inside string columns. - Values like `007` or `1_0` are no longer classified as numeric scale differences. - Small fixes: `x::DATE` detection, trailing `--` comments in `CREATE TABLE ... AS`, `DATE` literals outside UTC sessions, a broken README link, and a temp warehouse directory that was never cleaned up. - Cleanup: removed duplicated helpers (quoting, paren matching, time-unit tables, wall-clock conversion, error handling) and dead code. I ran the full suite against PySpark 4.2.0 before and after. The only change in outcomes is 6 records going from pass to skip: `DROP TABLE` statements for tables the runner had never created, which only passed before because of the forced `IF EXISTS`. -- 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]
