Skip to content
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

[Dev] Fix sporadically failing test window-rows-overflow.test #8678

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

Tishj
Copy link
Contributor

@Tishj Tishj commented Aug 24, 2023

I have removed the excessive aliasing and referring to the columns by offset.

That's not the target of the test, and it looks like this has a problem that is hard to detect.
I have spent quite a lot of effort trying to reproduce this issue locally and in a docker container with no success.

I think the issue might be related to AddColumnNameToBinding, we use a case_insensitive_set_t which is an unordered_set so because this ordering is not guaranteed this might result in some undefined behavior with a different implementation of unordered_set.

The test used to alias 40 columns, but test_all_types has likely grown since then, and the behavior of these aliases + referring to columns by position i.e using #5 seems problematic.

@Tishj
Copy link
Contributor Author

Tishj commented Aug 24, 2023

Note: though I mention AddColumnNameToBinding I have looked at this and have not found a smoking gun, we use vectors to store the aliases and it also looks like the parser code is deterministic.

But maybe the way the "names" for the table are retrieved is not deterministic?

@hannes
Copy link
Member

hannes commented Aug 25, 2023

I can confirm that this fixes the issue on gcc13 for me, thanks

@hannes hannes merged commit ade3443 into duckdb:main Aug 25, 2023
24 of 25 checks passed
@Tishj
Copy link
Contributor Author

Tishj commented Aug 25, 2023

I can confirm that this fixes the issue on gcc13 for me, thanks

Do you mean the CI no longer breaks or were you able to reproduce this locally before?
Because I could not seem to reproduce it no matter what I tried

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants