Skip to content

Conversation

@findepi
Copy link

@findepi findepi commented Oct 23, 2024

No description provided.

Copy link

@vgapeyev vgapeyev left a 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");

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?

Copy link
Author

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

@vgapeyev
Copy link

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?
@findepi findepi changed the title Fix tests on 41 branch Fix tests on 41 branch and temporarily disable those unfixable Oct 23, 2024
@findepi
Copy link
Author

findepi commented Oct 23, 2024

Also, maybe rename the PR to "Temporarily disable failing tests on 41 branch"?

Most of tests is fixed, not just disabled.
Changed to "Fix tests on 41 branch and temporarily disable those unfixable"

@findepi findepi merged commit de130ee into findepi/41 Oct 24, 2024
26 checks passed
@findepi findepi deleted the findepi/fix-df-41 branch October 24, 2024 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants