refactor: cleanup naming and macro usages for binary operator #17985

sunng87 · 2025-10-08T21:57:03Z

Which issue does this PR close?

N/A

Rationale for this change

The implementation of binary operators, especially for regex and bitwise, is a little bit difficult to read and understand.

the macro for bitwise operations are actually generic, while others have generic name but specific to bitwise operations.
macros for regexp operator is not necessary and can be simplified to function
some macros sits in binary.rs, while others in kernels

What changes are included in this PR?

update naming of bitwise macros
moved regexp macros into kernel.rs and refactored into functions when its possible, removed duplicated macros

Are these changes tested?

Unit tests

Are there any user-facing changes?

No.

alamb · 2025-10-09T18:15:58Z

The implementation of binary operators, especially for regex and bitwise, is a little bit difficult to read and understand.

Yes!

alamb · 2025-10-10T18:13:34Z

🤖 ./gh_compare_branch_bench.sh Benchmark Script Running
Linux aal-dev 6.14.0-1016-gcp #17~24.04.1-Ubuntu SMP Wed Sep 3 01:55:36 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing refactor/binary-op-kernels (fa39b2e) to 3b37ae0 diff
BENCH_NAME=regx
BENCH_COMMAND=cargo bench --bench regx
BENCH_FILTER=
BENCH_BRANCH_NAME=refactor_binary-op-kernels
Results will be posted here when complete

alamb · 2025-10-10T18:18:01Z

🤖: Benchmark completed

Details

group                           main                                   refactor_binary-op-kernels
-----                           ----                                   --------------------------
regexp_count_1000 string        1.00      3.9±0.02ms        ? ?/sec    1.00      3.9±0.02ms        ? ?/sec
regexp_count_1000 utf8view      1.00      4.0±0.02ms        ? ?/sec    1.00      4.0±0.02ms        ? ?/sec
regexp_instr_1000 string        1.00      4.6±0.03ms        ? ?/sec    1.00      4.6±0.03ms        ? ?/sec
regexp_instr_1000 utf8view      1.00      4.6±0.03ms        ? ?/sec    1.00      4.6±0.03ms        ? ?/sec
regexp_like_1000                1.01      4.0±0.02ms        ? ?/sec    1.00      3.9±0.02ms        ? ?/sec
regexp_like_1000 utf8view       1.00      4.0±0.02ms        ? ?/sec    1.00      4.0±0.02ms        ? ?/sec
regexp_match_1000               1.00      4.7±0.03ms        ? ?/sec    1.00      4.7±0.03ms        ? ?/sec
regexp_match_1000 utf8view      1.00      4.7±0.03ms        ? ?/sec    1.00      4.7±0.02ms        ? ?/sec
regexp_replace_1000             1.00      3.6±0.02ms        ? ?/sec    1.00      3.6±0.02ms        ? ?/sec
regexp_replace_1000 utf8view    1.00      3.6±0.03ms        ? ?/sec    1.00      3.6±0.02ms        ? ?/sec

alamb · 2025-10-10T18:18:03Z

🤖 ./gh_compare_branch_bench.sh Benchmark Script Running
Linux aal-dev 6.14.0-1016-gcp #17~24.04.1-Ubuntu SMP Wed Sep 3 01:55:36 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing refactor/binary-op-kernels (fa39b2e) to 3b37ae0 diff
BENCH_NAME=binary_op
BENCH_COMMAND=cargo bench --bench binary_op
BENCH_FILTER=
BENCH_BRANCH_NAME=refactor_binary-op-kernels
Results will be posted here when complete

alamb · 2025-10-10T18:29:14Z

🤖: Benchmark completed

Details

