Skip to content

Fix panic when hashing empty FixedSizeList Array - #13533

Merged
alamb merged 2 commits into
apache:mainfrom
findepi:findepi/fix-panic-when-hashing-empty-fixedsizelist-array-ff5e68
Nov 24, 2024
Merged

Fix panic when hashing empty FixedSizeList Array#13533
alamb merged 2 commits into
apache:mainfrom
findepi:findepi/fix-panic-when-hashing-empty-fixedsizelist-array-ff5e68

Conversation

@findepi

Copy link
Copy Markdown
Member

Previously it would panic due to division by zero.

@github-actionsgithub-actionsBot added the common Related to common crate label Nov 22, 2024
Previously it would panic due to division by zero.
@findepi
findepiforce-pushed the findepi/fix-panic-when-hashing-empty-fixedsizelist-array-ff5e68 branch from 3ea432f to 28e3b99CompareNovember 22, 2024 21:42
Comment threaddatafusion/common/src/hash_utils.rs Outdated
Comment on lines +325 to +329
let offset_size = if array.len() == 0 {
0
} else {
value_len as usize / array.len()
};

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Would this be equiv to array.value_length()?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think value_length is different

Like a FixedSlizeLIst(3) would have elements that are each 3 bytes, but the array can have any number of elements (rows)

So in that case value_length() is 3

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

in this example, value_length() is 3
array.len() is something, say N
so i though that value_len is 3 * N

ie. i kind of suspect this division is calculating what is know from the array type (the 3 in the example)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤔 https://docs.rs/arrow/latest/arrow/array/struct.FixedSizeBinaryArray.html#method.value_length

I don't understand what this offset_size is really used for to be honest -- it doesn't make a lot of sense below either

It seems the intention is more like offset_size = array.value_length() 🤔

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It seems the intention is more like offset_size = array.value_length() 🤔

that's my take too

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me -- thank you @findepi

Comment threaddatafusion/common/src/hash_utils.rs Outdated
Comment on lines +325 to +329
let offset_size = if array.len() == 0 {
0
} else {
value_len as usize / array.len()
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think value_length is different

Like a FixedSlizeLIst(3) would have elements that are each 3 bytes, but the array can have any number of elements (rows)

So in that case value_length() is 3

@findepi
findepiforce-pushed the findepi/fix-panic-when-hashing-empty-fixedsizelist-array-ff5e68 branch from 69136a2 to 4b46b8aCompareNovember 23, 2024 18:00
@findepi
findepi requested a review from alambNovember 23, 2024 18:00

@Weijun-HWeijun-H left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thanks @findepi

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good to me. THanks @findepi and @Weijun-H

@alamb
alamb merged commit 789390e into apache:mainNov 24, 2024
@findepi
findepi deleted the findepi/fix-panic-when-hashing-empty-fixedsizelist-array-ff5e68 branch November 25, 2024 19:26
@alambalamb mentioned this pull request Dec 3, 2024
3 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commonRelated to common crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@findepi@alamb@Weijun-H