You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "hntd187 (via GitHub)" <gi...@apache.org> on 2025/12/15 17:06:06 UTC
[PR] Add Spark 4 protobuf definitions and basic working [spark-connect-rust]
hntd187 opened a new pull request, #11:
URL: https://github.com/apache/spark-connect-rust/pull/11
### What changes were proposed in this pull request?
Adding the changed protobuf (and new) definitions for Spark 4 this will allow us to implement the new Spark 4 capabilities. This is still a WIP
### Why are the changes needed?
Spark updates things, so do we.
### Does this PR introduce _any_ user-facing change?
Yes, some interfaces will change, but for the most part user code that worked prior should still work.
### How was this patch tested?
By Running the tests, but running this on windows has always been difficult to see how well it actually works.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Spark 4 protobuf definitions and basic working [spark-connect-rust]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #11: Add Spark 4 protobuf definitions and basic working
URL: https://github.com/apache/spark-connect-rust/pull/11
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Spark 4 protobuf definitions and basic working [spark-connect-rust]
Posted by "hntd187 (via GitHub)" <gi...@apache.org>.
hntd187 commented on PR #11:
URL: https://github.com/apache/spark-connect-rust/pull/11#issuecomment-3761022196
I'd just like to merge this to get moving on some follow up PRs so I asked to have this reviewed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Spark 4 protobuf definitions and basic working [spark-connect-rust]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on PR #11:
URL: https://github.com/apache/spark-connect-rust/pull/11#issuecomment-3717516799
@xuanyuanking Is this PR still WIP as mentioned in the description right?
```
This is still a WIP
```
So, I marked this as draft.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Spark 4 protobuf definitions and basic working [spark-connect-rust]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #11:
URL: https://github.com/apache/spark-connect-rust/pull/11#issuecomment-5403906902
Superseded by #50, which delivers the consolidated Rust core Spark Connect client (now merged to `master`) and incorporates this work. Closing to keep the queue focused on the drop-in Python wrapper (#51). Thanks for the contribution — please reopen if anything here isn't covered by #50.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Spark 4 protobuf definitions and basic working [spark-connect-rust]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #11:
URL: https://github.com/apache/spark-connect-rust/pull/11#discussion_r2803126628
##########
crates/connect/src/expressions.rs:
##########
@@ -196,6 +215,7 @@ impl From<chrono::NaiveDate> for spark::expression::Literal {
value.signed_duration_since(chrono::NaiveDate::from_ymd_opt(1970, 1, 1).unwrap());
spark::expression::Literal {
+ data_type: Some(spark::DataType::from(14)),
Review Comment:
Isn't `#[cfg(feature = "spark-41")]` needed here?
##########
crates/connect/src/dataframe.rs:
##########
@@ -2542,37 +2566,6 @@ mod tests {
Ok(())
}
- #[tokio::test]
Review Comment:
Will this test case be rewritten using the Arrow C FFI interface in the future?
##########
crates/connect/src/plan.rs:
##########
@@ -834,6 +840,8 @@ impl From<spark::relation::RelType> for LogicalPlanBuilder {
common: Some(RelationCommon {
source_info: "".to_string(),
plan_id: Some(plan_id),
+ #[cfg(spark41)]
Review Comment:
```
#[cfg(feature = "spark-41")]
let relation = Relation {
common: Some(RelationCommon {
source_info: "".to_string(),
plan_id: Some(plan_id),
origin: None,
}),
rel_type: Some(rel_type),
};
#[cfg(not(feature = "spark-41"))]
let relation = Relation {
common: Some(RelationCommon {
source_info: "".to_string(),
plan_id: Some(plan_id),
}),
rel_type: Some(rel_type),
};
```
Maybe it could be changed like this? Would this allow us to avoid defining `spark41`?
##########
crates/connect/src/expressions.rs:
##########
@@ -147,6 +158,8 @@ impl From<&[u8]> for spark::expression::Literal {
impl From<i16> for spark::expression::Literal {
fn from(value: i16) -> Self {
spark::expression::Literal {
+ #[cfg(feature = "spark-41")]
+ data_type: Some(spark::DataType::from(5)),
Review Comment:
Using hard-coded numbers to represent Spark data types results in poor readability and maintainability.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Spark 4 protobuf definitions and basic working [spark-connect-rust]
Posted by "franciscoabsampaio (via GitHub)" <gi...@apache.org>.
franciscoabsampaio commented on PR #11:
URL: https://github.com/apache/spark-connect-rust/pull/11#issuecomment-4390578689
Hi!
Thanks for the contribution.
I am, in general, against solutions that involve hardcoding the Spark version to the crate's internals.
Spark 3 is also still the most widely used version (and definitely the most mature), so I am of the opinion that the Rust crate should support different versions.
Of course, as functionality changes significantly, support for older versions can (and should) drop, but since this crate doesn't even have a 1.0 release, it seems too rushed to move to Spark 4 already.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org