You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "milenkovicm (via GitHub)" <gi...@apache.org> on 2026/03/12 19:17:50 UTC
[PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
milenkovicm opened a new pull request, #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503
# Which issue does this PR close?
Closes #.
# Rationale for this change
Simplify shuffle reader in order to implement connection caching
# What changes are included in this PR?
- shuffle reader code refactor
- ballista configuration cleanup
# Are there any user-facing changes?
No
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#discussion_r2929629676
##########
ballista/core/src/config.rs:
##########
@@ -419,6 +427,23 @@ impl BallistaConfig {
v.parse::<f64>().unwrap()
}
}
+ /// sets the configuration value where key starts with ballista
+ /// prefix.
+ pub fn set_with_prefix(
+ &mut self,
+ key: &str,
+ value: &str,
+ ) -> datafusion::error::Result<()> {
+ let entries = Self::valid_entries();
+ //let k = format!("{}.{key}", BallistaConfig::PREFIX);
+
+ if entries.contains_key(key) {
+ self.settings.insert(key.to_string(), value.to_string());
Review Comment:
It would make sense
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "sqlbenchmark (via GitHub)" <gi...@apache.org>.
sqlbenchmark commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4049565739
## Ballista TPC-H Benchmark Results
**PR:** #1503 - shuffle (remote) reader cleanup
**PR Commit:** `79d07981`
**Base Commit:** `7d51c593` (main)
**Scale Factor:** SF1
**Iterations:** 3
### Query Comparison
| Query | main (ms) | PR (ms) | Change |
|-------|-----------|---------|--------|
| Q1 | 214.20 | 221.20 | :white_circle: +3.3% |
| Q2 | 102.20 | 122.20 | :red_circle: +19.6% |
| Q3 | 138.70 | 144.60 | :white_circle: +4.3% |
| Q4 | 74.50 | 81.40 | :red_circle: +9.3% |
| Q5 | 272.70 | 294.90 | :red_circle: +8.1% |
| Q6 | 60.20 | 61.10 | :white_circle: +1.5% |
| Q7 | 390.60 | 400.90 | :white_circle: +2.6% |
| Q8 | 318.70 | 350.20 | :red_circle: +9.9% |
| Q9 | 452.40 | 507.20 | :red_circle: +12.1% |
| Q10 | 160.40 | 170.00 | :red_circle: +6.0% |
| Q11 | 74.60 | 68.80 | :green_circle: -7.8% |
| Q12 | 110.00 | 111.70 | :white_circle: +1.5% |
| Q13 | 131.10 | 132.70 | :white_circle: +1.2% |
| Q14 | 62.90 | 64.40 | :white_circle: +2.4% |
| Q15 | 58.10 | 60.40 | :white_circle: +4.0% |
| Q16 | 53.90 | 53.20 | :white_circle: -1.3% |
| Q17 | 215.10 | 244.50 | :red_circle: +13.7% |
| Q18 | 450.20 | 664.90 | :red_circle: +47.7% |
| Q19 | 145.50 | 148.80 | :white_circle: +2.3% |
| Q20 | 96.70 | 102.20 | :red_circle: +5.7% |
| Q21 | 435.20 | 450.70 | :white_circle: +3.6% |
| Q22 | 45.20 | 44.90 | :white_circle: -0.7% |
**Total:** Main=4063.10ms, PR=4500.90ms (+10.8%)
---
*Automated benchmark run by dfbench*
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "sqlbenchmark (via GitHub)" <gi...@apache.org>.
sqlbenchmark commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4053700998
## Ballista TPC-H Benchmark Results
**PR:** #1503 - fix review comments
**PR Commit:** `0c2ed124`
**Base Commit:** `b204bffe` (main)
**Scale Factor:** SF10
**Iterations:** 5
### Query Comparison
| Query | main (ms) | PR (ms) | Change |
|-------|-----------|---------|--------|
| Q1 | 2013.30 | 2032.50 | :white_circle: +1.0% |
| Q2 | 832.50 | 816.80 | :white_circle: -1.9% |
| Q3 | 1299.90 | 1310.00 | :white_circle: +0.8% |
| Q4 | 612.20 | 614.30 | :white_circle: +0.3% |
| Q5 | 2895.60 | 2816.30 | :white_circle: -2.7% |
| Q6 | 540.60 | 540.30 | :white_circle: -0.1% |
| Q7 | 4343.00 | 4186.40 | :white_circle: -3.6% |
| Q8 | 3355.00 | 3283.60 | :white_circle: -2.1% |
| Q9 | 5046.80 | 5013.80 | :white_circle: -0.7% |
| Q10 | 1594.80 | 1597.60 | :white_circle: +0.2% |
| Q11 | 573.50 | 578.30 | :white_circle: +0.8% |
| Q12 | 1022.40 | 1000.90 | :white_circle: -2.1% |
| Q13 | 1515.40 | 1440.30 | :white_circle: -5.0% |
| Q14 | 595.10 | 596.00 | :white_circle: +0.2% |
| Q15 | 578.60 | 586.40 | :white_circle: +1.3% |
| Q16 | 363.00 | 353.80 | :white_circle: -2.5% |
| Q17 | 3833.40 | 3738.80 | :white_circle: -2.5% |
| Q18 | 6438.10 | 5571.30 | :green_circle: -13.5% |
| Q19 | 1299.60 | 1319.90 | :white_circle: +1.6% |
| Q20 | 1018.40 | 1022.50 | :white_circle: +0.4% |
| Q21 | 4775.70 | 4949.50 | :white_circle: +3.6% |
| Q22 | 303.40 | 316.70 | :white_circle: +4.4% |
**Total:** Main=44850.30ms, PR=43686.00ms (-2.6%)
---
*Automated benchmark run by dfbench*
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "sqlbenchmark (via GitHub)" <gi...@apache.org>.
sqlbenchmark commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4054384811
## Ballista TPC-H Benchmark Results
**PR:** #1503 - skip index check if sort shuffle is disabled
**PR Commit:** `9f2649d6`
**Base Commit:** `b204bffe` (main)
**Scale Factor:** SF10
**Iterations:** 5
### Query Comparison
| Query | main (ms) | PR (ms) | Change |
|-------|-----------|---------|--------|
| Q1 | 2022.80 | 2043.00 | :white_circle: +1.0% |
| Q2 | 820.50 | 826.80 | :white_circle: +0.8% |
| Q3 | 1288.80 | 1304.00 | :white_circle: +1.2% |
| Q4 | 605.00 | 606.00 | :white_circle: +0.2% |
| Q5 | 2831.80 | 2816.80 | :white_circle: -0.5% |
| Q6 | 539.30 | 532.50 | :white_circle: -1.3% |
| Q7 | 4251.30 | 4165.00 | :white_circle: -2.0% |
| Q8 | 3341.40 | 3293.30 | :white_circle: -1.4% |
| Q9 | 5007.30 | 5040.20 | :white_circle: +0.7% |
| Q10 | 1614.30 | 1542.40 | :white_circle: -4.5% |
| Q11 | 601.00 | 572.70 | :white_circle: -4.7% |
| Q12 | 1016.10 | 999.80 | :white_circle: -1.6% |
| Q13 | 1548.60 | 1418.50 | :green_circle: -8.4% |
| Q14 | 594.60 | 591.70 | :white_circle: -0.5% |
| Q15 | 573.40 | 583.60 | :white_circle: +1.8% |
| Q16 | 355.90 | 357.70 | :white_circle: +0.5% |
| Q17 | 3725.70 | 3722.90 | :white_circle: -0.1% |
| Q18 | 6848.70 | 5747.70 | :green_circle: -16.1% |
| Q19 | 1309.40 | 1309.70 | :white_circle: +0.0% |
| Q20 | 1043.30 | 1031.90 | :white_circle: -1.1% |
| Q21 | 4766.80 | 4804.90 | :white_circle: +0.8% |
| Q22 | 319.90 | 312.40 | :white_circle: -2.3% |
**Total:** Main=45025.90ms, PR=43623.50ms (-3.1%)
---
*Automated benchmark run by dfbench*
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4053831802
can't avoid clone for host, rust complains: "cannot borrow `*self` as mutable because it is also borrowed as immutable"
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4053538787
thanks @martin-g have a look when you get chance
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4049666752
@sqlbenchmark tpch --iterations 3 --scale-factor 10
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4053975509
found another few things to be fixed
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4053608139
@sqlbenchmark tpch --iterations 5 --scale-factor 10
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "sqlbenchmark (via GitHub)" <gi...@apache.org>.
sqlbenchmark commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4053595867
## Ballista TPC-H Benchmark Results
**PR:** #1503 - fix review comments
**PR Commit:** `0c2ed124`
**Base Commit:** `b204bffe` (main)
**Scale Factor:** SF1
**Iterations:** 5
### Query Comparison
| Query | main (ms) | PR (ms) | Change |
|-------|-----------|---------|--------|
| Q1 | 212.20 | 225.60 | :red_circle: +6.3% |
| Q2 | 98.20 | 124.10 | :red_circle: +26.4% |
| Q3 | 139.30 | 139.20 | :white_circle: -0.1% |
| Q4 | 75.90 | 82.50 | :red_circle: +8.7% |
| Q5 | 279.30 | 297.90 | :red_circle: +6.7% |
| Q6 | 59.50 | 60.30 | :white_circle: +1.3% |
| Q7 | 386.50 | 407.80 | :red_circle: +5.5% |
| Q8 | 327.10 | 343.00 | :white_circle: +4.9% |
| Q9 | 448.70 | 496.30 | :red_circle: +10.6% |
| Q10 | 160.20 | 174.00 | :red_circle: +8.6% |
| Q11 | 67.20 | 70.40 | :white_circle: +4.8% |
| Q12 | 110.90 | 112.30 | :white_circle: +1.3% |
| Q13 | 129.80 | 138.30 | :red_circle: +6.5% |
| Q14 | 72.10 | 73.40 | :white_circle: +1.8% |
| Q15 | 58.60 | 59.50 | :white_circle: +1.5% |
| Q16 | 53.40 | 55.60 | :white_circle: +4.1% |
| Q17 | 211.10 | 236.50 | :red_circle: +12.0% |
| Q18 | 465.10 | 616.10 | :red_circle: +32.5% |
| Q19 | 148.50 | 147.70 | :white_circle: -0.5% |
| Q20 | 99.40 | 101.80 | :white_circle: +2.4% |
| Q21 | 451.90 | 476.70 | :red_circle: +5.5% |
| Q22 | 44.50 | 45.20 | :white_circle: +1.6% |
**Total:** Main=4099.40ms, PR=4484.20ms (+9.4%)
---
*Automated benchmark run by dfbench*
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "sqlbenchmark (via GitHub)" <gi...@apache.org>.
sqlbenchmark commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4049747029
## Ballista TPC-H Benchmark Results
**PR:** #1503 - shuffle (remote) reader cleanup
**PR Commit:** `1307392a`
**Base Commit:** `7d51c593` (main)
**Scale Factor:** SF10
**Iterations:** 3
### Query Comparison
| Query | main (ms) | PR (ms) | Change |
|-------|-----------|---------|--------|
| Q1 | 2046.60 | 2021.40 | :white_circle: -1.2% |
| Q2 | 854.30 | 779.00 | :green_circle: -8.8% |
| Q3 | 1271.60 | 1299.10 | :white_circle: +2.2% |
| Q4 | 626.90 | 607.80 | :white_circle: -3.0% |
| Q5 | 2793.40 | 2784.80 | :white_circle: -0.3% |
| Q6 | 540.60 | 543.40 | :white_circle: +0.5% |
| Q7 | 4595.20 | 4121.50 | :green_circle: -10.3% |
| Q8 | 3329.90 | 3259.10 | :white_circle: -2.1% |
| Q9 | 4968.00 | 5001.40 | :white_circle: +0.7% |
| Q10 | 1588.00 | 1566.50 | :white_circle: -1.4% |
| Q11 | 576.30 | 573.20 | :white_circle: -0.5% |
| Q12 | 1051.60 | 1003.00 | :white_circle: -4.6% |
| Q13 | 1531.20 | 1418.80 | :green_circle: -7.3% |
| Q14 | 593.60 | 588.00 | :white_circle: -0.9% |
| Q15 | 576.30 | 571.70 | :white_circle: -0.8% |
| Q16 | 354.60 | 349.30 | :white_circle: -1.5% |
| Q17 | 3733.20 | 3726.00 | :white_circle: -0.2% |
| Q18 | 7048.60 | 5865.00 | :green_circle: -16.8% |
| Q19 | 1322.90 | 1302.50 | :white_circle: -1.5% |
| Q20 | 1032.80 | 1032.30 | :white_circle: -0.0% |
| Q21 | 4446.10 | 4929.60 | :red_circle: +10.9% |
| Q22 | 343.80 | 302.00 | :green_circle: -12.2% |
**Total:** Main=45225.50ms, PR=43645.40ms (-3.5%)
---
*Automated benchmark run by dfbench*
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "sqlbenchmark (via GitHub)" <gi...@apache.org>.
sqlbenchmark commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4049652518
## Ballista TPC-H Benchmark Results
**PR:** #1503 - shuffle (remote) reader cleanup
**PR Commit:** `1307392a`
**Base Commit:** `7d51c593` (main)
**Scale Factor:** SF1
**Iterations:** 3
### Query Comparison
| Query | main (ms) | PR (ms) | Change |
|-------|-----------|---------|--------|
| Q1 | 227.80 | 235.50 | :white_circle: +3.4% |
| Q2 | 104.90 | 105.10 | :white_circle: +0.2% |
| Q3 | 141.70 | 140.80 | :white_circle: -0.6% |
| Q4 | 78.90 | 76.90 | :white_circle: -2.5% |
| Q5 | 278.20 | 288.90 | :white_circle: +3.8% |
| Q6 | 58.70 | 60.10 | :white_circle: +2.4% |
| Q7 | 391.60 | 401.10 | :white_circle: +2.4% |
| Q8 | 320.00 | 319.20 | :white_circle: -0.3% |
| Q9 | 490.80 | 482.00 | :white_circle: -1.8% |
| Q10 | 169.90 | 169.50 | :white_circle: -0.2% |
| Q11 | 68.70 | 82.60 | :red_circle: +20.2% |
| Q12 | 114.50 | 110.30 | :white_circle: -3.7% |
| Q13 | 134.00 | 130.20 | :white_circle: -2.8% |
| Q14 | 63.40 | 63.70 | :white_circle: +0.5% |
| Q15 | 59.00 | 57.60 | :white_circle: -2.4% |
| Q16 | 54.40 | 52.20 | :white_circle: -4.0% |
| Q17 | 211.80 | 225.30 | :red_circle: +6.4% |
| Q18 | 602.70 | 662.00 | :red_circle: +9.8% |
| Q19 | 152.40 | 146.30 | :white_circle: -4.0% |
| Q20 | 97.40 | 97.80 | :white_circle: +0.4% |
| Q21 | 423.20 | 437.00 | :white_circle: +3.3% |
| Q22 | 44.90 | 44.70 | :white_circle: -0.4% |
**Total:** Main=4288.90ms, PR=4388.80ms (+2.3%)
---
*Automated benchmark run by dfbench*
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4053535098
@sqlbenchmark tpch --iterations 5 --scale-factor 1
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#discussion_r2929538122
##########
ballista/core/src/execution_plans/distributed_query.rs:
##########
@@ -701,8 +701,6 @@ async fn fetch_partition(
let partition_id = location.partition_id.ok_or_else(|| {
DataFusionError::Internal("Received empty partition id".to_owned())
})?;
- let host = metadata.host.as_str();
- let port = metadata.port as u16;
Review Comment:
Good catch
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4049386126
@sqlbenchmark tpch --iterations 3 --scale-factor 10
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#discussion_r2929764469
##########
ballista/core/src/execution_plans/distributed_query.rs:
##########
@@ -701,8 +701,6 @@ async fn fetch_partition(
let partition_id = location.partition_id.ok_or_else(|| {
DataFusionError::Internal("Received empty partition id".to_owned())
})?;
- let host = metadata.host.as_str();
- let port = metadata.port as u16;
Review Comment:
should be fixed now
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4049593234
@sqlbenchmark tpch --iterations 3 --scale-factor 1
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "sqlbenchmark (via GitHub)" <gi...@apache.org>.
sqlbenchmark commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4054248094
## Ballista TPC-H Benchmark Results
**PR:** #1503 - skip index check if sort shuffle is disabled
**PR Commit:** `9f2649d6`
**Base Commit:** `b204bffe` (main)
**Scale Factor:** SF1
**Iterations:** 5
### Query Comparison
| Query | main (ms) | PR (ms) | Change |
|-------|-----------|---------|--------|
| Q1 | 212.90 | 252.70 | :red_circle: +18.7% |
| Q2 | 96.20 | 114.50 | :red_circle: +19.0% |
| Q3 | 139.70 | 140.00 | :white_circle: +0.2% |
| Q4 | 85.10 | 82.80 | :white_circle: -2.7% |
| Q5 | 279.50 | 302.70 | :red_circle: +8.3% |
| Q6 | 63.30 | 62.30 | :white_circle: -1.6% |
| Q7 | 388.40 | 397.90 | :white_circle: +2.4% |
| Q8 | 332.40 | 329.50 | :white_circle: -0.9% |
| Q9 | 454.10 | 504.10 | :red_circle: +11.0% |
| Q10 | 160.10 | 169.20 | :red_circle: +5.7% |
| Q11 | 67.50 | 69.30 | :white_circle: +2.7% |
| Q12 | 111.70 | 112.40 | :white_circle: +0.6% |
| Q13 | 134.70 | 136.20 | :white_circle: +1.1% |
| Q14 | 65.90 | 68.00 | :white_circle: +3.2% |
| Q15 | 60.00 | 61.30 | :white_circle: +2.2% |
| Q16 | 53.50 | 54.10 | :white_circle: +1.1% |
| Q17 | 215.20 | 228.10 | :red_circle: +6.0% |
| Q18 | 551.40 | 610.80 | :red_circle: +10.8% |
| Q19 | 148.20 | 150.40 | :white_circle: +1.5% |
| Q20 | 96.30 | 98.60 | :white_circle: +2.4% |
| Q21 | 443.10 | 436.40 | :white_circle: -1.5% |
| Q22 | 49.30 | 45.10 | :green_circle: -8.5% |
**Total:** Main=4208.50ms, PR=4426.40ms (+5.2%)
---
*Automated benchmark run by dfbench*
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "sqlbenchmark (via GitHub)" <gi...@apache.org>.
sqlbenchmark commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4049485198
## Ballista TPC-H Benchmark Results
**PR:** #1503 - shuffle (remote) reader cleanup
**PR Commit:** `79d07981`
**Base Commit:** `7d51c593` (main)
**Scale Factor:** SF10
**Iterations:** 3
### Query Comparison
| Query | main (ms) | PR (ms) | Change |
|-------|-----------|---------|--------|
| Q1 | 2073.50 | 2003.30 | :white_circle: -3.4% |
| Q2 | 873.90 | 799.50 | :green_circle: -8.5% |
| Q3 | 1311.10 | 1327.10 | :white_circle: +1.2% |
| Q4 | 621.30 | 616.30 | :white_circle: -0.8% |
| Q5 | 2900.40 | 2820.00 | :white_circle: -2.8% |
| Q6 | 539.50 | 554.20 | :white_circle: +2.7% |
| Q7 | 4672.70 | 4126.60 | :green_circle: -11.7% |
| Q8 | 3346.70 | 3244.50 | :white_circle: -3.1% |
| Q9 | 5056.60 | 4953.30 | :white_circle: -2.0% |
| Q10 | 1584.60 | 1649.90 | :white_circle: +4.1% |
| Q11 | 582.10 | 584.40 | :white_circle: +0.4% |
| Q12 | 1032.10 | 1012.10 | :white_circle: -1.9% |
| Q13 | 1453.40 | 1417.90 | :white_circle: -2.4% |
| Q14 | 596.80 | 592.80 | :white_circle: -0.7% |
| Q15 | 599.70 | 599.10 | :white_circle: -0.1% |
| Q16 | 359.40 | 358.30 | :white_circle: -0.3% |
| Q17 | 3765.00 | 3674.50 | :white_circle: -2.4% |
| Q18 | 6926.30 | 5831.70 | :green_circle: -15.8% |
| Q19 | 1308.90 | 1295.40 | :white_circle: -1.0% |
| Q20 | 1058.30 | 1022.00 | :white_circle: -3.4% |
| Q21 | 4587.50 | 4671.30 | :white_circle: +1.8% |
| Q22 | 295.10 | 313.00 | :red_circle: +6.1% |
**Total:** Main=45544.90ms, PR=43467.20ms (-4.6%)
---
*Automated benchmark run by dfbench*
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#discussion_r2929400505
##########
ballista/core/src/execution_plans/shuffle_reader.rs:
##########
@@ -427,46 +414,44 @@ fn send_fetch_partitions(
// keep local shuffle files reading in serial order for memory control.
let response_sender_c = response_sender.clone();
- let customize_endpoint_c = customize_endpoint.clone();
- spawned_tasks.push(SpawnedTask::spawn(async move {
- for p in local_locations {
- let r = PartitionReaderEnum::Local
- .fetch_partition(
- &p,
- max_message_size,
- flight_transport,
- customize_endpoint_c.clone(),
- use_tls,
- )
- .await;
- if let Err(e) = response_sender_c.send(r).await {
- error!("Fail to send response event to the channel due to {e}");
+
+ //
+ // fetching local partitions (read from file)
+ //
+
+ spawned_tasks.push(SpawnedTask::spawn_blocking({
+ move || {
+ for p in local_locations {
+ let r = fetch_partition_local(&p);
+ if let Err(e) = response_sender_c.blocking_send(r) {
+ error!("Fail to send response event to the channel due to {e}");
+ }
}
}
}));
+ //
+ // fetching remote partitions (uses grpc flight protocol)
+ //
for p in remote_locations.into_iter() {
let semaphore = semaphore.clone();
let response_sender = response_sender.clone();
- let customize_endpoint_c = customize_endpoint.clone();
- spawned_tasks.push(SpawnedTask::spawn(async move {
- // Block if exceeds max request number.
- let permit = semaphore.acquire_owned().await.unwrap();
- let r = PartitionReaderEnum::FlightRemote
- .fetch_partition(
- &p,
- max_message_size,
- flight_transport,
- customize_endpoint_c,
- use_tls,
- )
- .await;
- // Block if the channel buffer is full.
- if let Err(e) = response_sender.send(r).await {
- error!("Fail to send response event to the channel due to {e}");
+
+ spawned_tasks.push(SpawnedTask::spawn({
+ // TODO: make BallistaConfig cheaper to clone
Review Comment:
Option 1) wrap it in Arc
Option 2) introduce `RemoteFetchConfig` that contains only the settings needed here
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#discussion_r2929350569
##########
ballista/core/src/config.rs:
##########
@@ -419,6 +427,23 @@ impl BallistaConfig {
v.parse::<f64>().unwrap()
}
}
+ /// sets the configuration value where key starts with ballista
Review Comment:
```suggestion
/// Sets the configuration value where key starts with ballista
```
##########
ballista/core/src/execution_plans/distributed_query.rs:
##########
@@ -701,8 +701,6 @@ async fn fetch_partition(
let partition_id = location.partition_id.ok_or_else(|| {
DataFusionError::Internal("Received empty partition id".to_owned())
})?;
- let host = metadata.host.as_str();
- let port = metadata.port as u16;
Review Comment:
Now if there is a proxy the host/port is the proxy. Before it was using the executor's host/port.
Just checking that this is intentional.
shuffle_reader::fetch_partition_remote() still uses the metadata host/port - https://github.com/apache/datafusion-ballista/pull/1503/changes#diff-821d458bfa4b39a875e8889c8e93594de9ade9fe75ff4336198f51751527f73fR486
##########
ballista/core/src/config.rs:
##########
@@ -419,6 +427,23 @@ impl BallistaConfig {
v.parse::<f64>().unwrap()
}
}
+ /// sets the configuration value where key starts with ballista
+ /// prefix.
+ pub fn set_with_prefix(
+ &mut self,
+ key: &str,
+ value: &str,
+ ) -> datafusion::error::Result<()> {
+ let entries = Self::valid_entries();
+ //let k = format!("{}.{key}", BallistaConfig::PREFIX);
+
+ if entries.contains_key(key) {
+ self.settings.insert(key.to_string(), value.to_string());
Review Comment:
`with_settings()` does validation of the value - https://github.com/apache/datafusion-ballista/pull/1503/changes#diff-62a06bd33b6bb8dba344edf9c08f6c41fcd3ff790d82c2157519cf46012a708aR221-R224
Is the same needed here ?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "sqlbenchmark (via GitHub)" <gi...@apache.org>.
sqlbenchmark commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4053972428
## Ballista TPC-H Benchmark Results
**PR:** #1503 - minor
**PR Commit:** `a967501d`
**Base Commit:** `b204bffe` (main)
**Scale Factor:** SF10
**Iterations:** 5
### Query Comparison
| Query | main (ms) | PR (ms) | Change |
|-------|-----------|---------|--------|
| Q1 | 2005.80 | 2031.80 | :white_circle: +1.3% |
| Q2 | 799.10 | 806.30 | :white_circle: +0.9% |
| Q3 | 1276.80 | 1289.80 | :white_circle: +1.0% |
| Q4 | 599.50 | 602.60 | :white_circle: +0.5% |
| Q5 | 2784.30 | 2828.70 | :white_circle: +1.6% |
| Q6 | 543.90 | 542.70 | :white_circle: -0.2% |
| Q7 | 4392.40 | 4162.40 | :green_circle: -5.2% |
| Q8 | 3353.60 | 3262.60 | :white_circle: -2.7% |
| Q9 | 5080.70 | 4907.00 | :white_circle: -3.4% |
| Q10 | 1604.40 | 1529.70 | :white_circle: -4.7% |
| Q11 | 583.50 | 573.40 | :white_circle: -1.7% |
| Q12 | 1022.20 | 1011.90 | :white_circle: -1.0% |
| Q13 | 1505.50 | 1386.10 | :green_circle: -7.9% |
| Q14 | 592.80 | 595.20 | :white_circle: +0.4% |
| Q15 | 587.50 | 582.40 | :white_circle: -0.9% |
| Q16 | 353.40 | 358.50 | :white_circle: +1.4% |
| Q17 | 3764.60 | 3773.30 | :white_circle: +0.2% |
| Q18 | 6295.60 | 5798.90 | :green_circle: -7.9% |
| Q19 | 1306.80 | 1319.00 | :white_circle: +0.9% |
| Q20 | 1032.50 | 1019.10 | :white_circle: -1.3% |
| Q21 | 5154.20 | 4937.70 | :white_circle: -4.2% |
| Q22 | 303.60 | 316.80 | :white_circle: +4.3% |
**Total:** Main=44942.70ms, PR=43635.90ms (-2.9%)
---
*Automated benchmark run by dfbench*
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4054275651
@sqlbenchmark tpch --iterations 5 --scale-factor 10
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4049502633
@sqlbenchmark tpch --iterations 3 --scale-factor 1
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#discussion_r2929632106
##########
ballista/core/src/execution_plans/shuffle_reader.rs:
##########
@@ -427,46 +414,44 @@ fn send_fetch_partitions(
// keep local shuffle files reading in serial order for memory control.
let response_sender_c = response_sender.clone();
- let customize_endpoint_c = customize_endpoint.clone();
- spawned_tasks.push(SpawnedTask::spawn(async move {
- for p in local_locations {
- let r = PartitionReaderEnum::Local
- .fetch_partition(
- &p,
- max_message_size,
- flight_transport,
- customize_endpoint_c.clone(),
- use_tls,
- )
- .await;
- if let Err(e) = response_sender_c.send(r).await {
- error!("Fail to send response event to the channel due to {e}");
+
+ //
+ // fetching local partitions (read from file)
+ //
+
+ spawned_tasks.push(SpawnedTask::spawn_blocking({
+ move || {
+ for p in local_locations {
+ let r = fetch_partition_local(&p);
+ if let Err(e) = response_sender_c.blocking_send(r) {
+ error!("Fail to send response event to the channel due to {e}");
+ }
}
}
}));
+ //
+ // fetching remote partitions (uses grpc flight protocol)
+ //
for p in remote_locations.into_iter() {
let semaphore = semaphore.clone();
let response_sender = response_sender.clone();
- let customize_endpoint_c = customize_endpoint.clone();
- spawned_tasks.push(SpawnedTask::spawn(async move {
- // Block if exceeds max request number.
- let permit = semaphore.acquire_owned().await.unwrap();
- let r = PartitionReaderEnum::FlightRemote
- .fetch_partition(
- &p,
- max_message_size,
- flight_transport,
- customize_endpoint_c,
- use_tls,
- )
- .await;
- // Block if the channel buffer is full.
- if let Err(e) = response_sender.send(r).await {
- error!("Fail to send response event to the channel due to {e}");
+
+ spawned_tasks.push(SpawnedTask::spawn({
+ // TODO: make BallistaConfig cheaper to clone
Review Comment:
Actually, there is `GrpcClientConfig` which should have been used here
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4053869985
@sqlbenchmark tpch --iterations 5 --scale-factor 10
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503#issuecomment-4054175946
@sqlbenchmark tpch --iterations 5 --scale-factor 1
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: (remote) shuffle reader cleanup [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm merged PR #1503:
URL: https://github.com/apache/datafusion-ballista/pull/1503
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org