You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/05/23 11:03:44 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #6429: Do not run avro sqllogictests tests unless the avro feature is enabled
alamb opened a new pull request, #6429:
URL: https://github.com/apache/arrow-datafusion/pull/6429
# Which issue does this PR close?
Closes https://github.com/apache/arrow-datafusion/issues/6399
# Rationale for this change
If a feature is not enabled, we should not fail tests
# What changes are included in this PR?
Change sqllogictest so that it does not run avro.sql if the `avro` feature is not available
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are these changes tested?
Yes
# Are there any user-facing changes?
You can now do:
```shell
RUST_LOG=debug cargo test -p datafusion --test sqllogictests -- avro
```
And it will pass
```
Finished test [unoptimized + debuginfo] target(s) in 0.28s
Running tests/sqllogictests/src/main.rs (/Users/alamb/Software/target-df/debug/deps/sqllogictests-ef50afea7db0aec6)
[2023-05-23T10:59:09Z INFO sqllogictests] Running with DataFusion runner: tests/sqllogictests/test_files/avro.slt
[2023-05-23T10:59:09Z INFO sqllogictests] Skipping avro.slt because avro feature is not enabled
[2023-05-23T10:59:09Z INFO sqllogictests] Skipping: tests/sqllogictests/test_files/avro.slt
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #6429: Do not run avro sqllogictests tests unless the avro feature is enabled
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6429:
URL: https://github.com/apache/arrow-datafusion/pull/6429#issuecomment-1559071716
@xudong963 or @melgenek I wonder if you have time to review this PR?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #6429: Do not run avro sqllogictests tests unless the avro feature is enabled
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6429:
URL: https://github.com/apache/arrow-datafusion/pull/6429#discussion_r1202125806
##########
datafusion/core/tests/sqllogictests/src/main.rs:
##########
@@ -84,7 +84,10 @@ async fn run_test_file(
relative_path: PathBuf,
) -> Result<(), Box<dyn Error>> {
info!("Running with DataFusion runner: {}", path.display());
- let test_ctx = context_for_test_file(&relative_path).await;
+ let Some(test_ctx) = context_for_test_file(&relative_path).await else {
+ info!("Skipping: {}", path.display());
Review Comment:
Don't feel strongly, was just an observation
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #6429: Do not run avro sqllogictests tests unless the avro feature is enabled
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6429:
URL: https://github.com/apache/arrow-datafusion/pull/6429#issuecomment-1559342345
Thanks for the quick turnaround everyone!@
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #6429: Do not run avro sqllogictests tests unless the avro feature is enabled
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6429:
URL: https://github.com/apache/arrow-datafusion/pull/6429#discussion_r1202104461
##########
datafusion/core/tests/sqllogictests/src/main.rs:
##########
@@ -84,7 +84,10 @@ async fn run_test_file(
relative_path: PathBuf,
) -> Result<(), Box<dyn Error>> {
info!("Running with DataFusion runner: {}", path.display());
- let test_ctx = context_for_test_file(&relative_path).await;
+ let Some(test_ctx) = context_for_test_file(&relative_path).await else {
+ info!("Skipping: {}", path.display());
Review Comment:
I think this will log twice, once in context_for_test_file and once here. I feel at least one of these is redundant...
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6429: Do not run avro sqllogictests tests unless the avro feature is enabled
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6429:
URL: https://github.com/apache/arrow-datafusion/pull/6429#discussion_r1202113058
##########
datafusion/core/tests/sqllogictests/src/main.rs:
##########
@@ -84,7 +84,10 @@ async fn run_test_file(
relative_path: PathBuf,
) -> Result<(), Box<dyn Error>> {
info!("Running with DataFusion runner: {}", path.display());
- let test_ctx = context_for_test_file(&relative_path).await;
+ let Some(test_ctx) = context_for_test_file(&relative_path).await else {
+ info!("Skipping: {}", path.display());
Review Comment:
Yes, it will log twice. I was thinking that I wanted to log the reason the test was skipped (in `context_for_test_file` ) as well as make double sure that the logs showed any test that was skipped (if/when we add more tests that are skipped)
Given that the logs aren't printed by default (you need to do `RUST_LOG=...` to do so ) I chose the "mo logs mo better" approach, but I can see how others might have a different preference
I don't feel strongly and will change it if others prefer less logging
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] xudong963 merged pull request #6429: Do not run avro sqllogictests tests unless the avro feature is enabled
Posted by "xudong963 (via GitHub)" <gi...@apache.org>.
xudong963 merged PR #6429:
URL: https://github.com/apache/arrow-datafusion/pull/6429
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org