kosiew commented on code in PR #24855: URL: https://github.com/apache/datafusion/pull/24855#discussion_r3956658743
########## datafusion/sqllogictest/README.md: ########## @@ -397,17 +397,18 @@ generated DataFusion logical plan (SQL statement → DF logical → Substrait Not all statements will be round-tripped, some statements like CREATE, INSERT, SET or EXPLAIN statements will be issued as is, but any other statement will be round-tripped to/from Substrait. -_WARNING_: as there are still a lot of failures in this mode (https://github.com/apache/datafusion/issues/16248), -it is not enforced in the CI, instead, it needs to be run manually with the following command: +_WARNING_: this mode lives behind the `substrait` feature, and the full suite still reports failures. CI therefore +runs it over a single file, through `cargo xtask ci step test substrait`, which filters to `limit.slt`. Some of the +failures are collected in https://github.com/apache/datafusion/issues/16248. To run it over every file: Review Comment: Could we avoid saying this runs over "every file"? The default runner still excludes `tpch` unless `INCLUDE_TPCH=true`, does not search the additional SQLite test directory unless `INCLUDE_SQLITE=true`, and skips `encrypted_parquet.slt` unless the `parquet_encryption` feature is enabled (`bin/sqllogictests.rs:983-1011`). Since this PR is fixing the accuracy of the runbook, I suggest saying something like "every file selected by the default suite" instead. It would also be good to avoid "all the .slt test files" in the earlier wording for the same reason. Alternatively, the README could document the extra options and features needed to include those excluded suites. ########## datafusion/sqllogictest/README.md: ########## @@ -397,17 +397,18 @@ generated DataFusion logical plan (SQL statement → DF logical → Substrait Not all statements will be round-tripped, some statements like CREATE, INSERT, SET or EXPLAIN statements will be issued as is, but any other statement will be round-tripped to/from Substrait. -_WARNING_: as there are still a lot of failures in this mode (https://github.com/apache/datafusion/issues/16248), -it is not enforced in the CI, instead, it needs to be run manually with the following command: +_WARNING_: this mode lives behind the `substrait` feature, and the full suite still reports failures. CI therefore +runs it over a single file, through `cargo xtask ci step test substrait`, which filters to `limit.slt`. Some of the +failures are collected in https://github.com/apache/datafusion/issues/16248. To run it over every file: ```shell -cargo test --test sqllogictests -- --substrait-round-trip +cargo test --test sqllogictests --features substrait -- --substrait-round-trip Review Comment: Optional suggestion: would it be worth adding a lightweight check that invokes the documented command with `--help`? For example, `cargo test --test sqllogictests --features substrait -- --help`. That could catch future drift in the Cargo feature or CLI spelling without running the intentionally failing round-trip suite. It would only validate that the command parses correctly, not which test files are selected. -- 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]