group                                      main                                   refactor_binary-op-kernels
-----                                      ----                                   --------------------------
short_circuit/and/all_false                1.04    147.5±0.32ns        ? ?/sec    1.00    142.0±0.35ns        ? ?/sec
short_circuit/and/all_true_in_and          1.00  1121.5±10.87µs        ? ?/sec    1.01  1130.7±11.19µs        ? ?/sec
short_circuit/and/one_true_first           1.00    438.6±2.00µs        ? ?/sec    1.00    437.4±2.47µs        ? ?/sec
short_circuit/and/one_true_last            1.01    385.9±2.68µs        ? ?/sec    1.00    383.0±1.78µs        ? ?/sec
short_circuit/and/one_true_middle          1.01    386.8±2.23µs        ? ?/sec    1.00    383.1±2.76µs        ? ?/sec
short_circuit/and/one_true_middle_left     1.00    377.5±1.27µs        ? ?/sec    1.00    377.3±3.17µs        ? ?/sec
short_circuit/and/one_true_middle_right    1.00    386.3±1.52µs        ? ?/sec    1.00    386.2±1.81µs        ? ?/sec
short_circuit/or/all_false_in_or           1.01  1129.9±12.27µs        ? ?/sec    1.00   1123.6±9.54µs        ? ?/sec
short_circuit/or/all_true                  1.01    142.6±0.33ns        ? ?/sec    1.00    141.6±1.72ns        ? ?/sec
short_circuit/or/one_false_first           1.01  1130.4±15.05µs        ? ?/sec    1.00   1122.5±9.37µs        ? ?/sec
short_circuit/or/one_false_last            1.00  1126.4±13.21µs        ? ?/sec    1.00  1126.1±12.33µs        ? ?/sec
short_circuit/or/one_false_middle          1.00   1126.6±8.50µs        ? ?/sec    1.00  1129.1±13.75µs        ? ?/sec
short_circuit/or/one_false_middle_left     1.01   1130.4±9.08µs        ? ?/sec    1.00   1118.2±7.91µs        ? ?/sec
short_circuit/or/one_false_middle_right    1.01  1130.8±14.25µs        ? ?/sec    1.00  1124.2±14.26µs        ? ?/sec

alamb · 2025-10-10T18:29:17Z

🤖 ./gh_compare_branch.sh Benchmark Script Running
Linux aal-dev 6.14.0-1016-gcp #17~24.04.1-Ubuntu SMP Wed Sep 3 01:55:36 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing refactor/binary-op-kernels (fa39b2e) to 3b37ae0 diff using: tpch_mem clickbench_partitioned clickbench_extended
Results will be posted here when complete

alamb · 2025-10-10T19:28:11Z

🤖: Benchmark completed

Details

