You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "vmingchen (via GitHub)" <gi...@apache.org> on 2025/02/26 14:02:28 UTC
[PR] Change tpch validation to use `exec_sql_on_tables` [datafusion-ray]
vmingchen opened a new pull request, #66:
URL: https://github.com/apache/datafusion-ray/pull/66
Fixes https://github.com/apache/datafusion-ray/issues/65
`exec_sql_on_tables` is a util function added by this PR that uses DataFution without Ray to execute queries. This ensures the validation is using the same Rust version as the DataFusion-Ray avoiding validation failure caused by inconsistency between different versions of DataFusion and DataFusion-Python. With this change, all TPCH validations are passing regardless of versions.
Also, expose the `schema` of `RayDataFrame` to facilitate debugging.
Thank @robtandy for the idea; see https://github.com/apache/datafusion-ray/issues/65#issuecomment-2679094341
--
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] Change tpch validation to use `exec_sql_on_tables` [datafusion-ray]
Posted by "vmingchen (via GitHub)" <gi...@apache.org>.
vmingchen commented on code in PR #66:
URL: https://github.com/apache/datafusion-ray/pull/66#discussion_r1973563759
##########
src/util.rs:
##########
@@ -397,6 +402,52 @@ fn print_node(plan: &Arc<dyn ExecutionPlan>, indent: usize, output: &mut String)
}
}
+async fn exec_sql(query: String, tables: Vec<(String, String)>) -> PyResult<RecordBatch> {
+ let ctx = SessionContext::new();
+ for (name, path) in tables {
+ if path.ends_with(".parquet") {
Review Comment:
Thanks for pointing this out! I looked into it, and it turned out [`register_parquet` internally use `register_listing_table` as well](https://github.com/apache/datafusion/blob/fc2fbb3d6b3aded73f1b0902168e008e580c89c1/datafusion/core/src/execution/context/parquet.rs#L55-L61). The later is capable of register both a single file and a directory of files. So I have changed the function to use `register_listing_table` only in https://github.com/apache/datafusion-ray/pull/66/commits/b8e0c6b3c10321896f39e33dee94ea843d91505a; the new commit also adds a unit test to check that it is working for both file and directory and a doc pointing to the format of the URI.
Please take another look; thanks!
--
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] Change tpch validation to use `exec_sql_on_tables` [datafusion-ray]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #66:
URL: https://github.com/apache/datafusion-ray/pull/66#discussion_r1975841145
##########
src/util.rs:
##########
@@ -397,6 +402,52 @@ fn print_node(plan: &Arc<dyn ExecutionPlan>, indent: usize, output: &mut String)
}
}
+async fn exec_sql(query: String, tables: Vec<(String, String)>) -> PyResult<RecordBatch> {
+ let ctx = SessionContext::new();
+ for (name, path) in tables {
+ if path.ends_with(".parquet") {
Review Comment:
Thanks for updating that. LGTM.
--
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] Change tpch validation to use `exec_sql_on_tables` [datafusion-ray]
Posted by "robtandy (via GitHub)" <gi...@apache.org>.
robtandy commented on PR #66:
URL: https://github.com/apache/datafusion-ray/pull/66#issuecomment-2685277160
@vmingchen, thank you! This looks good to me!
cc @andygrove for approval
--
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] Change tpch validation to use `exec_sql_on_tables` [datafusion-ray]
Posted by "vmingchen (via GitHub)" <gi...@apache.org>.
vmingchen commented on code in PR #66:
URL: https://github.com/apache/datafusion-ray/pull/66#discussion_r1973563759
##########
src/util.rs:
##########
@@ -397,6 +402,52 @@ fn print_node(plan: &Arc<dyn ExecutionPlan>, indent: usize, output: &mut String)
}
}
+async fn exec_sql(query: String, tables: Vec<(String, String)>) -> PyResult<RecordBatch> {
+ let ctx = SessionContext::new();
+ for (name, path) in tables {
+ if path.ends_with(".parquet") {
Review Comment:
Thanks for pointing this out! I looked into it, and it turned out [`register_parquet` internally use `register_listing_table` as well](https://github.com/apache/datafusion/blob/fc2fbb3d6b3aded73f1b0902168e008e580c89c1/datafusion/core/src/execution/context/parquet.rs#L55-L61). The later is capable of register both a single file and a directory of files. So I have changed the function to use `register_listing_table` only in https://github.com/apache/datafusion-ray/pull/66/commits/b8e0c6b3c10321896f39e33dee94ea843d91505a; the new commit also adds a unit test to check that it is working for both file and directory and a doc pointing to the format of the URI.
--
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] Change tpch validation to use `exec_sql_on_tables` [datafusion-ray]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #66:
URL: https://github.com/apache/datafusion-ray/pull/66#discussion_r1972273649
##########
src/util.rs:
##########
@@ -397,6 +402,52 @@ fn print_node(plan: &Arc<dyn ExecutionPlan>, indent: usize, output: &mut String)
}
}
+async fn exec_sql(query: String, tables: Vec<(String, String)>) -> PyResult<RecordBatch> {
+ let ctx = SessionContext::new();
+ for (name, path) in tables {
+ if path.ends_with(".parquet") {
Review Comment:
It may be better to check if path is a file or directory rather than basing the logic on file extension.
For example, in my local setup, I have a directory named `customer.parquet` that contains multiple Parquet files.
--
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] Change tpch validation to use `exec_sql_on_tables` [datafusion-ray]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #66:
URL: https://github.com/apache/datafusion-ray/pull/66
--
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