-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[sqllogictest] Run tests on Windows #5870
Conversation
if let Ok(mut statements) = DFParser::parse_sql(&sql) { | ||
statements.pop_front().expect("at least one SQL statement"); | ||
} |
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 piece was needed to have workarounds for creating tables without values. It is not needed anymore.
Ok(()) | ||
pub fn main() { | ||
// Tests from `tpch.slt` fail with stackoverflow with the default stack size. | ||
thread::Builder::new() |
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.
tpch.slt
test fails with stack overflow errors. The stack size on Windows seems to be 1Mb by default vs 2MB on Linux. I tried debugging what causes the error, but it seems that the stack is just deep enough due to the recursive design of the physical planner.
In case somebody with more understanding takes a closer look, I added a stack trace for the overflow as well as a test to reproduce the issue with unit tests on any platform here https://gist.github.com/melgenek/b6acf4467ee10c1941f3b103a17e1b1e.
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.
We have hit this before (e.g. #4065) -- I think cranking up the default size is fine.
Specifically, release builds seem to use much less stack so this is primarily a test issue rather than a production 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 -- thank you @melgenek
I also verified that the windows CI run indeed is running sqllogictests now: https://github.com/apache/arrow-datafusion/actions/runs/4611787296/jobs/8151915512?pr=5870#step:5:2877
Ok(()) | ||
pub fn main() { | ||
// Tests from `tpch.slt` fail with stackoverflow with the default stack size. | ||
thread::Builder::new() |
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.
We have hit this before (e.g. #4065) -- I think cranking up the default size is fine.
Specifically, release builds seem to use much less stack so this is primarily a test issue rather than a production issue
Which issue does this PR close?
Closes #4494.
Rationale for this change
Run sqllogictest tests in Windows in Github actions. These tests are currently disabled.
What changes are included in this PR?
Enable sqllogictest tests in Windows in Github actions. Additionally, there are some tricks to make tests work.
Are these changes tested?
The change is a test only and runs in ci.
Are there any user-facing changes?
N/A