migrate `logical_plan` tests to insta #16184

lifan-ake · 2025-05-25T03:34:45Z

Which issue does this PR close?

Closes Migrate logical_plan tests to insta #15792 .

Rationale for this change

What changes are included in this PR?

migrate logical_plan tests to insta

Are these changes tested?

Are there any user-facing changes?

lifan-ake · 2025-05-25T03:39:22Z

Hi @alamb and @blaginin , I'am trying to solve this issue #15792 .

This PR is ready to review, please take a look once you have time.

In my opinion, the doc should be easily readable by others. With normal assert expression, we can easily understand the doc. But if we use insta, there will be a learning curve for the doc. So I keep the doc unchanged.

blaginin

thanks, overall looks good - left a few comments

datafusion/expr/src/logical_plan/builder.rs

datafusion/expr/src/logical_plan/plan.rs

datafusion/expr/src/logical_plan/builder.rs

blaginin · 2025-05-25T10:53:07Z

Also can you please check the CI? Some tests are failing

blaginin

Thank you for the quick response! Just a few minor things and this PR will be good to go

datafusion/expr/src/logical_plan/builder.rs

blaginin · 2025-05-26T09:34:02Z

datafusion/expr/src/logical_plan/builder.rs

+        let DataFusionError::Internal(desc) = err else {
+            return plan_err!("Plan should have returned an DataFusionError::Internal");
+        };
+
+        let desc = desc
+            .split(DataFusionError::BACK_TRACE_SEP)
+            .collect::<Vec<&str>>()
+            .first()
+            .unwrap_or(&"")
+            .to_string();
+
+        assert_snapshot!(desc, @"trying to unnest on invalid data type UInt32");


what do you think about

Suggested change

let DataFusionError::Internal(desc) = err else {

return plan_err!("Plan should have returned an DataFusionError::Internal");

};

let desc = desc

.split(DataFusionError::BACK_TRACE_SEP)

.collect::<Vec<&str>>()

.first()

.unwrap_or(&"")

.to_string();

assert_snapshot!(desc, @"trying to unnest on invalid data type UInt32");

assert_snapshot!(err.strip_backtrace(), @r"

Internal error: trying to unnest on invalid data type UInt32.

This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker

");

that way, we won't have to manually split on the string

I tried this. When I run the test locally, the snapshot will contain the "This was likey cuased" sentence. But when I submit the code and run the CI, it will failed because the snapshot will not contain the last sentence. I am wondering why.

Thanks for the screenshot! This may be happening because locally you run tests without the backtrace feature.

As a result, when you run it without the feature, you get:

Internal error: trying to unnest on invalid data type UInt32. This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker

However, with backtrace, we get:

Internal error: trying to unnest on invalid data type UInt32. -- (backtrace) -- This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker

I think a robust solution would be adding a redaction to eliminate the backtrace and adding a dev note somewhere to apply that redaction to all tests that rely on error formatting. That said, your solution is fine too - up to you if you want to leave it as is or change.

Thank you for the explanation. And apart from redaction, how about we change the backtrace appending logic for errors? Maybe we can append the backtrace behind the output error message instead of the original error message? In that case, we may need another issue to track it.

datafusion/expr/src/logical_plan/display.rs

blaginin

Well done! I'll keep the PR open for a bit in case you want to respond to my point or if someone else wants to review.

blaginin · 2025-05-26T18:05:11Z

datafusion/expr/src/logical_plan/builder.rs

+        let DataFusionError::Internal(desc) = err else {
+            return plan_err!("Plan should have returned an DataFusionError::Internal");
+        };
+
+        let desc = desc
+            .split(DataFusionError::BACK_TRACE_SEP)
+            .collect::<Vec<&str>>()
+            .first()
+            .unwrap_or(&"")
+            .to_string();
+
+        assert_snapshot!(desc, @"trying to unnest on invalid data type UInt32");


Thanks for the screenshot! This may be happening because locally you run tests without the backtrace feature.

As a result, when you run it without the feature, you get:

Internal error: trying to unnest on invalid data type UInt32. This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker

However, with backtrace, we get:

Internal error: trying to unnest on invalid data type UInt32. -- (backtrace) -- This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker

I think a robust solution would be adding a redaction to eliminate the backtrace and adding a dev note somewhere to apply that redaction to all tests that rely on error formatting. That said, your solution is fine too - up to you if you want to leave it as is or change.

blaginin · 2025-05-26T18:08:22Z

Also, welcome to the project 🎉

lifan-ake · 2025-05-27T02:29:39Z

Hi @alamb , I think this PR is ready to merge.

alamb · 2025-05-27T16:32:25Z

Thank you @lifan-ake and @blaginin

github-actions bot added the logical-expr Logical plan and expressions label May 25, 2025

blaginin reviewed May 25, 2025

View reviewed changes

lifan-ake requested a review from blaginin May 26, 2025 03:24

lifan-ake added 6 commits May 26, 2025 17:14

migrate logical_plan tests to insta

442c61f

fix assert error

54b4f84

fix according to review

5e15c5a

strip backtrace from internal error

2eb4c05

format

fae9d63

format

bfe5884

lifan-ake force-pushed the main branch from 4e81079 to bfe5884 Compare May 26, 2025 09:14

blaginin reviewed May 26, 2025

View reviewed changes

lifan-ake added 3 commits May 26, 2025 19:23

Merge branch 'apache:main' into main

a5ef9ad

fix format("outer_query")

4e7994c

fix Internal error

c178a5a

lifan-ake requested a review from blaginin May 26, 2025 11:45

blaginin approved these changes May 26, 2025

View reviewed changes

alamb merged commit 68e26f1 into apache:main May 27, 2025
29 checks passed

migrate logical_plan tests to insta #16184

migrate logical_plan tests to insta #16184

Uh oh!

Conversation

lifan-ake commented May 25, 2025

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?

Uh oh!

lifan-ake commented May 25, 2025

Uh oh!

blaginin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blaginin commented May 25, 2025

Uh oh!

blaginin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blaginin May 26, 2025

Choose a reason for hiding this comment

Uh oh!

lifan-ake May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lifan-ake May 26, 2025

Choose a reason for hiding this comment

Uh oh!

blaginin May 26, 2025

Choose a reason for hiding this comment

Uh oh!

lifan-ake May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blaginin left a comment

Choose a reason for hiding this comment

Uh oh!

blaginin May 26, 2025

Choose a reason for hiding this comment

Uh oh!

blaginin commented May 26, 2025

Uh oh!

lifan-ake commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

alamb commented May 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

migrate `logical_plan` tests to insta #16184

migrate `logical_plan` tests to insta #16184

lifan-ake May 26, 2025 •

edited

Loading

lifan-ake May 27, 2025 •

edited

Loading

lifan-ake commented May 27, 2025 •

edited

Loading