Skip to content

backport #5204 to Spark3.2#5227

Merged
szehon-ho merged 1 commit intoapache:masterfrom
huaxingao:complex_filter
Jul 8, 2022
Merged

backport #5204 to Spark3.2#5227
szehon-ho merged 1 commit intoapache:masterfrom
huaxingao:complex_filter

Conversation

@huaxingao
Copy link
Contributor

This PR backports the changes in #5204 to Spark 3.2

@github-actions github-actions bot added the spark label Jul 8, 2022
@huaxingao
Copy link
Contributor Author

@szehon-ho Do we need to back-port to 3.1, 3.0 and 2.4 too?

@szehon-ho
Copy link
Member

szehon-ho commented Jul 8, 2022

@huaxingao yea I think its always beneficial if the issue exists in that Spark version, I was more eager about fixing 3.2 personally but I think we can do it to the others as well.

@szehon-ho szehon-ho merged commit b0937a4 into apache:master Jul 8, 2022
@szehon-ho
Copy link
Member

Thanks @huaxingao for backport and @hililiwei for additional review

@huaxingao
Copy link
Contributor Author

Thanks a lot @szehon-ho @hililiwei

@huaxingao huaxingao deleted the complex_filter branch July 8, 2022 18:38
try {
expr = SparkFilters.convert(filter);
} catch (IllegalArgumentException e) {
// converting to Iceberg Expression failed, so this expression cannot be pushed down
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably log something here

Copy link
Member

@szehon-ho szehon-ho Jul 8, 2022

Choose a reason for hiding this comment

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

Yea there was a similar comment on : #5204 (comment), but I think the point was the pushed filters are logged.

I'm ok to log unsuccessful filters too if it helps (could also add the exprs unsuccessfully bound as well in the next statement)

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be useful to know that a filter was intentionally not pushed down for a particular reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am OK to have a follow-up to log the unsuccessful filters and also the unsuccessfully bound exprs in the next statement

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants