[GitHub] [arrow-rs] alamb commented on a diff in pull request #2212: remove redundant CI benchmark check, cleanups

[GitBox <gi...@apache.org>]

alamb commented on code in PR #2212:
URL: https://github.com/apache/arrow-rs/pull/2212#discussion_r932464002


##########
.github/workflows/rust.yml:
##########
@@ -44,37 +44,10 @@ jobs:
       - name: Run tests
         shell: bash
         run: |
-          export ARROW_TEST_DATA=$(pwd)/testing/data
-          export PARQUET_TEST_DATA=$(pwd)/parquet-testing/data
           # do not produce debug symbols to keep memory usage down
           export RUSTFLAGS="-C debuginfo=0"
           cargo test
 
-  check_benches:

Review Comment:
   Here is some evidence that this is already covered
   
   When I broke an arrow benchmark:
   
   ```shell
   echo "fff" >> arrow/benches/string_dictionary_builder.rs
   ```
   
   Then I ran the check that is part of arrow: https://github.com/apache/arrow-rs/blob/bc493d92c2a032a95d92dab81642f05182f20d81/.github/workflows/arrow.yml#L88
   
   ```shell
   cargo check -p arrow --all-targets
       Checking arrow v19.0.0 (/Users/alamb/Software/arrow-rs2/arrow)
   error: expected one of `!` or `::`, found `<eof>`
     --> arrow/benches/string_dictionary_builder.rs:71:1
      |
   71 | fff
      | ^^^ expected one of `!` or `::`
   
   error: could not compile `arrow` due to previous error
   warning: build failed, waiting for other jobs to finish...
   ```
   
   Similarly, when I broke parquet
   
   ```shell
   echo "ggg" >> parquet/benches/arrow_reader.rs 
   ```
   
   A check run by the parquet tests also finds it
   
   ```shell
   cargo check -p parquet --all-features --all-targets
      Compiling parquet v19.0.0 (/Users/alamb/Software/arrow-rs2/parquet)
   error: expected one of `!` or `::`, found `<eof>`
      --> parquet/benches/arrow_reader.rs:695:1
       |
   695 | ggg
       | ^^^ expected one of `!` or `::`
   ```



##########
.github/workflows/parquet.yml:
##########
@@ -72,26 +72,17 @@ jobs:
         with:
           rust-version: stable
       - name: Check compilation
-        run: |

Review Comment:
   I didn't see any reason to run checks that did not have `--all-targets` -- so this change ensures that we test with the following combinations:
   1.  default features,
   2. `--all-features`
   3. `--no-default-features --features=arrow` 
   
   With all targets
   
   Previously, `--all-features --all-targets` was not run checked (and thus the becnhmarks were not checked)



-- 
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