Comparing HEAD and refactor_binary-op-kernels
--------------------
Benchmark clickbench_extended.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query        ┃        HEAD ┃ refactor_binary-op-kernels ┃        Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 0     │  2889.68 ms │                 2904.97 ms │     no change │
│ QQuery 1     │  1379.77 ms │                 1263.90 ms │ +1.09x faster │
│ QQuery 2     │  2658.11 ms │                 2620.14 ms │     no change │
│ QQuery 3     │  1192.75 ms │                 1240.86 ms │     no change │
│ QQuery 4     │  2377.16 ms │                 2368.09 ms │     no change │
│ QQuery 5     │ 28637.96 ms │                28516.20 ms │     no change │
│ QQuery 6     │  4318.97 ms │                 4339.23 ms │     no change │
│ QQuery 7     │  3988.70 ms │                 3830.78 ms │     no change │
└──────────────┴─────────────┴────────────────────────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┓
┃ Benchmark Summary                         ┃            ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━┩
│ Total Time (HEAD)                         │ 47443.09ms │
│ Total Time (refactor_binary-op-kernels)   │ 47084.17ms │
│ Average Time (HEAD)                       │  5930.39ms │
│ Average Time (refactor_binary-op-kernels) │  5885.52ms │
│ Queries Faster                            │          1 │
│ Queries Slower                            │          0 │
│ Queries with No Change                    │          7 │
│ Queries with Failure                      │          0 │
└───────────────────────────────────────────┴────────────┘
--------------------
Benchmark clickbench_partitioned.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query        ┃        HEAD ┃ refactor_binary-op-kernels ┃        Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 0     │     2.33 ms │                    2.62 ms │  1.12x slower │
│ QQuery 1     │    51.72 ms │                   52.62 ms │     no change │
│ QQuery 2     │   141.25 ms │                  145.53 ms │     no change │
│ QQuery 3     │   171.33 ms │                  166.02 ms │     no change │
│ QQuery 4     │  1122.92 ms │                 1142.30 ms │     no change │
│ QQuery 5     │  1573.44 ms │                 1611.35 ms │     no change │
│ QQuery 6     │     2.54 ms │                    2.64 ms │     no change │
│ QQuery 7     │    58.06 ms │                   57.35 ms │     no change │
│ QQuery 8     │  1540.92 ms │                 1556.65 ms │     no change │
│ QQuery 9     │  1985.43 ms │                 2056.65 ms │     no change │
│ QQuery 10    │   402.87 ms │                  394.54 ms │     no change │
│ QQuery 11    │   447.07 ms │                  459.43 ms │     no change │
│ QQuery 12    │  1452.58 ms │                 1494.46 ms │     no change │
│ QQuery 13    │  2194.89 ms │                 2306.34 ms │  1.05x slower │
│ QQuery 14    │  1317.27 ms │                 1335.48 ms │     no change │
│ QQuery 15    │  1318.68 ms │                 1303.06 ms │     no change │
│ QQuery 16    │  2753.22 ms │                 2786.68 ms │     no change │
│ QQuery 17    │  2760.12 ms │                 2801.03 ms │     no change │
│ QQuery 18    │  5455.47 ms │                 5264.98 ms │     no change │
│ QQuery 19    │   134.22 ms │                  132.24 ms │     no change │
│ QQuery 20    │  2089.54 ms │                 2092.51 ms │     no change │
│ QQuery 21    │  2383.69 ms │                 2441.90 ms │     no change │
│ QQuery 22    │  4131.35 ms │                 4089.70 ms │     no change │
│ QQuery 23    │ 12884.15 ms │                12970.55 ms │     no change │
│ QQuery 24    │   225.28 ms │                  233.08 ms │     no change │
│ QQuery 25    │   524.05 ms │                  509.14 ms │     no change │
│ QQuery 26    │   230.06 ms │                  217.43 ms │ +1.06x faster │
│ QQuery 27    │  2993.39 ms │                 2937.78 ms │     no change │
│ QQuery 28    │ 23641.71 ms │                25179.86 ms │  1.07x slower │
│ QQuery 29    │   987.02 ms │                  992.22 ms │     no change │
│ QQuery 30    │  1389.28 ms │                 1367.61 ms │     no change │
│ QQuery 31    │  1396.79 ms │                 1410.10 ms │     no change │
│ QQuery 32    │  4946.49 ms │                 4874.61 ms │     no change │
│ QQuery 33    │  6288.18 ms │                 6134.22 ms │     no change │
│ QQuery 34    │  6477.72 ms │                 6570.06 ms │     no change │
│ QQuery 35    │  2231.36 ms │                 2198.24 ms │     no change │
│ QQuery 36    │   125.31 ms │                  127.16 ms │     no change │
│ QQuery 37    │    53.09 ms │                   54.80 ms │     no change │
│ QQuery 38    │   123.82 ms │                  124.77 ms │     no change │
│ QQuery 39    │   212.47 ms │                  214.15 ms │     no change │
│ QQuery 40    │    44.33 ms │                   42.93 ms │     no change │
│ QQuery 41    │    41.66 ms │                   42.02 ms │     no change │
│ QQuery 42    │    35.82 ms │                   36.83 ms │     no change │
└──────────────┴─────────────┴────────────────────────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┓
┃ Benchmark Summary                         ┃            ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━┩
│ Total Time (HEAD)                         │ 98342.89ms │
│ Total Time (refactor_binary-op-kernels)   │ 99933.67ms │
│ Average Time (HEAD)                       │  2287.04ms │
│ Average Time (refactor_binary-op-kernels) │  2324.04ms │
│ Queries Faster                            │          1 │
│ Queries Slower                            │          3 │
│ Queries with No Change                    │         39 │
│ Queries with Failure                      │          0 │
└───────────────────────────────────────────┴────────────┘
--------------------
Benchmark tpch_mem_sf1.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query        ┃      HEAD ┃ refactor_binary-op-kernels ┃        Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 1     │ 176.46 ms │                  216.57 ms │  1.23x slower │
│ QQuery 2     │  26.11 ms │                   30.04 ms │  1.15x slower │
│ QQuery 3     │  42.76 ms │                   44.07 ms │     no change │
│ QQuery 4     │  29.15 ms │                   33.34 ms │  1.14x slower │
│ QQuery 5     │  80.62 ms │                   93.57 ms │  1.16x slower │
│ QQuery 6     │  20.04 ms │                   27.21 ms │  1.36x slower │
│ QQuery 7     │ 239.13 ms │                  259.66 ms │  1.09x slower │
│ QQuery 8     │  33.87 ms │                   36.18 ms │  1.07x slower │
│ QQuery 9     │ 111.65 ms │                  122.81 ms │  1.10x slower │
│ QQuery 10    │  62.78 ms │                   77.52 ms │  1.23x slower │
│ QQuery 11    │  17.95 ms │                   19.36 ms │  1.08x slower │
│ QQuery 12    │  53.79 ms │                   53.10 ms │     no change │
│ QQuery 13    │  49.06 ms │                   52.24 ms │  1.06x slower │
│ QQuery 14    │  15.21 ms │                   15.18 ms │     no change │
│ QQuery 15    │  25.50 ms │                   26.34 ms │     no change │
│ QQuery 16    │  25.11 ms │                   25.93 ms │     no change │
│ QQuery 17    │ 161.23 ms │                  165.09 ms │     no change │
│ QQuery 18    │ 332.17 ms │                  339.49 ms │     no change │
│ QQuery 19    │  37.31 ms │                   37.26 ms │     no change │
│ QQuery 20    │  51.34 ms │                   51.51 ms │     no change │
│ QQuery 21    │ 358.97 ms │                  331.05 ms │ +1.08x faster │
│ QQuery 22    │  20.60 ms │                   20.69 ms │     no change │
└──────────────┴───────────┴────────────────────────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━┓
┃ Benchmark Summary                         ┃           ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━┩
│ Total Time (HEAD)                         │ 1970.80ms │
│ Total Time (refactor_binary-op-kernels)   │ 2078.20ms │
│ Average Time (HEAD)                       │   89.58ms │
│ Average Time (refactor_binary-op-kernels) │   94.46ms │
│ Queries Faster                            │         1 │
│ Queries Slower                            │        11 │
│ Queries with No Change                    │        10 │
│ Queries with Failure                      │         0 │
└───────────────────────────────────────────┴───────────┘

