Skip to content

Minor: Fix incorrect indices for hashing struct - #8775

Merged
alamb merged 3 commits into
apache:mainfrom
jayzhan211:hash-column-wider-than-index
Jan 6, 2024
Merged

Minor: Fix incorrect indices for hashing struct#8775
alamb merged 3 commits into
apache:mainfrom
jayzhan211:hash-column-wider-than-index

Conversation

@jayzhan211

@jayzhan211jayzhan211 commented Jan 6, 2024

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Bug in #8552
closes#8769

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Failed test is added

Are there any user-facing changes?

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
let num_columns = array.num_columns();
let row_len = array.len();

// Skip null columns

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I forgot to correct the comment last time. Indices are all about row, not columns

let valid_row_indices: Vec<usize> = if let Some(nulls) = nulls {
nulls.valid_indices().collect()
} else {
(0..num_columns).collect()

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Main fix

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@github-actionsgithub-actionsBot added the sqllogictest SQL Logic Tests (.slt) label Jan 6, 2024
10

query RRR
query RRR rowsort

@jayzhan211jayzhan211Jan 6, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

rowsort is added because I got and groupby is not order sensitive

700 1000 850
100 600 350

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.

Yeah -- also added the same fix in #8769 and I agree this looks good

@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.

Thank you @jayzhan211

10

query RRR
query RRR rowsort

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.

Yeah -- also added the same fix in #8769 and I agree this looks good

@alamb
alamb merged commit 7316274 into apache:mainJan 6, 2024
thinkharderdev pushed a commit to coralogix/arrow-datafusion that referenced this pull request Feb 26, 2024
* fix bug
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
* fmt
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
* add rowsort
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
---------
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sqllogictestSQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@jayzhan211@alamb