-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Update README occording to the new examples (#18529) #19257
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.
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
Update README occording to the new examples (#18529) #19257
Conversation
High Level OverviewThis PR restructures and improves the organization of the Note: The |
|
For anyone else who is curious, you can see it rendered at this page: https://github.com/cj-zhukov/datafusion/tree/cj-zhukov/update-readme-occording-to-the-new-examples/datafusion-examples |
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cj-zhukov -- this look like a nice improvement to me
I wonder if there is any way to ensure this document stays in sync with the code. For example, if someone adds a new example, how do we ensure that an appropriate entry is added here 🤔
Maybe we could add a ci check similar to the config doc check
datafusion/.github/workflows/rust.yml
Lines 683 to 710 in 2a08013
| config-docs-check: | |
| name: check configs.md and ***_functions.md is up-to-date | |
| needs: linux-build-lib | |
| runs-on: ubuntu-latest | |
| container: | |
| image: amd64/rust | |
| steps: | |
| - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 | |
| with: | |
| submodules: true | |
| fetch-depth: 1 | |
| - name: Setup Rust toolchain | |
| uses: ./.github/actions/setup-builder | |
| with: | |
| rust-version: stable | |
| - uses: actions/setup-node@395ad3262231945c25e8478fd5baf05154b1d79f # v6.1.0 | |
| with: | |
| node-version: "20" | |
| - name: Check if configs.md has been modified | |
| run: | | |
| # If you encounter an error, run './dev/update_config_docs.sh' and commit | |
| ./dev/update_config_docs.sh | |
| git diff --exit-code | |
| - name: Check if any of the ***_functions.md has been modified | |
| run: | | |
| # If you encounter an error, run './dev/update_function_docs.sh' and commit | |
| ./dev/update_function_docs.sh | |
| git diff --exit-code |
datafusion-examples/README.md
Outdated
|
|
||
| | Group | Subcommand | Category | File Path | Description | | ||
| | ----------------- | ---------------- | -------------- | ------------------------------------------------ | ---------------------------------------------------------- | | ||
| | builtin_functions | date_time | Single Process | `examples/builtin_functions/date_time.rs` | Examples of date-time related functions and queries | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make the file path a link so readers can easily navigate to the code with a single click?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point! Let's do it.
|
@Jefffrey since you originally suggested using the tabular format for organizing the examples, you might be interested in taking a look at this PR. |
datafusion-examples/README.md
Outdated
| - [`examples/flight/client.rs`](examples/flight/client.rs) and [`examples/flight/server.rs`](examples/flight/server.rs): Run DataFusion as a standalone process and execute SQL queries from a client using the Arrow Flight protocol. | ||
| ## Builtin Functions Examples | ||
|
|
||
| | Group | Subcommand | Category | File Path | Description | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably remove the Group and Category columns as they seem constant per header; we can mention them upfront above the table & beneath the header, to reduce the table size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point! I think reducing the table size is a good idea.
Jefffrey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be good to go once the CI checks pass
…me-occording-to-the-new-examples
|
Thanks again @cj-zhukov and @Jefffrey |
This would be great if automated! I've opened a follow-up PR for that #19294 |
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?