-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix projection name with DataFrame::with_column and window functions #12000
Conversation
…sage Signed-off-by: Devan <devandbenz@gmail.com>
…sage Signed-off-by: Devan <devandbenz@gmail.com>
Signed-off-by: Devan <devandbenz@gmail.com>
Signed-off-by: Devan <devandbenz@gmail.com>
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.
Looks good to me -- thank you @devanbenz
cc @timsaucer thank you for the report. Perhaps you have time to review the change as well
datafusion/core/src/dataframe/mod.rs
Outdated
let func = Expr::WindowFunction(WindowFunction::new( | ||
WindowFunctionDefinition::BuiltInWindowFunction( | ||
BuiltInWindowFunction::RowNumber, | ||
), | ||
vec![], | ||
)) | ||
.alias("row_num"); |
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 you can use the expr fn here and make this more concise:
let func = Expr::WindowFunction(WindowFunction::new( | |
WindowFunctionDefinition::BuiltInWindowFunction( | |
BuiltInWindowFunction::RowNumber, | |
), | |
vec![], | |
)) | |
.alias("row_num"); | |
let func = row_number().alias("row_num"); |
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 also ran this test without the code changes and it fails like this:
assertion `left == right` failed
left: 4
right: 5
Left: 4
Right: 5
<Click to see difference>
thread 'dataframe::tests::test_window_function_with_column' panicked at datafusion/core/src/dataframe/mod.rs:2882:9:
assertion `left == right` failed
left: 4
right: 5
stack backtrace:
0: rust_begin_unwind
at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/std/src/panicking.rs:652:5
...
And the output was like
[
"+----+----+-----+-----------------------------------------------------------------------+---+",
"| c1 | c2 | c3 | ROW_NUMBER() ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING | r |",
"+----+----+-----+-----------------------------------------------------------------------+---+",
"| c | 2 | 1 | 1 | 1 |",
"| d | 5 | -40 | 2 | 2 |",
"+----+----+-----+-----------------------------------------------------------------------+---+",
]
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.
Sounds good I went ahead and used the more concise method call. Thanks!
Signed-off-by: Devan <devandbenz@gmail.com>
Signed-off-by: Devan <devandbenz@gmail.com>
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.
LGTM. I also tested locally and it produces the expected results. Thank you for taking care of this!
Thanks again @devanbenz and @timsaucer -- getting better every day! |
Signed-off-by: Devan devandbenz@gmail.com## Which issue does this PR close?
Closes #11982
Rationale for this change
Previous usage of dataframe
with_column
using a Window expression was causing an unnecessary projection in the output.I also tested manually with a few other
Expr
to test if that would cause issues by filtering the qualifier:outputs:
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?