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

test: re-enable window function over parquet with forced collisions #11939

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

korowa
Copy link
Contributor

@korowa korowa commented Aug 11, 2024

Which issue does this PR close?

Closes #11660.

Rationale for this change

After several attempts to reproduce the failure locally and in fork CI it still was not reproducing. Also I wasn't able to find any workflow failure related to 1.80 upgrade in PRs and on main. So, trying to just uncomment the test.

What changes are included in this PR?

Test uncommented.

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Aug 11, 2024
@alamb alamb changed the title test: window function over parquet with forced collisions test: re-enable window function over parquet with forced collisions Aug 12, 2024
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @korowa

FYI @nrc who originally reported this was failing

@alamb
Copy link
Contributor

alamb commented Aug 12, 2024

I re-ran the relevant test:

cargo test --test sqllogictests --features=force_hash_collisions -- parquet

And indeed it passes locally for me too

andrewlamb@Andrews-MBP-2:~/Software/datafusion$ cargo test --test sqllogictests --features=force_hash_collisions -- parquet
warning: function `hash_map_array` is never used
   --> datafusion/common/src/hash_utils.rs:249:4
    |
249 | fn hash_map_array(
    |    ^^^^^^^^^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default

warning: `datafusion-common` (lib) generated 1 warning
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.40s
     Running bin/sqllogictests.rs (target/debug/deps/sqllogictests-a3320b7543efea67)
Running "parquet.slt"
Running "parquet_sorted_statistics.slt"
andrewlamb@Andrews-MBP-2:~/Software/datafusion$

@@ -251,27 +251,25 @@ SELECT COUNT(*) FROM timestamp_with_tz;
----
131072

# FIXME(#TODO) fails with feature `force_hash_collisions`
# https://github.com/apache/datafusion/issues/11660
# Perform the query:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Perform the query:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to add some comments why test is so specific

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree better comments would be nice -- however I don't think this PR makes the comment any more/less cryptic, so I don't think we should hold it for better comments.

Maybe we can improve the comments as a future PR

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks agian @korowa and @comphead

@@ -251,27 +251,25 @@ SELECT COUNT(*) FROM timestamp_with_tz;
----
131072

# FIXME(#TODO) fails with feature `force_hash_collisions`
# https://github.com/apache/datafusion/issues/11660
# Perform the query:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree better comments would be nice -- however I don't think this PR makes the comment any more/less cryptic, so I don't think we should hold it for better comments.

Maybe we can improve the comments as a future PR

@alamb alamb merged commit e4be013 into apache:main Aug 14, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Window function test fails when force_hash_collisions is enabled
3 participants