Skip to content

Fix InListExpr to return the correct number of rows#8601

Merged
Dandandan merged 3 commits intoapache:mainfrom
alamb:alamb/fix_page_filter
Dec 22, 2023
Merged

Fix InListExpr to return the correct number of rows#8601
Dandandan merged 3 commits intoapache:mainfrom
alamb:alamb/fix_page_filter

Conversation

@alamb
Copy link
Copy Markdown
Contributor

@alamb alamb commented Dec 20, 2023

Which issue does this PR close?

Closes #8600

Rationale for this change

This was causing a panic for us when reading parquet files when pushing down filters . More details on #8600

What changes are included in this PR?

  1. Return the correct number of rows when evaluating InListExpr with constants

Are these changes tested?

Yes

Are there any user-facing changes?

Bug fix

@github-actions github-actions Bot added the physical-expr Changes to the physical-expr crates label Dec 20, 2023
let value = self.expr.evaluate(batch)?;
let r = match &self.static_filter {
Some(f) => f.contains(value.into_array(1)?.as_ref(), self.negated)?,
Some(f) => f.contains(value.into_array(num_rows)?.as_ref(), self.negated)?,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the actual fix -- the rest of the PR is test

@alamb alamb force-pushed the alamb/fix_page_filter branch from 15881bc to a77d6a2 Compare December 20, 2023 15:45
Some(f) => f.contains(value.into_array(1)?.as_ref(), self.negated)?,
Some(f) => f.contains(value.into_array(num_rows)?.as_ref(), self.negated)?,
None => {
let value = value.into_array(batch.num_rows())?;
Copy link
Copy Markdown
Contributor

@Dandandan Dandandan Dec 21, 2023

Choose a reason for hiding this comment

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

Maybe we can use the let value = value.into_array(batch.num_rows())?; for both arms here (before matching).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea -- done in 7cde104

@Dandandan Dandandan merged commit 55121d8 into apache:main Dec 22, 2023
@Dandandan
Copy link
Copy Markdown
Contributor

Thanks @alamb !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Evaluating InList without columns always returns a record batch with 1 row (causing panic "selection contains less than the number of selected row")

3 participants