-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Standardize the separator in name #10363
Conversation
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
e95b933
to
0356cf5
Compare
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
@@ -1859,7 +1859,7 @@ pub(crate) fn create_name(e: &Expr) -> Result<String> { | |||
AggregateFunctionDefinition::UDF(..) => { | |||
let names: Vec<String> = | |||
args.iter().map(create_name).collect::<Result<_>>()?; | |||
names.join(",") | |||
names.join(", ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one of the main change
@@ -291,7 +291,7 @@ pub trait ScalarUDFImpl: Debug + Send + Sync { | |||
/// | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we use this for tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test with insta
. Because changing them manually is painful. I did this in datafusion-example
, since they are not test
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/
@@ -121,7 +121,7 @@ fn create_function_physical_name( | |||
false => "", | |||
}; | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the changes in this file the only in the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.