alamb

This is amazing @sunng87 -- thank you

datafusion/physical-expr/src/expressions/binary/kernels.rs

alamb · 2025-10-10T18:10:01Z

datafusion/physical-expr/src/expressions/binary/kernels.rs

+                    other
+                ),
+            }.map(
+                // downcast_dictionary_array duplicates code per possible key type, so we aim to do all prep work before


eventually it would be nicer to preserve the dictionary encoding (but I realize this PR doesn't change the unpacking pattern here)

Yes this patch doesn't have any feature change, it's just some housekeeping work.

sunng87 added 2 commits October 9, 2025 04:33

refactor: rename macros full clear meaning

7c91882

refactor: cleanup and simplify regexp match macros

fa39b2e

github-actions bot added the physical-expr Changes to the physical-expr crates label Oct 8, 2025

alamb approved these changes Oct 11, 2025

View reviewed changes

refactor: update error messages

10d431f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: cleanup naming and macro usages for binary operator #17985

refactor: cleanup naming and macro usages for binary operator #17985

sunng87 commented Oct 8, 2025 •

edited

Loading

Uh oh!

alamb commented Oct 9, 2025

Uh oh!

alamb commented Oct 10, 2025

Uh oh!

alamb commented Oct 10, 2025

Uh oh!

alamb commented Oct 10, 2025

Uh oh!

alamb commented Oct 10, 2025

Uh oh!

alamb commented Oct 10, 2025

Uh oh!

alamb commented Oct 10, 2025

Uh oh!

alamb left a comment

Uh oh!

Uh oh!

Uh oh!

alamb Oct 10, 2025

Uh oh!

sunng87 Oct 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

refactor: cleanup naming and macro usages for binary operator #17985

Are you sure you want to change the base?

refactor: cleanup naming and macro usages for binary operator #17985

Conversation

sunng87 commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alamb commented Oct 9, 2025

Uh oh!

alamb commented Oct 10, 2025

Uh oh!

alamb commented Oct 10, 2025

Uh oh!

alamb commented Oct 10, 2025

Uh oh!

alamb commented Oct 10, 2025

Uh oh!

alamb commented Oct 10, 2025

Uh oh!

alamb commented Oct 10, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alamb Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

sunng87 Oct 11, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sunng87 commented Oct 8, 2025 •

edited

Loading