You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "zhuliquan (via GitHub)" <gi...@apache.org> on 2024/11/22 16:49:14 UTC
[PR] minor: allow dead code for Scenario::UTF8 [datafusion]
zhuliquan opened a new pull request, #13531:
URL: https://github.com/apache/datafusion/pull/13531
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #13530.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
I always receive this warning when running `cargo clippy`, and I would like to get rid of it.
## What changes are included in this PR?
<!--
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?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: allow external_access_plan run on windows [datafusion]
Posted by "zhuliquan (via GitHub)" <gi...@apache.org>.
zhuliquan commented on PR #13531:
URL: https://github.com/apache/datafusion/pull/13531#issuecomment-2495824192
> > π€ the test now seems to be failing
> > I wonder if it has to do with how the temp files are created π€
>
> I suspect that the specific char '%7E' (i.e. '~') appears in the directory, and the Windows system seems to be unable to handle this.
>
> ```
> thread 'parquet::external_access_plan::two_selections' panicked at datafusion\core\tests\parquet\external_access_plan.rs:289:10:
> called `Result::unwrap()` on an `Err` value: ParquetError(External(NotFound { path: "C:\\Users\\RUNNER%7E1\\AppData\\Local\\Temp\\user_access_planObCLIR.parquet", source: Os { code: 3, kind: NotFound, message: "The system cannot find the path specified." } }))
> ```
>
> So I fix below code to avoid the specific char which begin with '%'.
>
> 1. replacing '\' to '/' when `target_os` is `windows`.
> https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/tests/parquet/external_access_plan.rs#L323-L328
>
> Because, `Path::new` will replace '\\' to '\\%5C' (i.e. '\') in `PartitionedFile::new`.
> https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/tests/parquet/external_access_plan.rs#L330
>
> and
> https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/src/datasource/listing/mod.rs#L90-L93
> 2. using current dir as temp file dir (Because `temp` dir in CI is located at `C:\\Users\\RUNNER%7E1` and other loading parquet cases which are run ok on windows also use data in `../../parquet-testing/data`), and remove temp file at the end of the case run.
> https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/tests/parquet/external_access_plan.rs#L392
>
> and
> https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/tests/parquet/external_access_plan.rs#L366
> 3. For sake of removing temp file at the end of each case run, I remove singleton mode of TestData in `get_test_data`
> https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/tests/parquet/external_access_plan.rs#L382-L419
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: allow external_access_plan run on windows [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13531:
URL: https://github.com/apache/datafusion/pull/13531#discussion_r1855561110
##########
datafusion/core/tests/parquet/external_access_plan.rs:
##########
@@ -369,45 +379,43 @@ struct TestData {
file_size: u64,
}
-static TEST_DATA: OnceLock<TestData> = OnceLock::new();
+static _TEST_DATA: OnceLock<TestData> = OnceLock::new();
/// Return a parquet file with 2 row groups each with 5 rows
-fn get_test_data() -> &'static TestData {
- TEST_DATA.get_or_init(|| {
Review Comment:
The reason to use the `TEST_DATA` thing was to avoid creating the same file over and over again.
Do we need to remove `TEST_DATA`?
It seems like the core difference is the use of `tempfile_in` rather than the change from static
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: allow external_access_plan run on windows [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13531:
URL: https://github.com/apache/datafusion/pull/13531#issuecomment-2495457107
π€ the test now seems to be failing
I wonder if it has to do with how the temp files are created π€
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: allow external_access_plan run on windows [datafusion]
Posted by "zhuliquan (via GitHub)" <gi...@apache.org>.
zhuliquan commented on PR #13531:
URL: https://github.com/apache/datafusion/pull/13531#issuecomment-2495851607
> > > π€ the test now seems to be failing
> > > I wonder if it has to do with how the temp files are created π€
> >
> >
> > I suspect that the specific char '%7E' (i.e. '~') appears in the directory, and the Windows system seems to be unable to handle this.
> > ```
> > thread 'parquet::external_access_plan::two_selections' panicked at datafusion\core\tests\parquet\external_access_plan.rs:289:10:
> > called `Result::unwrap()` on an `Err` value: ParquetError(External(NotFound { path: "C:\\Users\\RUNNER%7E1\\AppData\\Local\\Temp\\user_access_planObCLIR.parquet", source: Os { code: 3, kind: NotFound, message: "The system cannot find the path specified." } }))
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > So I fix below code to avoid the specific char which begin with '%'.
> >
> > 1. replacing '' to '/' when `target_os` is `windows`.
> > https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/tests/parquet/external_access_plan.rs#L323-L328
> >
> > Because, `Path::new` will replace '\' to '\%5C' (i.e. '\') in `PartitionedFile::new`.
> > https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/tests/parquet/external_access_plan.rs#L330
> >
> > and
> > https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/src/datasource/listing/mod.rs#L90-L93
> > 2. using current dir as temp file dir (Because `temp` dir in CI is located at `C:\\Users\\RUNNER%7E1` and other loading parquet cases which are run ok on windows also use data in `../../parquet-testing/data`), and remove temp file at the end of the case run.
> > https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/tests/parquet/external_access_plan.rs#L392
> >
> > and
> > https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/tests/parquet/external_access_plan.rs#L366
> > 3. For sake of removing temp file at the end of each case run, I remove singleton mode of TestData in `get_test_data`
> > https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/tests/parquet/external_access_plan.rs#L382-L419
I fix this code to avoid convert special characters
https://github.com/apache/datafusion/blob/aaa2e4b648b6e9e1cc66de6f10e07fc2bc25eac1/datafusion/core/src/datasource/listing/mod.rs#L92-L95
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: allow external_access_plan run on windows [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #13531:
URL: https://github.com/apache/datafusion/pull/13531
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: allow external_access_plan run on windows [datafusion]
Posted by "zhuliquan (via GitHub)" <gi...@apache.org>.
zhuliquan commented on PR #13531:
URL: https://github.com/apache/datafusion/pull/13531#issuecomment-2495389426
> we should really avoid `#[allow(dead_code)]` for stuff used only sometimes, because we won't know when this can be removed/
>
> This is used in `external_access_plan` which is disabled on windows. Maybe we should try enabling it?
Yeah, I enable it
```rust
let new_file_name = if cfg!(target_os = "windows") {
// Windows path separator is different from Unix
file_name.replace("\\", "/")
} else {
file_name.clone()
};
```
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: allow external_access_plan run on windows [datafusion]
Posted by "zhuliquan (via GitHub)" <gi...@apache.org>.
zhuliquan commented on code in PR #13531:
URL: https://github.com/apache/datafusion/pull/13531#discussion_r1856582833
##########
datafusion/core/tests/parquet/external_access_plan.rs:
##########
@@ -369,45 +379,43 @@ struct TestData {
file_size: u64,
}
-static TEST_DATA: OnceLock<TestData> = OnceLock::new();
+static _TEST_DATA: OnceLock<TestData> = OnceLock::new();
/// Return a parquet file with 2 row groups each with 5 rows
-fn get_test_data() -> &'static TestData {
- TEST_DATA.get_or_init(|| {
Review Comment:
> If we aren't going to use `TEST_DATA` anymore perhaps we can remove it (rather than name it starting with `_` to avoid a compiler error) π€
Yeah, I share same point with itοΌ I remove it later.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: allow external_access_plan run on windows [datafusion]
Posted by "zhuliquan (via GitHub)" <gi...@apache.org>.
zhuliquan closed pull request #13531: test: allow external_access_plan run on windows
URL: https://github.com/apache/datafusion/pull/13531
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] minor: allow dead code for Scenario::UTF8 [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #13531:
URL: https://github.com/apache/datafusion/pull/13531#issuecomment-2494767825
we should really avoid `#[allow(dead_code)]` for stuff used only sometimes, because we won't know when this can be removed/
This is used in `external_access_plan` which is disabled on windows.
Maybe we should try enabling it?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: allow external_access_plan run on windows [datafusion]
Posted by "zhuliquan (via GitHub)" <gi...@apache.org>.
zhuliquan commented on PR #13531:
URL: https://github.com/apache/datafusion/pull/13531#issuecomment-2495528989
> π€ the test now seems to be failing
>
> I wonder if it has to do with how the temp files are created π€
I noticed that the char '%' appears in the directory, and the Windows system seems to be unable to find the file.
```
thread 'parquet::external_access_plan::two_selections' panicked at datafusion\core\tests\parquet\external_access_plan.rs:289:10:
called `Result::unwrap()` on an `Err` value: ParquetError(External(NotFound { path: "C:\\Users\\RUNNER%7E1\\AppData\\Local\\Temp\\user_access_planObCLIR.parquet", source: Os { code: 3, kind: NotFound, message: "The system cannot find the path specified." } }))
```
So I fix below code to avoid char '%'.
1. replacing '\\' to '/' when `target_os` is `windows`.
https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/tests/parquet/external_access_plan.rs#L323-L328
Because, `Path::new` will replace '\\' to '\\%5C' in `PartitionedFile::new`.
https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/tests/parquet/external_access_plan.rs#L330
and
https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/src/datasource/listing/mod.rs#L90-L93
2. using current dir as temp file dir (Because `temp` dir in CI is located at `C:\\Users\\RUNNER%7E1` and other loading parquet cases which are run ok on windows also use data in `../../parquet-testing/data`), and remove temp file at the end of the case run.
https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/tests/parquet/external_access_plan.rs#L392
and
https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/tests/parquet/external_access_plan.rs#L366
3. For sake of removing temp file at the end of each case run, I remove singleton mode of TestData in `get_test_data`
https://github.com/apache/datafusion/blob/b65d2a3a770596bc33d90f11a908ae29065068bf/datafusion/core/tests/parquet/external_access_plan.rs#L382-L419
5.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] minor: allow dead code for Scenario::UTF8 [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13531:
URL: https://github.com/apache/datafusion/pull/13531#issuecomment-2494517218
Thanks @zhuliquan - it would also be really nice to figure out how you are running the code/tests to see this error. Maybe there is some better way we can come up with to fix it
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: allow external_access_plan run on windows [datafusion]
Posted by "zhuliquan (via GitHub)" <gi...@apache.org>.
zhuliquan commented on code in PR #13531:
URL: https://github.com/apache/datafusion/pull/13531#discussion_r1856580633
##########
datafusion/core/tests/parquet/external_access_plan.rs:
##########
@@ -369,45 +379,43 @@ struct TestData {
file_size: u64,
}
-static TEST_DATA: OnceLock<TestData> = OnceLock::new();
+static _TEST_DATA: OnceLock<TestData> = OnceLock::new();
/// Return a parquet file with 2 row groups each with 5 rows
-fn get_test_data() -> &'static TestData {
- TEST_DATA.get_or_init(|| {
Review Comment:
> The reason to use the `TEST_DATA` thing was to avoid creating the same file over and over again.
>
> Do we need to remove `TEST_DATA`?
>
> It seems like the core difference is the use of `tempfile_in` rather than the change from static
There is no chance for delete TEST_DATA. Because there is no main test function to lead minor test cases.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: allow external_access_plan run on windows [datafusion]
Posted by "zhuliquan (via GitHub)" <gi...@apache.org>.
zhuliquan commented on code in PR #13531:
URL: https://github.com/apache/datafusion/pull/13531#discussion_r1856582833
##########
datafusion/core/tests/parquet/external_access_plan.rs:
##########
@@ -369,45 +379,43 @@ struct TestData {
file_size: u64,
}
-static TEST_DATA: OnceLock<TestData> = OnceLock::new();
+static _TEST_DATA: OnceLock<TestData> = OnceLock::new();
/// Return a parquet file with 2 row groups each with 5 rows
-fn get_test_data() -> &'static TestData {
- TEST_DATA.get_or_init(|| {
Review Comment:
> If we aren't going to use `TEST_DATA` anymore perhaps we can remove it (rather than name it starting with `_` to avoid a compiler error) π€
@alamb Yeah, I share same point with you, I remove it now.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: allow external_access_plan run on windows [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13531:
URL: https://github.com/apache/datafusion/pull/13531#discussion_r1855561330
##########
datafusion/core/tests/parquet/external_access_plan.rs:
##########
@@ -369,45 +379,43 @@ struct TestData {
file_size: u64,
}
-static TEST_DATA: OnceLock<TestData> = OnceLock::new();
+static _TEST_DATA: OnceLock<TestData> = OnceLock::new();
/// Return a parquet file with 2 row groups each with 5 rows
-fn get_test_data() -> &'static TestData {
- TEST_DATA.get_or_init(|| {
Review Comment:
If we aren't going to use `TEST_DATA` anymore perhaps we can remove it (rather than name it starting with `_` to avoid a compiler error) π€
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org