You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "crepererum (via GitHub)" <gi...@apache.org> on 2023/05/13 01:00:11 UTC
[GitHub] [arrow-datafusion] crepererum opened a new pull request, #6348: refactor: split `CommonSubexprEliminate::try_optimize`
crepererum opened a new pull request, #6348:
URL: https://github.com/apache/arrow-datafusion/pull/6348
# Which issue does this PR close?
Fixes #6277.
# Rationale for this change
Having a single method w/ all optimization algorithms lets rustc+LLVM pick the largest possible stack size over all branches when compiled under the `dev` profile (e.g. for tests). So even if there's a deeply nested projection (e.g. for `tpcds_logical_q64`), we pay for the more complex aggregation optimization with every recursion.
Splitting the method into sub-methods seems to fix this.
# What changes are included in this PR?
Just moving code around, no functional change.
# Are these changes tested?
Manually w/:
```bash
#!/usr/bin/env bash
pushd datafusion/core
export RUST_MIN_STACK=1800000
exec "$@"
```
and
```console
$ cargo with './exec.sh {bin} {args}' -- test -p datafusion --test tpcds_planning -- tpcds_logical_q64 --nocapture
```
# Are there any user-facing changes?
Less stack usage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] crepererum commented on pull request #6348: refactor: split `CommonSubexprEliminate::try_optimize`
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on PR #6348:
URL: https://github.com/apache/arrow-datafusion/pull/6348#issuecomment-1546476925
@mingmwang could you please verify that this fixes the issue on your system?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] crepererum commented on pull request #6348: refactor: split `CommonSubexprEliminate::try_optimize`
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on PR #6348:
URL: https://github.com/apache/arrow-datafusion/pull/6348#issuecomment-1560731206
Looks like #6277 is no longer reproducible. Since in my test this PR here improved stack usage and also leads to cleaner code, I'll rebase and merge it anyways.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] crepererum merged pull request #6348: refactor: split `CommonSubexprEliminate::try_optimize`
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum merged PR #6348:
URL: https://github.com/apache/arrow-datafusion/pull/6348
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] waynexia commented on pull request #6348: refactor: split `CommonSubexprEliminate::try_optimize`
Posted by "waynexia (via GitHub)" <gi...@apache.org>.
waynexia commented on PR #6348:
URL: https://github.com/apache/arrow-datafusion/pull/6348#issuecomment-1556454533
>I found the diff easier to understand by reviewing https://github.com/apache/arrow-datafusion/pull/6348/files?w=1
Thanks for this 👍 Hidding white space makes it much easier to review.
This patch looks good to me. But I'm not sure if the original issue is fixed as I cannot reproduce it with 8k stack environment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] crepererum commented on pull request #6348: refactor: split `CommonSubexprEliminate::try_optimize`
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on PR #6348:
URL: https://github.com/apache/arrow-datafusion/pull/6348#issuecomment-1556770606
Ping @mingmwang .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mingmwang commented on pull request #6348: refactor: split `CommonSubexprEliminate::try_optimize`
Posted by "mingmwang (via GitHub)" <gi...@apache.org>.
mingmwang commented on PR #6348:
URL: https://github.com/apache/arrow-datafusion/pull/6348#issuecomment-1547913629
@crepererum
I will verify this PR tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org