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

refactor: change some hashbrown RawTable uses to HashTable (round 2) #13524

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

crepererum
Copy link
Contributor

Which issue does this PR close?

For #13433, but only parts of it.

Rationale for this change

Prepare hashbrown 0.15 upgrade.

What changes are included in this PR?

  • HashTableAllocExt
  • a few migrations

Are these changes tested?

Existing tests pass.

Are there any user-facing changes?

No.

@github-actions github-actions bot added the physical-expr Physical Expressions label Nov 22, 2024
@@ -216,18 +216,18 @@ impl GroupValues for GroupValuesRows {
}
std::mem::swap(&mut new_group_values, &mut group_values);

// SAFETY: self.map outlives iterator and is not modified concurrently
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Picking this one, but this pattern occurs multiple times:

That SAFETY statement is just plain wrong: while iterating over the map, we erase elements from it. That is THE prime example for "concurrent modification of containers" and why Rust's lifetimes/reference system prevents that. I'm kinda surprised that this hasn't exploded yet.

@crepererum
Copy link
Contributor Author

The test failure (full logs here):

2024-11-22T12:00:01.8774000Z External error: query result mismatch:
2024-11-22T12:00:01.8776864Z [SQL] select make_array(make_array(1)) x UNION ALL SELECT make_array(arrow_cast(make_array(-1), 'LargeList(Int8)')) x;
2024-11-22T12:00:01.8778565Z [Diff] (-expected|+actual)
2024-11-22T12:00:01.8778975Z -   [[-1]]
2024-11-22T12:00:01.8779248Z -   [[1]]
2024-11-22T12:00:01.8779488Z +   [[1]]
2024-11-22T12:00:01.8779735Z +   [[-1]]
2024-11-22T12:00:01.8780003Z at test_files/union.slt:775

Is the order of UNION ALL even defined?

@Dandandan
Copy link
Contributor

The test failure (full logs here):

2024-11-22T12:00:01.8774000Z External error: query result mismatch:
2024-11-22T12:00:01.8776864Z [SQL] select make_array(make_array(1)) x UNION ALL SELECT make_array(arrow_cast(make_array(-1), 'LargeList(Int8)')) x;
2024-11-22T12:00:01.8778565Z [Diff] (-expected|+actual)
2024-11-22T12:00:01.8778975Z -   [[-1]]
2024-11-22T12:00:01.8779248Z -   [[1]]
2024-11-22T12:00:01.8779488Z +   [[1]]
2024-11-22T12:00:01.8779735Z +   [[-1]]
2024-11-22T12:00:01.8780003Z at test_files/union.slt:775

Is the order of UNION ALL even defined?

It isn't / shouldn't be AFAIK

@Dandandan
Copy link
Contributor

Test was fixed in #13547

@Dandandan
Copy link
Contributor

Dandandan commented Nov 25, 2024

Changes look good, can we run some benchmark?

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

Successfully merging this pull request may close these issues.

2 participants