-
Notifications
You must be signed in to change notification settings - Fork 2
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 tests on 41 branch and temporarily disable those unfixable #71
Conversation
7309009
to
802a25e
Compare
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 reasonable -- modulo the possibly-missing assert. Or did I misunderstand what's going on there?
.err() | ||
.unwrap() | ||
.message() | ||
.contains("is declared as non-nullable but contains null values"); |
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.
Should there be another assert!
around this result....contains()
check?
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.
keen eye. thanks for catching me fall
Also, maybe rename the PR to "Temporarily disable failing tests on 41 branch"? |
A change marking count(c) as non-null uncovered a bug that count(c) is sometimes null today, in certain windowed queries. The bug has already been fixed in DataFusion (PR 11989). Retracting the non-nullness marking would have downstream issues on compilation. Cherry picking the fix isn't feasible, without pulling many other changes the fix depends on.
clippy is temporarily disabled. It seems somehow that we're getting new clippy errors for old code. Are we running wrong Rust toolchain version?
Most of tests is fixed, not just disabled. |
3d702e8
to
bbd0543
Compare
No description provided.