You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "cj-zhukov (via GitHub)" <gi...@apache.org> on 2025/11/26 13:39:28 UTC
[PR] Run the examples in the new format [datafusion]
cj-zhukov opened a new pull request, #18946:
URL: https://github.com/apache/datafusion/pull/18946
## 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 #https://github.com/apache/datafusion/issues/18512.
## 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.
-->
## 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] Run the examples in the new format [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#issuecomment-3596058838
I moved logic from main to method run -- `example.run()` now handles all possible variants, including `All`. `All` iterates over the variants and call .run() recursively.
--
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] Run the examples in the new format [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2567423184
##########
datafusion-examples/examples/custom_data_source/main.rs:
##########
Review Comment:
This is a good point! Thanks for noticing 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] Run the examples in the new format [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2591964615
##########
datafusion-examples/examples/builtin_functions/main.rs:
##########
@@ -67,12 +71,38 @@ impl FromStr for ExampleKind {
}
impl ExampleKind {
- const ALL: [Self; 3] = [Self::DateTime, Self::FunctionFactory, Self::Regexp];
+ const ALL_VARIANTS: [Self; 4] = [
Review Comment:
I think this sounds like a good plan
--
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] Run the examples in the new format [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2591583430
##########
ci/scripts/rust_example.sh:
##########
@@ -25,12 +25,26 @@ export CARGO_PROFILE_CI_STRIP=true
cd datafusion-examples/examples/
cargo build --profile ci --examples
-files=$(ls .)
-for filename in $files
-do
- example_name=`basename $filename ".rs"`
- # Skip tests that rely on external storage and flight
- if [ ! -d $filename ]; then
- cargo run --profile ci --example $example_name
- fi
+SKIP_LIST=("external_dependency" "flight" "ffi")
+
+skip_example() {
+ local name="$1"
+ for skip in "${SKIP_LIST[@]}"; do
+ if [ "$name" = "$skip" ]; then
+ return 0
+ fi
+ done
+ return 1
+}
+
+for dir in */; do
+ example_name=$(basename "$dir")
+
+ if skip_example "$example_name"; then
+ echo "Skipping $example_name"
+ continue
+ fi
+
+ echo "Running example group: $example_name"
Review Comment:
This is a good point! Let's do 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] Run the examples in the new format [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#issuecomment-3620016127
> I'm seeing a parquet page-index–related error when running `custom_file_casts` example, but I didn’t modify this file in my PR. So I don’t believe this issue is caused by my changes. I’m not sure what the root cause is.
I suspect this was introduced in this PR by @adriangb :
- https://github.com/apache/datafusion/pull/18998
--
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] Run the examples in the new format [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#issuecomment-3581408681
### High-Level Overview
This PR adds support for running grouped examples more easily and updates the CI script to match the new structure.
1. Added `All` variant to `ExampleKind` that runs all runnable examples inside that group:
```bash
cargo run --example <group> -- all
```
2. Updated rust_examples.sh
The script now:
- Iterates over example directories instead of .rs files
- Skips a small list of unsupported groups (external_dependency, flight, ffi)
- Runs each group using:
```bash
cargo run --profile ci --example <group> all
```
--
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] Run the examples in the new format [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2592037349
##########
ci/scripts/rust_example.sh:
##########
@@ -22,6 +22,15 @@ set -e
export CARGO_PROFILE_CI_OPT_LEVEL="s"
export CARGO_PROFILE_CI_STRIP=true
+# Remove leftover files inside test_* directories inside datafusion-examples/
Review Comment:
You're right that the shell script cleanup works, but in this case the underlying issue is that the example writes into a relative path like `./datafusion-examples/test_parquet/` . When the example runs under `datafusion-examples/examples/`, this creates a nested `datafusion-examples/` directory, which the script then mistakenly treats as another example group.
To avoid this class of problems entirely, I think it’s cleaner and more robust for the example to write into a tempfile rather than modifying directories inside the repository. This keeps examples self-contained, removes unintended side effects, and lets us simplify the shell script.
I can remove the cleanup logic from the script and keep the example using a temp directory. Let me know if you'd prefer a different approach, but this seems like the cleanest long-term fix.
--
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] Run the examples in the new format [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2567388735
##########
datafusion-examples/examples/custom_data_source/main.rs:
##########
Review Comment:
```suggestion
FileStreamProvider,
```
##########
datafusion-examples/examples/custom_data_source/main.rs:
##########
Review Comment:
```suggestion
Self::FileStreamProvider => "file_stream_provider",
```
##########
datafusion-examples/examples/custom_data_source/main.rs:
##########
Review Comment:
```suggestion
"file_stream_provider" => Ok(Self::FileStreamProvider),
```
##########
datafusion-examples/examples/udf/main.rs:
##########
Review Comment:
```suggestion
Self::Udwf => "udwf",
```
##########
datafusion-examples/examples/custom_data_source/main.rs:
##########
@@ -100,7 +115,36 @@ impl ExampleKind {
const EXAMPLE_NAME: &str = "custom_data_source";
fn variants() -> Vec<&'static str> {
- Self::ALL.iter().map(|x| x.as_ref()).collect()
+ Self::ALL_VARIANTS
+ .iter()
+ .map(|example| example.as_ref())
+ .collect()
+ }
+
+ async fn run(&self) -> Result<()> {
+ match self {
+ ExampleKind::CsvJsonOpener => csv_json_opener::csv_json_opener().await?,
+ ExampleKind::CsvSqlStreaming => {
+ csv_sql_streaming::csv_sql_streaming().await?
+ }
+ ExampleKind::CustomDatasource => {
+ custom_datasource::custom_datasource().await?
+ }
+ ExampleKind::CustomFileCasts => {
+ custom_file_casts::custom_file_casts().await?
+ }
+ ExampleKind::CustomFileFormat => {
+ custom_file_format::custom_file_format().await?
+ }
+ ExampleKind::DefaultColumnValues => {
+ default_column_values::default_column_values().await?
+ }
+ ExampleKind::FileFtreamProvider => {
Review Comment:
```suggestion
ExampleKind::FileStreamProvider => {
```
##########
ci/scripts/rust_example.sh:
##########
@@ -25,12 +25,26 @@ export CARGO_PROFILE_CI_STRIP=true
cd datafusion-examples/examples/
cargo build --profile ci --examples
-files=$(ls .)
-for filename in $files
-do
- example_name=`basename $filename ".rs"`
- # Skip tests that rely on external storage and flight
- if [ ! -d $filename ]; then
- cargo run --profile ci --example $example_name
- fi
+SKIP_LIST=("external_dependency" "flight" "ffi")
+
+skip_example() {
+ local name="$1"
+ for skip in "${SKIP_LIST[@]}"; do
+ if [ "$name" = "$skip" ]; then
+ return 0
+ fi
+ done
+ return 1
+}
+
+for dir in */; do
+ example_name=$(basename "$dir")
+
+ if skip_example "$example_name"; then
+ echo "Skipping $example_name"
+ continue
+ fi
+
+ echo "Running example group: $example_name"
+ cargo run --profile ci --example "$example_name" all
Review Comment:
```suggestion
cargo run --profile ci --example "$example_name" -- all
```
without ` -- ` `all` will be passed as an argument to `cargo`, no ?
##########
datafusion-examples/examples/custom_data_source/main.rs:
##########
Review Comment:
```suggestion
Self::FileStreamProvider,
```
##########
datafusion-examples/examples/custom_data_source/main.rs:
##########
@@ -87,7 +91,18 @@ impl FromStr for ExampleKind {
}
impl ExampleKind {
- const ALL: [Self; 7] = [
+ const ALL_VARIANTS: [Self; 8] = [
+ Self::All,
+ Self::CsvJsonOpener,
+ Self::CsvSqlStreaming,
+ Self::CustomDatasource,
+ Self::CustomFileCasts,
+ Self::CustomFileFormat,
+ Self::DefaultColumnValues,
+ Self::FileFtreamProvider,
Review Comment:
```suggestion
Self::FileStreamProvider,
```
--
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] Run the examples in the new format [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2567381833
##########
datafusion-examples/examples/proto/main.rs:
##########
@@ -50,19 +53,35 @@ impl FromStr for ExampleKind {
fn from_str(s: &str) -> Result<Self> {
match s {
+ "all" => Ok(Self::All),
"composed_extension_codec" => Ok(Self::ComposedExtensionCodec),
_ => Err(DataFusionError::Execution(format!("Unknown example: {s}"))),
}
}
}
impl ExampleKind {
- const ALL: [Self; 1] = [Self::ComposedExtensionCodec];
+ const ALL_VARIANTS: [Self; 2] = [Self::All, Self::ComposedExtensionCodec];
+
+ const RUNNABLE_VARIANTS: [Self; 1] = [Self::ComposedExtensionCodec];
const EXAMPLE_NAME: &str = "proto";
fn variants() -> Vec<&'static str> {
- Self::ALL.iter().map(|x| x.as_ref()).collect()
+ Self::ALL_VARIANTS
+ .iter()
+ .map(|example| example.as_ref())
+ .collect()
+ }
+
+ async fn run(&self) -> Result<()> {
+ match self {
+ ExampleKind::ComposedExtensionCodec => {
+ composed_extension_codec::composed_extension_codec().await?
+ }
+ ExampleKind::All => unreachable!("`All` should be handled in main"),
Review Comment:
Thank you for the suggestion!
I used unreachable!() here because the `All` variant should never reach this branch under the current control flow -- it’s intended to be handled earlier in main with help of `RUNNABLE_VARIANTS`, so hitting this code path would indicate a logical bug rather than a runtime error. In that sense, it felt reasonable to treat it as an unreachable state.
That said, I’m definitely open to discussing this further. If the community feels that returning a `DataFusionError` is more consistent with how similar cases are handled in the examples or elsewhere in the codebase, I’m happy to adjust the implementation. My main goal is to follow the established patterns and keep the code clear for future contributors.
Would love to hear more thoughts from others on what the preferred approach is 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@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] Run the examples in the new format [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2567494436
##########
ci/scripts/rust_example.sh:
##########
@@ -25,12 +25,26 @@ export CARGO_PROFILE_CI_STRIP=true
cd datafusion-examples/examples/
cargo build --profile ci --examples
-files=$(ls .)
-for filename in $files
-do
- example_name=`basename $filename ".rs"`
- # Skip tests that rely on external storage and flight
- if [ ! -d $filename ]; then
- cargo run --profile ci --example $example_name
- fi
+SKIP_LIST=("external_dependency" "flight" "ffi")
+
+skip_example() {
+ local name="$1"
+ for skip in "${SKIP_LIST[@]}"; do
+ if [ "$name" = "$skip" ]; then
+ return 0
+ fi
+ done
+ return 1
+}
+
+for dir in */; do
+ example_name=$(basename "$dir")
+
+ if skip_example "$example_name"; then
+ echo "Skipping $example_name"
+ continue
+ fi
+
+ echo "Running example group: $example_name"
+ cargo run --profile ci --example "$example_name" all
Review Comment:
Agree with you - using `--` is the correct and idiomatic way to pass arguments to an example
--
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] Run the examples in the new format [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#issuecomment-3619680902
I'm seeing a parquet page-index–related error when running `custom_file_casts` example, but I didn’t modify this file in my PR. So I don’t believe this issue is caused by my changes. I’m not sure what the root cause is.
--
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] Run the examples in the new format [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#issuecomment-3588064235
I ran into issues when reading data from CSV files located outside the `datafusion-examples` crate. The relative paths behaved differently in CI than locally, so the examples couldn’t reliably access files stored under the main `datafusion` crate.
As a workaround, I embedded the CSV content directly in the example as bytes, wrote it to a temporary file, and registered that file with the context. This keeps the example self-contained and ensures it works consistently in both local development and CI.
If we want a cleaner long-term approach, we may want to consider storing example-specific data files inside the `datafusion-examples` crate itself (e.g., an `examples/data/ `directory). That would allow examples to load files without depending on paths outside the crate. I’m open to discussing better structures for handling example data across the project.
--
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] Run the examples in the new format [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2570997632
##########
datafusion-examples/examples/proto/main.rs:
##########
@@ -50,19 +53,35 @@ impl FromStr for ExampleKind {
fn from_str(s: &str) -> Result<Self> {
match s {
+ "all" => Ok(Self::All),
"composed_extension_codec" => Ok(Self::ComposedExtensionCodec),
_ => Err(DataFusionError::Execution(format!("Unknown example: {s}"))),
}
}
}
impl ExampleKind {
- const ALL: [Self; 1] = [Self::ComposedExtensionCodec];
+ const ALL_VARIANTS: [Self; 2] = [Self::All, Self::ComposedExtensionCodec];
+
+ const RUNNABLE_VARIANTS: [Self; 1] = [Self::ComposedExtensionCodec];
const EXAMPLE_NAME: &str = "proto";
fn variants() -> Vec<&'static str> {
- Self::ALL.iter().map(|x| x.as_ref()).collect()
+ Self::ALL_VARIANTS
+ .iter()
+ .map(|example| example.as_ref())
+ .collect()
+ }
+
+ async fn run(&self) -> Result<()> {
+ match self {
+ ExampleKind::ComposedExtensionCodec => {
+ composed_extension_codec::composed_extension_codec().await?
+ }
+ ExampleKind::All => unreachable!("`All` should be handled in main"),
Review Comment:
@martin-g Thank you for the suggestion, I really like this! I'm going to work on 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] Run the examples in the new format [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#issuecomment-3619629997
I removed cleaning temp dirs from shell script as it's longer 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@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] Run the examples in the new format [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#issuecomment-3620085361
I have a fix for this, I am just working on 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] Run the examples in the new format [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#issuecomment-3617715136
Updating the branch to pick up CI fixes
--
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] Run the examples in the new format [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #18946:
URL: https://github.com/apache/datafusion/pull/18946
--
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] Run the examples in the new format [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2589513456
##########
datafusion-examples/examples/builtin_functions/regexp.rs:
##########
@@ -32,12 +35,30 @@ use datafusion::prelude::*;
/// https://docs.rs/regex/latest/regex/#grouping-and-flags
pub async fn regexp() -> Result<()> {
let ctx = SessionContext::new();
- ctx.register_csv(
- "examples",
- "datafusion/physical-expr/tests/data/regex.csv",
- CsvReadOptions::new(),
- )
- .await?;
+ // content from file 'datafusion/physical-expr/tests/data/regex.csv'
+ let csv_data = r#"values,patterns,replacement,flags
+abc,^(a),bb\1bb,i
Review Comment:
why inline this content? It is fine, I am just curious
##########
ci/scripts/rust_example.sh:
##########
@@ -25,12 +25,26 @@ export CARGO_PROFILE_CI_STRIP=true
cd datafusion-examples/examples/
cargo build --profile ci --examples
-files=$(ls .)
-for filename in $files
-do
- example_name=`basename $filename ".rs"`
- # Skip tests that rely on external storage and flight
- if [ ! -d $filename ]; then
- cargo run --profile ci --example $example_name
- fi
+SKIP_LIST=("external_dependency" "flight" "ffi")
+
+skip_example() {
+ local name="$1"
+ for skip in "${SKIP_LIST[@]}"; do
+ if [ "$name" = "$skip" ]; then
+ return 0
+ fi
+ done
+ return 1
+}
+
+for dir in */; do
+ example_name=$(basename "$dir")
+
+ if skip_example "$example_name"; then
+ echo "Skipping $example_name"
+ continue
+ fi
+
+ echo "Running example group: $example_name"
Review Comment:
When I ran this script twice, I got an error the second time around:
```shell
./ci/scripts/rust_example.sh
./ci/scripts/rust_example.sh
```
The second run made this:
```
Running example: deserialize_to_struct
Running example group: datafusion-examples
error: no example target named `datafusion-examples` in default-run packages
help: available example targets:
builtin_functions
custom_data_source
data_io
dataframe
execution_monitoring
external_dependency
flight
proto
query_planning
sql_ops
udf
```
##########
datafusion-examples/examples/builtin_functions/main.rs:
##########
@@ -67,12 +71,38 @@ impl FromStr for ExampleKind {
}
impl ExampleKind {
- const ALL: [Self; 3] = [Self::DateTime, Self::FunctionFactory, Self::Regexp];
+ const ALL_VARIANTS: [Self; 4] = [
Review Comment:
When looking at the amount of boiler plate code, I think we can use strum to do the same thing https://crates.io/crates/strum
I know in general adding a new dependency is something we try to avoid, but given strum is [already in the workspace](https://github.com/apache/datafusion/blob/f22a3f3955e667605c0ccbfd6e216f91f4f134ee/Cargo.lock#L6013-L6017), using it in examples seems reasonable to me
Specifically,
- https://docs.rs/strum_macros/latest/strum_macros/derive.EnumIter.html
- https://docs.rs/strum_macros/latest/strum_macros/derive.EnumString.html
--
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] Run the examples in the new format [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#issuecomment-3620019478
I am taking a look
--
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] Run the examples in the new format [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#issuecomment-3616885096
Thanks again as always for working on this
--
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] Run the examples in the new format [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#issuecomment-3615525987
I updated:
- `rust_examples.sh` to remove leftover files inside test_* directories inside `datafusion-examples`
- `dataframe.rs` example to use a temporary directory
--
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] Run the examples in the new format [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2567476581
##########
datafusion-examples/examples/proto/main.rs:
##########
@@ -50,19 +53,35 @@ impl FromStr for ExampleKind {
fn from_str(s: &str) -> Result<Self> {
match s {
+ "all" => Ok(Self::All),
"composed_extension_codec" => Ok(Self::ComposedExtensionCodec),
_ => Err(DataFusionError::Execution(format!("Unknown example: {s}"))),
}
}
}
impl ExampleKind {
- const ALL: [Self; 1] = [Self::ComposedExtensionCodec];
+ const ALL_VARIANTS: [Self; 2] = [Self::All, Self::ComposedExtensionCodec];
+
+ const RUNNABLE_VARIANTS: [Self; 1] = [Self::ComposedExtensionCodec];
const EXAMPLE_NAME: &str = "proto";
fn variants() -> Vec<&'static str> {
- Self::ALL.iter().map(|x| x.as_ref()).collect()
+ Self::ALL_VARIANTS
+ .iter()
+ .map(|example| example.as_ref())
+ .collect()
+ }
+
+ async fn run(&self) -> Result<()> {
+ match self {
+ ExampleKind::ComposedExtensionCodec => {
+ composed_extension_codec::composed_extension_codec().await?
+ }
+ ExampleKind::All => unreachable!("`All` should be handled in main"),
Review Comment:
I'd move the logic from line https://github.com/apache/datafusion/pull/18946/files/60c2a9e6a991efde6b04fc14ef88dc64d7fc632b#diff-8e66baa781bf741371e4d4011550f74c14490472cd2191fe305a0313e1da9231R102 to run(). This way there won't be a need of panicking/returning errors at all.
I.e. just call `example.run()` and handle all possible variants, including ALL, inside run(). ALL will iterate over the variants and call .run() recursively.
--
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] Run the examples in the new format [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2567424541
##########
datafusion-examples/examples/proto/main.rs:
##########
@@ -50,19 +53,35 @@ impl FromStr for ExampleKind {
fn from_str(s: &str) -> Result<Self> {
match s {
+ "all" => Ok(Self::All),
"composed_extension_codec" => Ok(Self::ComposedExtensionCodec),
_ => Err(DataFusionError::Execution(format!("Unknown example: {s}"))),
}
}
}
impl ExampleKind {
- const ALL: [Self; 1] = [Self::ComposedExtensionCodec];
+ const ALL_VARIANTS: [Self; 2] = [Self::All, Self::ComposedExtensionCodec];
+
+ const RUNNABLE_VARIANTS: [Self; 1] = [Self::ComposedExtensionCodec];
const EXAMPLE_NAME: &str = "proto";
fn variants() -> Vec<&'static str> {
- Self::ALL.iter().map(|x| x.as_ref()).collect()
+ Self::ALL_VARIANTS
+ .iter()
+ .map(|example| example.as_ref())
+ .collect()
+ }
+
+ async fn run(&self) -> Result<()> {
+ match self {
+ ExampleKind::ComposedExtensionCodec => {
+ composed_extension_codec::composed_extension_codec().await?
+ }
+ ExampleKind::All => unreachable!("`All` should be handled in main"),
Review Comment:
@martin-g could you provide more insights?
--
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] Run the examples in the new format [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#issuecomment-3621460442
@kumarUjjawal please let us know if you want a chance to re-review. Otherwise I'll plan to merge this in tomorrow
--
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] Run the examples in the new format [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2591406127
##########
datafusion-examples/examples/builtin_functions/regexp.rs:
##########
@@ -32,12 +35,30 @@ use datafusion::prelude::*;
/// https://docs.rs/regex/latest/regex/#grouping-and-flags
pub async fn regexp() -> Result<()> {
let ctx = SessionContext::new();
- ctx.register_csv(
- "examples",
- "datafusion/physical-expr/tests/data/regex.csv",
- CsvReadOptions::new(),
- )
- .await?;
+ // content from file 'datafusion/physical-expr/tests/data/regex.csv'
+ let csv_data = r#"values,patterns,replacement,flags
+abc,^(a),bb\1bb,i
Review Comment:
I inlined the CSV because I ran into issues reading files that live outside the `datafusion-examples`crate (https://github.com/apache/datafusion/pull/18946#issuecomment-3588064235) The relative path works locally but not in CI, so the example couldn’t reliably find `datafusion/physical-expr/tests/data/regex.csv`
To keep the example self-contained and stable in CI, I embedded the data and wrote it to a temp file before registering it.
Longer term, a cleaner solution might be to store example data directly inside the `datafusion-examples` crate (e.g. an `examples/data/folder`). I’d be happy to work with that in a follow-up.
--
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] Run the examples in the new format [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2589551287
##########
ci/scripts/rust_example.sh:
##########
@@ -25,12 +25,26 @@ export CARGO_PROFILE_CI_STRIP=true
cd datafusion-examples/examples/
cargo build --profile ci --examples
-files=$(ls .)
-for filename in $files
-do
- example_name=`basename $filename ".rs"`
- # Skip tests that rely on external storage and flight
- if [ ! -d $filename ]; then
- cargo run --profile ci --example $example_name
- fi
+SKIP_LIST=("external_dependency" "flight" "ffi")
+
+skip_example() {
+ local name="$1"
+ for skip in "${SKIP_LIST[@]}"; do
+ if [ "$name" = "$skip" ]; then
+ return 0
+ fi
+ done
+ return 1
+}
+
+for dir in */; do
+ example_name=$(basename "$dir")
+
+ if skip_example "$example_name"; then
+ echo "Skipping $example_name"
+ continue
+ fi
+
+ echo "Running example group: $example_name"
Review Comment:
I think the issue is that some of the examples leave files around:
```shell
andrewlamb@Andrews-MacBook-Pro-3:~/Software/datafusion2$ find datafusion-examples/examples/datafusion-examples
datafusion-examples/examples/datafusion-examples
datafusion-examples/examples/datafusion-examples/test_json
datafusion-examples/examples/datafusion-examples/test_json/lQqU6IGDpwHJyGQB_0.json
datafusion-examples/examples/datafusion-examples/test_json/KKsELYFJ4st3GUOa_0.json
datafusion-examples/examples/datafusion-examples/test_csv
datafusion-examples/examples/datafusion-examples/test_csv/9Alj5dF7w72vLpp0_0.csv.gz
datafusion-examples/examples/datafusion-examples/test_csv/4Bh8tCQllXXE43A9_0.csv.gz
datafusion-examples/examples/datafusion-examples/test_table
datafusion-examples/examples/datafusion-examples/test_table/Y2CVyUXOXRPhDhFA_0.parquet
datafusion-examples/examples/datafusion-examples/test_table/2nskzCtDZnEJR851_0.parquet
datafusion-examples/examples/datafusion-examples/test_parquet
datafusion-examples/examples/datafusion-examples/test_parquet/4CIKeX6U9Ik1YYgY_0.parquet
datafusion-examples/examples/datafusion-examples/test_parquet/ooUJiB7QMqY0BRdN_0.parquet
```
Maybe we could change the examples to use a temporary directory or else have this script clean up before running
--
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] Run the examples in the new format [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#issuecomment-3621562181
> @kumarUjjawal please let us know if you want a chance to re-review. Otherwise I'll plan to merge this in tomorrow
Looks good to me.
--
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] Run the examples in the new format [datafusion]
Posted by "cj-zhukov (via GitHub)" <gi...@apache.org>.
cj-zhukov commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2591397615
##########
datafusion-examples/examples/builtin_functions/main.rs:
##########
@@ -67,12 +71,38 @@ impl FromStr for ExampleKind {
}
impl ExampleKind {
- const ALL: [Self; 3] = [Self::DateTime, Self::FunctionFactory, Self::Regexp];
+ const ALL_VARIANTS: [Self; 4] = [
Review Comment:
I was actually considering using `strum` for this part, and I agree that it would simplify the boilerplate. Since we try to keep dependencies minimal in examples, I initially decided not to introduce it and instead went with the more explicit implementation to keep the PR focused.
To avoid scope creep for this change, I’d prefer to finish this PR as-is.
After it is merged, I’d be happy to open a follow-up PR to refactor this piece using `strum`, especially since it’s already part of the workspace. That would make the examples cleaner and easier to maintain.
--
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] Run the examples in the new format [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2592655870
##########
ci/scripts/rust_example.sh:
##########
@@ -22,6 +22,15 @@ set -e
export CARGO_PROFILE_CI_OPT_LEVEL="s"
export CARGO_PROFILE_CI_STRIP=true
+# Remove leftover files inside test_* directories inside datafusion-examples/
Review Comment:
I agree writing to a temp file is the best long term solution
I think we should then remove the chnages to the shell script as it will be confusing (and no longer 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@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] Run the examples in the new format [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2591970937
##########
ci/scripts/rust_example.sh:
##########
@@ -22,6 +22,15 @@ set -e
export CARGO_PROFILE_CI_OPT_LEVEL="s"
export CARGO_PROFILE_CI_STRIP=true
+# Remove leftover files inside test_* directories inside datafusion-examples/
Review Comment:
this is probably enough -- you don't need to also change the example to run in a temp dir as well,right?
--
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] Run the examples in the new format [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#issuecomment-3621926896
Thanks @cj-zhukov and @kumarUjjawal and @martin-g
--
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] Run the examples in the new format [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #18946:
URL: https://github.com/apache/datafusion/pull/18946#discussion_r2567339529
##########
datafusion-examples/examples/proto/main.rs:
##########
@@ -50,19 +53,35 @@ impl FromStr for ExampleKind {
fn from_str(s: &str) -> Result<Self> {
match s {
+ "all" => Ok(Self::All),
"composed_extension_codec" => Ok(Self::ComposedExtensionCodec),
_ => Err(DataFusionError::Execution(format!("Unknown example: {s}"))),
}
}
}
impl ExampleKind {
- const ALL: [Self; 1] = [Self::ComposedExtensionCodec];
+ const ALL_VARIANTS: [Self; 2] = [Self::All, Self::ComposedExtensionCodec];
+
+ const RUNNABLE_VARIANTS: [Self; 1] = [Self::ComposedExtensionCodec];
const EXAMPLE_NAME: &str = "proto";
fn variants() -> Vec<&'static str> {
- Self::ALL.iter().map(|x| x.as_ref()).collect()
+ Self::ALL_VARIANTS
+ .iter()
+ .map(|example| example.as_ref())
+ .collect()
+ }
+
+ async fn run(&self) -> Result<()> {
+ match self {
+ ExampleKind::ComposedExtensionCodec => {
+ composed_extension_codec::composed_extension_codec().await?
+ }
+ ExampleKind::All => unreachable!("`All` should be handled in main"),
Review Comment:
```suggestion
ExampleKind::All => {
return Err(DataFusionError::Execution(
"`All` variant must be handled in main; do not call run() directly".to_string()
))
}
```
--
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