You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/01/26 11:55:43 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request #1684: Move `information_schema` tests out of execution/context.rs to `sql_integration` tests
alamb opened a new pull request #1684:
URL: https://github.com/apache/arrow-datafusion/pull/1684
# Which issue does this PR close?
re https://github.com/apache/arrow-datafusion/issues/743
related, perhaps, to https://github.com/apache/arrow-datafusion/issues/1505
Built on the great work of @matthewmturner and @hntd187
# Rationale for this change
There are a bunch of tests in execution/context.rs that are not related to `ExecutionContext` (they are end to end tests for sql that happen to use the functions in `ExecutionContext`)
This both slows down compile times in the `datafusion` crate as well as makes it harder to discover existing test coverage.
This was annoying me recently when working in the `context` module so I decided to whip out a PR. I was iniitially going to do it in a single PR, but it was getting massive so I figured I would do it in chunks
# What changes are included in this PR?
I broke this PR into two commits for (hopefully) easier review.
1. cut/paste the code from context.rs to various places in `datafusion/tests/sql`
2. The second then updates the tests to get them to run in their new home
# Are there any user-facing changes?
No
--
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 change in pull request #1684: Move `information_schema` tests out of execution/context.rs to `sql_integration` tests
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1684:
URL: https://github.com/apache/arrow-datafusion/pull/1684#discussion_r794569032
##########
File path: datafusion/tests/sql/mod.rs
##########
@@ -693,6 +695,21 @@ fn make_timestamp_nano_table() -> Result<Arc<MemTable>> {
make_timestamp_table::<TimestampNanosecondType>()
}
+/// Return a new table provider that has a single Int32 column with
+/// values between `seq_start` and `seq_end`
+pub fn table_with_sequence(
Review comment:
I actually just tried to do this and I hit the issue that the test library would have to depend on DataFusion but then datafusion would depend on the test library resulting in a circular dependency and cargo sadness.
So my plan will be to continue cutting stuff out of context.rs and then aim eventually to remove all of `datafusion/src/test` (either to its own crate or elsewhere)
--
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 change in pull request #1684: Move `information_schema` tests out of execution/context.rs to `sql_integration` tests
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1684:
URL: https://github.com/apache/arrow-datafusion/pull/1684#discussion_r792564409
##########
File path: datafusion/tests/sql/mod.rs
##########
@@ -693,6 +695,21 @@ fn make_timestamp_nano_table() -> Result<Arc<MemTable>> {
make_timestamp_table::<TimestampNanosecondType>()
}
+/// Return a new table provider that has a single Int32 column with
+/// values between `seq_start` and `seq_end`
+pub fn table_with_sequence(
Review comment:
this is now duplicated with the code in datafusion/src/test which is only compiled in test mode and thus can't be used from integration tests such as `sql_integration`
I think ideally we would move the contents of `datafusion/src/test` into its own `test_helpers` *crate* or something so it could be shared 🤔
--
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] hntd187 commented on a change in pull request #1684: Move `information_schema` tests out of execution/context.rs to `sql_integration` tests
Posted by GitBox <gi...@apache.org>.
hntd187 commented on a change in pull request #1684:
URL: https://github.com/apache/arrow-datafusion/pull/1684#discussion_r792758009
##########
File path: datafusion/tests/sql/mod.rs
##########
@@ -693,6 +695,21 @@ fn make_timestamp_nano_table() -> Result<Arc<MemTable>> {
make_timestamp_table::<TimestampNanosecondType>()
}
+/// Return a new table provider that has a single Int32 column with
+/// values between `seq_start` and `seq_end`
+pub fn table_with_sequence(
Review comment:
I like that and just use it in dev-dependencies where necessary.
--
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 merged pull request #1684: Move `information_schema` tests out of execution/context.rs to `sql_integration` tests
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1684:
URL: https://github.com/apache/arrow-datafusion/pull/1684
--
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 change in pull request #1684: Move `information_schema` tests out of execution/context.rs to `sql_integration` tests
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1684:
URL: https://github.com/apache/arrow-datafusion/pull/1684#discussion_r792563062
##########
File path: datafusion/src/execution/context.rs
##########
@@ -3551,476 +3550,6 @@ mod tests {
Ok(())
}
- #[tokio::test]
Review comment:
this code is just moved out of here
--
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