Standardize the separator in name#10363
Conversation
github-actions
bot
added
logical-expr
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
jayzhan211
force-pushed
the
standardize-name-format
branch
from
e95b933 to
0356cf5
Compare
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
jayzhan211
changed the title
| let names: Vec<String> = | ||
| args.iter().map(create_name).collect::<Result<_>>()?; | ||
| names.join(",") | ||
| names.join(", ") |
There was a problem hiding this comment.
one of the main change
| fn display_name(&self, args: &[Expr]) -> Result<String> { | ||
| let names: Vec<String> = args.iter().map(create_name).collect::<Result<_>>()?; | ||
| Ok(format!("{}({})", self.name(), names.join(","))) | ||
| Ok(format!("{}({})", self.name(), names.join(", "))) |
There was a problem hiding this comment.
one of the main change
| #[macro_export] | ||
| macro_rules! assert_snapshot { | ||
| ($CHUNKS: expr) => { | ||
| let formatted = $crate::arrow::util::pretty::pretty_format_batches_with_options( | ||
| $CHUNKS, | ||
| &$crate::format::DEFAULT_FORMAT_OPTIONS, | ||
| ) | ||
| .unwrap() | ||
| .to_string(); | ||
|
|
||
| let actual_lines: Vec<&str> = formatted.trim().lines().collect(); | ||
|
|
||
| insta::assert_yaml_snapshot!(actual_lines); | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
test with insta. Because changing them manually is painful. I did this in datafusion-example, since they are not test.
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @jayzhan211 this looks like a really nice contributon
Given the insta tool is a new dependency and I think has larger implications than just this PR I think we should create a new ticket / PR to propose adding it to DataFusion, rather than including it as part of this unrelated PR (though I suspect one reason to use insta is to make updating the tests easier)
| ]; | ||
|
|
||
| assert_fn_batches!(expr, expected, 1); | ||
| assert_fn_batches!(expr, 1); |
There was a problem hiding this comment.
I think it was easier to see what was going on with these tests when the results are inlined -- i think insta supports that too "Inlined snapshots" -- https://insta.rs/docs/snapshot-types/
| }; | ||
|
|
||
| let phys_name = format!("{}({}{})", fun, distinct_str, names.join(",")); | ||
| let phys_name = format!("{}({}{})", fun, distinct_str, names.join(", ")); |
There was a problem hiding this comment.
Are the changes in this file the only in the PR?
There was a problem hiding this comment.
Several one, but are all like this
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
3 participants
Which issue does this PR close?
Closes #10364 .
Rationale for this change
I got the name checking error that due to the mismatch of separator in creating name. I found that most of the name built with single comma and space, so I think we should switch others to this format.
Error is like
I got one with
t2.struct(t1.time,t1.load1,t1.load2,t1.host), Utf8("c3")and anothert2.struct(t1.time,t1.load1,t1.load2,t1.host),Utf8("c3"). That is why I think we should fix the separator.What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?
You can find the error here with
cargo test --test sqllogictests -- expr. Although, it is not on the main branch, but I think most of the changes are unrelated to the name checking issue.