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.

Sign up for GitHub

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

Jump to bottom

[GLUTEN-7261][CORE] Support offloading partial filters to native scan #8082

Merged
merged 4 commits into from
Dec 11, 2024
Merged

[GLUTEN-7261][CORE] Support offloading partial filters to native scan #8082

merged 4 commits into from
Dec 11, 2024

Conversation

zml1206
Copy link
Contributor

@zml1206 zml1206 commented Nov 28, 2024 •
edited by rui-mo
Loading

What changes were proposed in this pull request?

Scan used to fallback if there was an unsupported filter. This PR filters out the supported expressions and offloads scan as much as possible to improve the performance. Before this change, the plan was "vanilla vectorized scan + c2r + vanilla filter" when scan contained an unsupported filter, and now the plan becomes "native scan + c2r + vanilla filter".

(Fixes: #7261)

How was this patch tested?

UT

@github-actions github-actions bot added CORE works for Gluten Core VELOX labels Nov 28, 2024
@zml1206 zml1206 changed the title [GLUTEN-7261][CORE] Use pushedFilters to offload scan when filter need fallbac [GLUTEN-7261][CORE] Use pushedFilters to offload scan when filter need fallback Nov 28, 2024
@github-actions GitHub Actions
Copy link

#7261

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

1 similar comment
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

@zml1206
Copy link
Contributor Author

zml1206 commented Nov 28, 2024

cc @FelixYBW it can resolve #7261

@zml1206 zml1206 requested a review from zhztheplayer November 29, 2024 00:57
@zhztheplayer
Copy link
Member

cc @rui-mo

zhztheplayer
zhztheplayer reviewed Dec 4, 2024
View reviewed changes
gluten-substrait/src/main/scala/org/apache/gluten/execution/ScanTransformerFactory.scala Outdated
transform.copy(dataFilters = PushDownUtil.pushFilters(scanExec.dataFilters))
} else {
transform
}
Copy link
Member

@zhztheplayer zhztheplayer Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The code in ScanTransformerFactory is used by validator and offload rules. It feels a little weird to do validation in it? Do we have better choices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about use only pushedFilter here and rely on PushDownFilterToScan for subsequent pushdown?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds feasible to me. Thanks.

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 4, 2024

Run Gluten Clickhouse CI on x86

@zml1206 zml1206 changed the title [GLUTEN-7261][CORE] Use pushedFilters to offload scan when filter need fallback [GLUTEN-7261][CORE] Use pushedFilters instead of dataFilters to offload scan Dec 4, 2024
@zml1206
Copy link
Contributor Author

zml1206 commented Dec 4, 2024

Test failure seems unrelated.

rui-mo
rui-mo reviewed Dec 4, 2024
View reviewed changes
Copy link
Contributor

@rui-mo rui-mo left a comment

Choose a reason for hiding this comment

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

Thanks. Added some questions.

gluten-substrait/src/main/scala/org/apache/spark/sql/utils/PushDownUtil.scala Outdated Show resolved Hide resolved
@zml1206 zml1206 marked this pull request as draft December 6, 2024 01:12
@github-actions github-actions bot added DATA_LAKE and removed VELOX labels Dec 6, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 6, 2024

Run Gluten Clickhouse CI on x86

@zml1206 zml1206 changed the title [GLUTEN-7261][CORE] Use pushedFilters instead of dataFilters to offload scan [GLUTEN-7261][CORE] Push partial filters to offload scan when filter need fallback Dec 6, 2024
@zml1206 zml1206 marked this pull request as ready for review December 6, 2024 07:13
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 6, 2024

Run Gluten Clickhouse CI on x86

@github-actions github-actions bot added the VELOX label Dec 6, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 6, 2024

Run Gluten Clickhouse CI on x86

@zml1206 zml1206 requested a review from rui-mo December 10, 2024 05:32
rui-mo
rui-mo approved these changes Dec 10, 2024
View reviewed changes
Copy link
Contributor

@rui-mo rui-mo left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good overall.

gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala Outdated Show resolved Hide resolved
gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala Outdated Show resolved Hide resolved
gluten-substrait/src/main/scala/org/apache/gluten/execution/BatchScanExecTransformer.scala Outdated Show resolved Hide resolved
rui-mo
rui-mo reviewed Dec 10, 2024
View reviewed changes
Copy link
Contributor

