fix: Substrait serializer clippy error: not calling truncate #14723

niebayes · 2025-02-17T09:12:38Z

Which issue does this PR close?

Closes Substrait serializer clippy error: not calling truncate #9727

Rationale for this change

It's reasonable to always truncate an existing file before serializing a plan.
On the other hand, the following deserialize method always read the file to end which does not consider the case that the file is not truncated before serialization.

What changes are included in this PR?

Applied a clippy suggestion by adding a truncate(true).

Are these changes tested?

Tested by clippy

Are there any user-facing changes?

No

alamb

Thank you @niebayes -- I think this is an improvment, but it would be better if:

Return an error of the file already exists
Added doccomments comments to serialize

datafusion/substrait/src/serializer.rs

mbrobbel · 2025-02-18T09:03:35Z

datafusion/substrait/src/serializer.rs

+    match std::fs::metadata(path.as_ref()) {
+        Ok(meta) if meta.len() > 0 => {
+            return Err(DataFusionError::Substrait(format!(
+                "Failed to encode substrait plan: the file {} already exists and is not empty", 


You could remove substrait here because it's already part of the error prefix

datafusion/datafusion/common/src/error.rs

Line 517 in 19fe44c

DataFusionError::Substrait(_) => "Substrait error: ",

Suggested change

"Failed to encode substrait plan: the file {} already exists and is not empty",

"Failed to encode plan: the file {} already exists and is not empty",

Adding a substrait is because there already had an error message "Failed to encode substrait plan" in the fuction serialize_bytes. I was following that error message to make error messages consistent.

Removed the word "substrait" from all error messages.

datafusion/substrait/src/serializer.rs

alamb

Thank you @niebayes and @mbrobbel -- looks great to me

I think @mbrobbel 's recent comments and suggestions are worth considering too as they would improve the code substantially, but we can also do it as a follow on PR too

Co-authored-by: Matthijs Brobbel <m1brobbel@gmail.com>

niebayes · 2025-02-18T16:11:08Z

@alamb Applied suggestions from @mbrobbel. Please check it.

datafusion/substrait/src/serializer.rs

Co-authored-by: Matthijs Brobbel <m1brobbel@gmail.com>

mbrobbel

Thanks! I added some follow-up suggestions.

datafusion/substrait/tests/cases/serialize.rs

mbrobbel · 2025-02-19T08:13:59Z

datafusion/substrait/src/serializer.rs

@@ -56,8 +67,9 @@ pub async fn deserialize(path: &str) -> Result<Box<Plan>> {
    deserialize_bytes(protobuf_in).await
 }

+/// Deserializes a plan from the bytes.


Follow-up suggestion (breaking change): this function is marked async but it doesn't have to be.

mbrobbel · 2025-02-19T08:15:10Z

datafusion/substrait/src/serializer.rs

+/// Reads the file at `path` and deserializes a plan from the bytes.
+pub async fn deserialize(path: impl AsRef<Path>) -> Result<Box<Plan>> {


Follow-up suggestion (breaking change): this function is marked async but it doesn't have to be (see comment on deserialize_bytes).

Suggested change

/// Reads the file at `path` and deserializes a plan from the bytes.

pub async fn deserialize(path: impl AsRef<Path>) -> Result<Box<Plan>> {

/// Reads the file at `path` and deserializes a plan from the bytes.

pub fn deserialize(path: impl AsRef<Path>) -> Result<Box<Plan>> {

I plan to contribute more to the datafusion-substrait crate. Personally, I would like to reserve the breaking change in one of the following PR.

What do you think? @alamb

Agree -- let's discuss API changes in follow on PRs / issues

mbrobbel · 2025-02-19T08:17:08Z

datafusion/substrait/src/serializer.rs

+    let mut file = OpenOptions::new().write(true).create_new(true).open(path)?;
    file.write_all(&protobuf_out?)?;


Follow-up suggestion: use tokio::io::AsyncWriteExt::write_all instead of the blocking std::io::Write::write_all in this async function.

I have replaced the sync OpenOptions with the async counterpart backed by tokio.

mbrobbel · 2025-02-19T08:20:10Z

datafusion/substrait/src/serializer.rs

+/// Plans a sql and serializes the generated logical plan to bytes.
 pub async fn serialize_bytes(sql: &str, ctx: &SessionContext) -> Result<Vec<u8>> {


If we replace the sql arg with a DataFrame arg, this function (and serialize) can be non-async.

Not sure which one is preferable for users.
@alamb cc

I think we should leave this one as a str in this PR and we can potentially add a DataFrame specific one as a follow on

However, given there is already an API to serialize LogicalPlans directly I am not sure how much more value a DataFrame one would add

https://docs.rs/datafusion-substrait/latest/datafusion_substrait/#example-serializing-logicalplans

Co-authored-by: Matthijs Brobbel <m1brobbel@gmail.com>

alamb

Thanks again @niebayes and @mbrobbel

specify truncate true

d9bcf6c

github-actions bot added the substrait Changes to the substrait crate label Feb 17, 2025

alamb approved these changes Feb 17, 2025

View reviewed changes

datafusion/substrait/src/serializer.rs Outdated Show resolved Hide resolved

add error handling

3696ff7

mbrobbel reviewed Feb 18, 2025

View reviewed changes

alamb approved these changes Feb 18, 2025

View reviewed changes

niebayes and others added 2 commits February 19, 2025 00:05

Apply suggestions from code review

2b07ec4

Co-authored-by: Matthijs Brobbel <m1brobbel@gmail.com>

remove substrait from error messages

5ce1ec8

mbrobbel reviewed Feb 18, 2025

View reviewed changes

datafusion/substrait/src/serializer.rs Outdated Show resolved Hide resolved

niebayes and others added 2 commits February 19, 2025 00:26

Apply suggestions from code review

13182cd

Co-authored-by: Matthijs Brobbel <m1brobbel@gmail.com>

simplify serialize

907b6ce

mbrobbel approved these changes Feb 18, 2025

View reviewed changes

fix ut

7cb405a

mbrobbel reviewed Feb 19, 2025

View reviewed changes

niebayes and others added 2 commits February 19, 2025 17:14

Update datafusion/substrait/tests/cases/serialize.rs

398d8e4

Co-authored-by: Matthijs Brobbel <m1brobbel@gmail.com>

apply part of follow-up suggestions

f7d2053

alamb approved these changes Feb 20, 2025

View reviewed changes

alamb merged commit 2b39b84 into apache:main Feb 20, 2025
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Substrait serializer clippy error: not calling truncate #14723

fix: Substrait serializer clippy error: not calling truncate #14723

niebayes commented Feb 17, 2025 •

edited

Loading

alamb left a comment

mbrobbel Feb 18, 2025

niebayes Feb 18, 2025

niebayes Feb 18, 2025

alamb left a comment

niebayes commented Feb 18, 2025

mbrobbel left a comment

mbrobbel Feb 19, 2025

mbrobbel Feb 19, 2025

niebayes Feb 19, 2025 •

edited

Loading

alamb Feb 19, 2025

mbrobbel Feb 19, 2025

niebayes Feb 19, 2025

mbrobbel Feb 19, 2025

niebayes Feb 19, 2025

alamb Feb 19, 2025

alamb left a comment

	"Failed to encode substrait plan: the file {} already exists and is not empty",
	"Failed to encode plan: the file {} already exists and is not empty",

		/// Reads the file at `path` and deserializes a plan from the bytes.
		pub async fn deserialize(path: impl AsRef<Path>) -> Result<Box<Plan>> {

		let mut file = OpenOptions::new().write(true).create_new(true).open(path)?;
		file.write_all(&protobuf_out?)?;

		/// Plans a sql and serializes the generated logical plan to bytes.
		pub async fn serialize_bytes(sql: &str, ctx: &SessionContext) -> Result<Vec<u8>> {

fix: Substrait serializer clippy error: not calling truncate #14723

fix: Substrait serializer clippy error: not calling truncate #14723

Conversation

niebayes commented Feb 17, 2025 • edited Loading

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?

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

niebayes commented Feb 18, 2025

mbrobbel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niebayes Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

niebayes commented Feb 17, 2025 •

edited

Loading

niebayes Feb 19, 2025 •

edited

Loading