Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix running examples readme #11225

Merged
merged 2 commits into from
Jul 6, 2024
Merged

Fix running examples readme #11225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd6b96d
Fix running examples readme
findepi Jul 2, 2024
5ddec76
Remove doubled fmt check from CI
findepi Jul 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion ci/scripts/rust_example.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

set -ex
cd datafusion-examples/examples/
cargo fmt --all -- --check
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove this? I think it is intended to ensure that cargo fmt is run on the examples.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, and i see how this is not obvious. That's why it's separate commit with an explanation:

Remove doubled fmt check from CI

Examples' format is checked in `check-fmt` job, so can be skipped in
`rust_example.sh`.

cargo check --examples

files=$(ls .)
Expand Down
3 changes: 3 additions & 0 deletions datafusion-examples/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ cd datafusion
# Download test data
git submodule update --init

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this locally and the existing command (in the main datafusion directory) also seems to work fine

~/Software/datafusion2$ cargo run --example csv_sql

     Running `target/debug/examples/csv_sql`
+----+-----------------------------+-----------------------------+
| c1 | MIN(aggregate_test_100.c12) | MAX(aggregate_test_100.c12) |
+----+-----------------------------+-----------------------------+
| c  | 0.2667177795079635          | 0.991517828651004           |
| a  | 0.02182578039211991         | 0.9567595541247681          |
| b  | 0.04893135681998029         | 0.9185813970744787          |
| e  | 0.01479305307777301         | 0.9965400387585364          |
| d  | 0.061029375346466685        | 0.9748360509016578          |
+----+-----------------------------+-----------------------------+
+----+------+
| c2 | c3   |
+----+------+
| 1  | -85  |
| 3  | 13   |
| 4  | -38  |
| 4  | -54  |
| 5  | 36   |
| 1  | -25  |
| 5  | -31  |
| 2  | 45   |
| 3  | 13   |
| 3  | 17   |
| 4  | 65   |
| 4  | -101 |
| 2  | -48  |
| 1  | -56  |
| 1  | -5   |
| 3  | 14   |
| 1  | 83   |
| 3  | -12  |
| 3  | -72  |
| 2  | -43  |
| 5  | -101 |
+----+------+

It does also work in datafusion-examples/examples/ but cding into that directory seems unecessary

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @alamb for testing this.

let me quote the commit message, maybe it explains my reasoning better:

...
Some examples are runnable from any place (e.g. `csv_sql`), but some
expect a specific working directory (e.g. `regexp`). Running from
`datafusion-examples/examples` is tested on CI so guaranteed to work,
let's put this path in the README.
...

It does also work in datafusion-examples/examples/ but cding into that directory seems unecessary

i agree.
It happened to be necessary for the first example i tried to run. maybe i was just unlucky

per the commit message

...
As a follow-up, we should look what it would take to make examples
runnable directly from an IDE such as RustRover.

-- I also am against cd-ing into directory as unnecessary and also want the examples to be runnable from an IDE.
If we want this to be the case, this validated by the CI scripts.

# Change to the examples directory
cd datafusion-examples/examples

# Run the `dataframe` example:
# ... use the equivalent for other examples
cargo run --example dataframe
Expand Down