@rui-mo rui-mo left a comment

Choose a reason for hiding this comment

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

Regarding the plan change, we used to get 'vanilla scan + vanilla filter' for unsupported filter, and now we can get 'native scan + c2r + vanilla filter'. The new plan offloads 'scan' to native while I assume when the c2r is large and time-consuming we might not get performance improvement. Can our RAS strategy cover this case? cc: @zhztheplayer Thanks.

@zml1206
Copy link
Contributor Author

zml1206 commented Dec 10, 2024

Regarding the plan change, we used to get 'vanilla scan + vanilla filter' for unsupported filter, and now we can get 'native scan + c2r + vanilla filter'. The new plan offloads 'scan' to native while I assume when the c2r is large and time-consuming we might not get performance improvement. Can our RAS strategy cover this case? cc: @zhztheplayer Thanks.

RAS currently cannot solve this problem, but from our production point of view, the cost of c2r is relatively small. @zhztheplayer What do you think?

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI on x86

1 similar comment
@zml1206
Copy link
Contributor Author

zml1206 commented Dec 10, 2024

Run Gluten Clickhouse CI on x86

@rui-mo
Copy link
Contributor

rui-mo commented Dec 11, 2024 •
edited
Loading

from our production point of view, the cost of c2r is relatively small

With the previous plan, C2R is only needed for rows after the filter, whose number might be largely reduced, while in the new plan, all rows need to be converted as rows. Perhaps in some cases the speedup of native scan cannot compensate for this overhead, and we might get performance regression. Perhaps we could optimize the plan in the future to choose between the two options with the RAS strategy.

@zml1206
Copy link
Contributor Author

zml1206 commented Dec 11, 2024

from our production point of view, the cost of c2r is relatively small

With the previous plan, C2R is only needed for rows after the filter, whose number might be largely reduced, while in the new plan, all rows need to be converted as rows. Perhaps in some cases the speedup of native scan cannot compensate for this overhead, and we might get performance regression.

Before PR is Scan + ColumnToRow + Filter, After PR is nativeScan + VeloxColumnToRow + Filter, the number of rows after nativeScan should be less than or equal to that after scan.

@rui-mo
Copy link
Contributor

rui-mo commented Dec 11, 2024 •
edited
Loading

Before PR is Scan + ColumnToRow + Filter

Why do we have the C2R between, because for unsupported filters they should be vanilla Spark computed? Please note that if a filter causes fallback of Scan, the filter must also fallback.

@zml1206
Copy link
Contributor Author

zml1206 commented Dec 11, 2024

Why do we have the C2R between, because for unsupported filters they should be vanilla Spark computed?

The default for vanilla Spark reading parquet is vectorization.

@rui-mo
Copy link
Contributor

rui-mo commented Dec 11, 2024

@zml1206 I see your point. Thanks for explaning!

@rui-mo rui-mo changed the title [GLUTEN-7261][CORE] Push partial filters to offload scan when filter need fallback [GLUTEN-7261][CORE] Support offloading partial filters to native scan Dec 11, 2024
@rui-mo rui-mo merged commit 1036c96 into apache:main Dec 11, 2024
48 checks passed
yikf pushed a commit to yikf/incubator-gluten that referenced this pull request Dec 13, 2024
@zml1206 @yikf
[GLUTEN-7261][CORE] Support offloading partial filters to native scan (…
1f693c3 
…apache#8082)

Scan used to fallback if there was an unsupported filter. This PR filters out the supported expressions and offloads scan as much as possible to improve the performance. Before this change, the plan was "vanilla vectorized scan + c2r + vanilla filter" when scan contained an unsupported filter, and now the plan becomes "native scan + c2r + vanilla filter".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhztheplayer zhztheplayer zhztheplayer left review comments

@rui-mo rui-mo rui-mo approved these changes

Assignees
No one assigned
Labels
CORE works for Gluten Core DATA_LAKE VELOX
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[VL] offload table scan when filter need fallback
3 participants
@zml1206 @zhztheplayer @rui-mo