-
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
minor: port more window tests to sqllogictests #5330
Conversation
#// } | ||
|
||
# async fn window_frame_ranges_preceding_following_desc() -> Result<()> { | ||
query error DataFusion error: Internal error: Operator + is not implemented for types Int8(5) and Utf8("1"). 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 |
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.
This one is strange to me -- it seems like a real bug -- if you agree I will file a ticket for it.
However, the corresponding query ran just fine in .rs rust 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.
filed #5346
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's a bug, I also met it in other code when disable skip_failed
@@ -164,7 +164,7 @@ pub fn regularize(mut frame: WindowFrame, order_bys: usize) -> Result<WindowFram | |||
} | |||
} else { | |||
return Err(DataFusionError::Plan(format!( | |||
"With window frame of type RANGE, the ORDER BY expression must be of length 1, got {}", order_bys))); | |||
"With window frame of type RANGE, the ORDER BY expression must be of length 1, got {order_bys}"))); |
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.
clippy was complaining about this for me locally
cc @mustafasrepo and @ozankabak |
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, we will use the SLT functionality for new tests too from now on. Some CI checks are failing though due to an error message matching issue.
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 prepare to merge this PR in one day in case of new conflict.
Thanks everyone! |
Benchmark runs are scheduled for baseline = a79f9c8 and contender = 3882eea. 3882eea is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
* minor: port more window tests * Update comments and add link to ticket * escape
Which issue does this PR close?
Part of #4495
Follow on to #5199
Rationale for this change
I am trying to keep the testability of DataFusion reasonable. Sqllogictests are easier to add / update so we are trying to move stuff over there.
What changes are included in this PR?
Port more window tests to window.slt
Are these changes tested?
Yes all tests
Are there any user-facing changes?
No