refactor: replace `unwrap_or` with `unwrap_or_else` for improved lazy… #15841

NevroHelios · 2025-04-24T10:43:14Z

… evaluation

Which issue does this PR close?

Eliminate the function call in xxx_or (e.g. unwrap_or("".to_string()) #15802

Closes Eliminate the function call in xxx_or (e.g. unwrap_or("".to_string()) #15802

Rationale for this change

As per the issue explained I updated the function call unwrap_or with unwrap_or_else

Are these changes tested?

I build and tested the code locally with cargo build --workspace -j1 and cargo test --workspace -j1

@xudong963

… evaluation

xudong963

Thank you @NevroHelios , I triggered the CI, will review after ci passes

NevroHelios · 2025-04-24T11:20:36Z

Thanks for your update @xudong963. I noticed a few checks have failed. I will look into the issues and push the updates shortly.
If there are any specific clippy or formatting issues I might have overlooked, feel free to point them out - any pointer will be helpful and thanks again for the CI trigger!

…`unwrap_or_else` for better lazy evaluation

NevroHelios · 2025-04-24T14:26:45Z

Hi @xudong963
I made the changes and pushed the updates. Could you please initiate the CI workflows when you get a chance?

alamb · 2025-04-29T01:36:04Z

This PR seems to have some CI failures

Please mark it as ready for review when it is ready for another look

alamb · 2025-04-29T01:36:19Z

Thank you for this PR @NevroHelios

NevroHelios · 2025-04-30T22:00:32Z

Hi @alamb, apologies for the delay, I had exams recently. I've run the tests locally and everything looks good now. When you get a chance, could you please re-trigger the CI? Thanks!

alamb

Thanks @NevroHelios -- this looks like a good set of changes. Thank you -- let's see how the CI goes.

I had one question about one of the changes, but otherwise this looks good to me

benchmarks/src/util/options.rs

datafusion/common/src/column.rs

NevroHelios · 2025-05-01T14:24:56Z

I pushed the updates. Could you please run the ci again? @alamb

xudong963 · 2025-05-02T00:04:52Z

benchmarks/src/util/options.rs

-        if let Some(sort_spill_reservation_bytes) = self.sort_spill_reservation_bytes {
-            config =
-                config.with_sort_spill_reservation_bytes(sort_spill_reservation_bytes);


Why was the part removed?

I agree it looks like a mistake

During the copiler errors I updated this config but later I restored most parts but I believe I mistakenly overlooked these lines during the process. Thanks for points that out. I will push the updates shortly.

NevroHelios · 2025-05-03T03:40:40Z

Hi. I pushed the updates. Could you please re run the CI? @alamb

NevroHelios · 2025-05-04T19:25:38Z

Is there anything else I am missing or need to add? @alamb @xudong963

alamb

Thank you @NevroHelios -- there are just a few more things (remove the use of cloned) and this will be good to go

datafusion/physical-optimizer/src/enforce_sorting/mod.rs

datafusion/physical-optimizer/src/enforce_sorting/replace_with_order_preserving_variants.rs

datafusion/physical-optimizer/src/update_aggr_exprs.rs

alamb · 2025-05-05T14:05:55Z

🤖 ./gh_compare_branch_bench.sh Benchmark Script Running
Linux aal-dev 6.11.0-1013-gcp #13~24.04.1-Ubuntu SMP Wed Apr 2 16:34:16 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing NevroHelios/main (81218cb) to c1a4957 diff
BENCH_NAME=sql_planner
BENCH_COMMAND=cargo bench --bench sql_planner
BENCH_FILTER=
BENCH_BRANCH_NAME=NevroHelios_main
Results will be posted here when complete

alamb · 2025-05-05T15:03:43Z

🤖: Benchmark completed

Details

group                                         NevroHelios_main                       main
-----                                         ----------------                       ----
logical_aggregate_with_join                   1.00    734.1±6.40µs        ? ?/sec    1.00    737.8±7.70µs        ? ?/sec
logical_select_all_from_1000                  1.00    127.8±0.20ms        ? ?/sec    1.05    134.6±0.32ms        ? ?/sec
logical_select_one_from_700                   1.00    416.1±5.04µs        ? ?/sec    1.00    417.8±1.75µs        ? ?/sec
logical_trivial_join_high_numbered_columns    1.00    379.2±4.67µs        ? ?/sec    1.00    379.7±2.01µs        ? ?/sec
logical_trivial_join_low_numbered_columns     1.00    363.7±2.11µs        ? ?/sec    1.00    365.5±2.08µs        ? ?/sec
physical_intersection                         1.00    846.5±6.48µs        ? ?/sec    1.01    850.8±5.16µs        ? ?/sec
physical_join_consider_sort                   1.00   1370.0±5.39µs        ? ?/sec    1.00   1366.0±6.36µs        ? ?/sec
physical_join_distinct                        1.00    354.0±5.56µs        ? ?/sec    1.00    355.6±1.43µs        ? ?/sec
physical_many_self_joins                      1.00     10.3±0.05ms        ? ?/sec    1.00     10.3±0.07ms        ? ?/sec
physical_plan_clickbench_all                  1.00    139.1±1.27ms        ? ?/sec    1.00    139.6±1.36ms        ? ?/sec
physical_plan_clickbench_q1                   1.00  1681.8±18.64µs        ? ?/sec    1.00  1678.6±16.48µs        ? ?/sec
physical_plan_clickbench_q10                  1.00      2.4±0.03ms        ? ?/sec    1.01      2.5±0.03ms        ? ?/sec
physical_plan_clickbench_q11                  1.00      2.5±0.03ms        ? ?/sec    1.00      2.5±0.04ms        ? ?/sec
physical_plan_clickbench_q12                  1.01      2.7±0.02ms        ? ?/sec    1.00      2.7±0.04ms        ? ?/sec
physical_plan_clickbench_q13                  1.00      2.3±0.04ms        ? ?/sec    1.00      2.3±0.03ms        ? ?/sec
physical_plan_clickbench_q14                  1.00      2.5±0.03ms        ? ?/sec    1.00      2.5±0.03ms        ? ?/sec
physical_plan_clickbench_q15                  1.02      2.4±0.02ms        ? ?/sec    1.00      2.4±0.02ms        ? ?/sec
physical_plan_clickbench_q16                  1.01      2.3±0.03ms        ? ?/sec    1.00      2.3±0.03ms        ? ?/sec
physical_plan_clickbench_q17                  1.00      2.4±0.03ms        ? ?/sec    1.00      2.4±0.03ms        ? ?/sec
physical_plan_clickbench_q18                  1.00  1979.4±24.43µs        ? ?/sec    1.00  1970.2±21.05µs        ? ?/sec
physical_plan_clickbench_q19                  1.01      2.9±0.04ms        ? ?/sec    1.00      2.9±0.03ms        ? ?/sec
physical_plan_clickbench_q2                   1.00  1911.8±17.18µs        ? ?/sec    1.00  1907.1±21.02µs        ? ?/sec
physical_plan_clickbench_q20                  1.00  1645.8±18.95µs        ? ?/sec    1.00  1638.5±19.79µs        ? ?/sec
physical_plan_clickbench_q21                  1.00  1934.8±27.08µs        ? ?/sec    1.00  1932.6±19.30µs        ? ?/sec
physical_plan_clickbench_q22                  1.01      2.6±0.02ms        ? ?/sec    1.00      2.6±0.02ms        ? ?/sec
physical_plan_clickbench_q23                  1.00      2.9±0.02ms        ? ?/sec    1.00      2.9±0.04ms        ? ?/sec
physical_plan_clickbench_q24                  1.00      4.6±0.04ms        ? ?/sec    1.02      4.7±0.04ms        ? ?/sec
physical_plan_clickbench_q25                  1.01      2.0±0.02ms        ? ?/sec    1.00  1974.9±22.20µs        ? ?/sec
physical_plan_clickbench_q26                  1.01  1814.8±19.29µs        ? ?/sec    1.00  1798.2±15.81µs        ? ?/sec
physical_plan_clickbench_q27                  1.01      2.0±0.02ms        ? ?/sec    1.00  1994.2±21.71µs        ? ?/sec
physical_plan_clickbench_q28                  1.00      2.8±0.03ms        ? ?/sec    1.00      2.8±0.04ms        ? ?/sec
physical_plan_clickbench_q29                  1.00      3.5±0.04ms        ? ?/sec    1.00      3.5±0.04ms        ? ?/sec
physical_plan_clickbench_q3                   1.00  1882.0±21.14µs        ? ?/sec    1.00  1880.2±20.28µs        ? ?/sec
physical_plan_clickbench_q30                  1.00     14.4±0.18ms        ? ?/sec    1.00     14.5±0.11ms        ? ?/sec
physical_plan_clickbench_q31                  1.00      2.8±0.03ms        ? ?/sec    1.00      2.8±0.04ms        ? ?/sec
physical_plan_clickbench_q32                  1.00      2.8±0.03ms        ? ?/sec    1.00      2.8±0.04ms        ? ?/sec
physical_plan_clickbench_q33                  1.00      2.4±0.03ms        ? ?/sec    1.00      2.4±0.03ms        ? ?/sec
physical_plan_clickbench_q34                  1.00      2.1±0.02ms        ? ?/sec    1.00      2.1±0.02ms        ? ?/sec
physical_plan_clickbench_q35                  1.00      2.2±0.03ms        ? ?/sec    1.00      2.2±0.03ms        ? ?/sec
physical_plan_clickbench_q36                  1.01      3.0±0.02ms        ? ?/sec    1.00      3.0±0.03ms        ? ?/sec
physical_plan_clickbench_q37                  1.01      3.1±0.04ms        ? ?/sec    1.00      3.0±0.03ms        ? ?/sec
physical_plan_clickbench_q38                  1.00      3.1±0.04ms        ? ?/sec    1.00      3.0±0.04ms        ? ?/sec
physical_plan_clickbench_q39                  1.01      2.8±0.03ms        ? ?/sec    1.00      2.8±0.03ms        ? ?/sec
physical_plan_clickbench_q4                   1.00  1641.5±18.11µs        ? ?/sec    1.00  1647.4±20.55µs        ? ?/sec
physical_plan_clickbench_q40                  1.00      3.4±0.04ms        ? ?/sec    1.00      3.4±0.06ms        ? ?/sec
physical_plan_clickbench_q41                  1.01      3.0±0.04ms        ? ?/sec    1.00      3.0±0.03ms        ? ?/sec
physical_plan_clickbench_q42                  1.01      3.0±0.03ms        ? ?/sec    1.00      2.9±0.03ms        ? ?/sec
physical_plan_clickbench_q43                  1.01      3.1±0.03ms        ? ?/sec    1.00      3.0±0.03ms        ? ?/sec
physical_plan_clickbench_q44                  1.00  1797.2±27.13µs        ? ?/sec    1.00  1795.8±18.42µs        ? ?/sec
physical_plan_clickbench_q45                  1.00  1796.1±18.12µs        ? ?/sec    1.01  1813.4±22.23µs        ? ?/sec
physical_plan_clickbench_q46                  1.01      2.2±0.03ms        ? ?/sec    1.00      2.2±0.02ms        ? ?/sec
physical_plan_clickbench_q47                  1.01      2.9±0.07ms        ? ?/sec    1.00      2.8±0.03ms        ? ?/sec
physical_plan_clickbench_q48                  1.01      3.4±0.04ms        ? ?/sec    1.00      3.3±0.04ms        ? ?/sec
physical_plan_clickbench_q49                  1.00      3.8±0.05ms        ? ?/sec    1.00      3.8±0.05ms        ? ?/sec
physical_plan_clickbench_q5                   1.00  1835.1±23.11µs        ? ?/sec    1.00  1839.2±22.83µs        ? ?/sec
physical_plan_clickbench_q50                  1.01      3.6±0.04ms        ? ?/sec    1.00      3.6±0.03ms        ? ?/sec
physical_plan_clickbench_q6                   1.01  1851.4±25.10µs        ? ?/sec    1.00  1835.5±24.31µs        ? ?/sec
physical_plan_clickbench_q7                   1.00  1687.4±21.38µs        ? ?/sec    1.01  1708.1±29.57µs        ? ?/sec
physical_plan_clickbench_q8                   1.01      2.3±0.02ms        ? ?/sec    1.00      2.3±0.02ms        ? ?/sec
physical_plan_clickbench_q9                   1.00      2.3±0.02ms        ? ?/sec    1.00      2.3±0.03ms        ? ?/sec
physical_plan_tpcds_all                       1.00   1099.1±5.69ms        ? ?/sec    1.00   1101.6±4.51ms        ? ?/sec
physical_plan_tpch_all                        1.00     65.3±0.58ms        ? ?/sec    1.00     65.2±0.42ms        ? ?/sec
physical_plan_tpch_q1                         1.00      2.1±0.01ms        ? ?/sec    1.01      2.2±0.01ms        ? ?/sec
physical_plan_tpch_q10                        1.00      3.3±0.01ms        ? ?/sec    1.01      3.3±0.01ms        ? ?/sec
physical_plan_tpch_q11                        1.00      3.4±0.05ms        ? ?/sec    1.00      3.4±0.02ms        ? ?/sec
physical_plan_tpch_q12                        1.00  1941.5±16.14µs        ? ?/sec    1.00  1941.0±10.90µs        ? ?/sec
physical_plan_tpch_q13                        1.00  1494.4±12.02µs        ? ?/sec    1.01  1502.2±11.07µs        ? ?/sec
physical_plan_tpch_q14                        1.00  1942.4±16.01µs        ? ?/sec    1.01  1953.0±10.99µs        ? ?/sec
physical_plan_tpch_q16                        1.00      2.6±0.01ms        ? ?/sec    1.00      2.6±0.01ms        ? ?/sec
physical_plan_tpch_q17                        1.00      2.6±0.02ms        ? ?/sec    1.00      2.6±0.01ms        ? ?/sec
physical_plan_tpch_q18                        1.00      2.7±0.02ms        ? ?/sec    1.00      2.7±0.01ms        ? ?/sec
physical_plan_tpch_q19                        1.01      3.9±0.03ms        ? ?/sec    1.00      3.9±0.02ms        ? ?/sec
physical_plan_tpch_q2                         1.00      5.7±0.02ms        ? ?/sec    1.00      5.7±0.03ms        ? ?/sec
physical_plan_tpch_q20                        1.00      3.4±0.02ms        ? ?/sec    1.00      3.4±0.02ms        ? ?/sec
physical_plan_tpch_q21                        1.00      4.5±0.05ms        ? ?/sec    1.00      4.5±0.02ms        ? ?/sec
physical_plan_tpch_q22                        1.00      3.0±0.01ms        ? ?/sec    1.00      3.0±0.02ms        ? ?/sec
physical_plan_tpch_q3                         1.00      2.2±0.02ms        ? ?/sec    1.00      2.2±0.01ms        ? ?/sec
physical_plan_tpch_q4                         1.00   1674.2±7.97µs        ? ?/sec    1.01  1685.4±11.14µs        ? ?/sec
physical_plan_tpch_q5                         1.00      3.2±0.04ms        ? ?/sec    1.00      3.2±0.02ms        ? ?/sec
physical_plan_tpch_q6                         1.00    932.2±7.24µs        ? ?/sec    1.01    938.0±7.43µs        ? ?/sec
physical_plan_tpch_q7                         1.00      4.4±0.03ms        ? ?/sec    1.00      4.4±0.02ms        ? ?/sec
physical_plan_tpch_q8                         1.00      5.3±0.03ms        ? ?/sec    1.00      5.3±0.02ms        ? ?/sec
physical_plan_tpch_q9                         1.00      4.2±0.03ms        ? ?/sec    1.01      4.2±0.02ms        ? ?/sec
physical_select_aggregates_from_200           1.00     26.3±0.11ms        ? ?/sec    1.01     26.6±0.13ms        ? ?/sec
physical_select_all_from_1000                 1.00    142.3±0.27ms        ? ?/sec    1.05    149.3±0.39ms        ? ?/sec
physical_select_one_from_700                  1.00  1062.6±13.33µs        ? ?/sec    1.00   1066.0±3.79µs        ? ?/sec
physical_sorted_union_orderby                 1.00     63.7±0.37ms        ? ?/sec    1.01     64.1±0.81ms        ? ?/sec
physical_theta_join_consider_sort             1.00   1745.1±7.74µs        ? ?/sec    1.00   1747.6±4.96µs        ? ?/sec
physical_unnest_to_join                       1.00   1330.7±9.57µs        ? ?/sec    1.00   1332.6±6.95µs        ? ?/sec
with_param_values_many_columns                1.00    169.6±0.90µs        ? ?/sec    1.01    171.4±1.05µs        ? ?/sec

NevroHelios · 2025-05-05T16:47:03Z

Hi @alamb
I updated and pushed the code, avaoiding the use of .clone(). Could you please rerun the CI?

alamb

Thank you @NevroHelios -- this now looks good to me. Thank you for sticking with it

alamb · 2025-05-05T19:10:24Z

datafusion/physical-optimizer/src/enforce_distribution.rs

-                    .output_ordering()
-                    .unwrap_or(&LexOrdering::default())
-                    .clone(),
+                input.plan.output_ordering().cloned().unwrap_or_default(),


the original code clones the output ordering too so this looks equivalent to me

alamb · 2025-05-05T19:11:54Z

FYI @Rachelint as you filed Eliminate the function call in xxx_or (e.g. unwrap_or("".to_string()) #15802

alamb · 2025-05-06T20:27:57Z

Thank you very much @NevroHelios

refactor: replace unwrap_or with unwrap_or_else for improved lazy…

beff993

… evaluation

xudong963 reviewed Apr 24, 2025

View reviewed changes

NevroHelios and others added 2 commits April 24, 2025 12:51

refactor: improve code readability by adjusting formatting and using …

ce0b453

…`unwrap_or_else` for better lazy evaluation

Merge branch 'main' into main

ab8f1b2

Merge branch 'main' into main

3ebc1dd

alamb marked this pull request as draft April 29, 2025 01:35

[FIX] added imports

0aa5622

NevroHelios marked this pull request as ready for review April 30, 2025 21:52

alamb reviewed Apr 30, 2025

View reviewed changes

benchmarks/src/util/options.rs Outdated Show resolved Hide resolved

datafusion/common/src/column.rs Show resolved Hide resolved

[FIX] formatting and restored original config logic

e149e75

Merge branch 'main' into main

d7bf235

NevroHelios requested review from alamb and xudong963 May 1, 2025 16:33

xudong963 reviewed May 2, 2025

View reviewed changes

NevroHelios and others added 2 commits May 2, 2025 20:14

config restored

1d3fa64

Merge branch 'main' into main

831f66f

NevroHelios requested a review from xudong963 May 2, 2025 14:47

NevroHelios added 2 commits May 2, 2025 23:57

Merge branch 'main' into main

2049537

Merge branch 'main' into main

81218cb

alamb reviewed May 5, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

NevroHelios and others added 4 commits May 5, 2025 21:29

Merge branch 'main' into main

2295fd3

optimized the use of .clone()

d4e2ef5

removed the use of clone

22181da

cleanup the clone usecase

f6c13b2

NevroHelios requested a review from alamb May 5, 2025 18:43

alamb approved these changes May 5, 2025

View reviewed changes

alamb mentioned this pull request May 5, 2025

Eliminate the function call in xxx_or (e.g. unwrap_or("".to_string()) #15802

Closed

alamb merged commit 0ebe473 into apache:main May 6, 2025
29 checks passed

refactor: replace unwrap_or with unwrap_or_else for improved lazy… #15841

refactor: replace unwrap_or with unwrap_or_else for improved lazy… #15841

Uh oh!

Conversation

NevroHelios commented Apr 24, 2025

Which issue does this PR close?

Rationale for this change

Are these changes tested?

Uh oh!

xudong963 left a comment

Choose a reason for hiding this comment

Uh oh!

NevroHelios commented Apr 24, 2025

Uh oh!

NevroHelios commented Apr 24, 2025

Uh oh!

alamb commented Apr 29, 2025

Uh oh!

alamb commented Apr 29, 2025

Uh oh!

NevroHelios commented Apr 30, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NevroHelios commented May 1, 2025

Uh oh!

xudong963 May 2, 2025

Choose a reason for hiding this comment

Uh oh!

alamb May 2, 2025

Choose a reason for hiding this comment

Uh oh!

NevroHelios May 2, 2025

Choose a reason for hiding this comment

Uh oh!

NevroHelios commented May 3, 2025

Uh oh!

NevroHelios commented May 4, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

alamb commented May 5, 2025

Uh oh!

alamb commented May 5, 2025

Uh oh!

NevroHelios commented May 5, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb May 5, 2025

Choose a reason for hiding this comment

Uh oh!

alamb commented May 5, 2025

Uh oh!

Uh oh!

alamb commented May 6, 2025

Uh oh!

Uh oh!

refactor: replace `unwrap_or` with `unwrap_or_else` for improved lazy… #15841

refactor: replace `unwrap_or` with `unwrap_or_else` for improved lazy… #15841