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.

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

Refactor ByteGroupValueBuilder to use MaybeNullBufferBuilder #12681

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Sep 30, 2024

Which issue does this PR close?

Follow on to #12269 from @jayzhan211 and #12623
Part of #12680

Rationale for this change

#12623 added a more performant way to track nulls in the PrimitiveGroupValueBuilder. Let's use the same thing for Strings/Binary views to reduce replication

What changes are included in this PR?

  1. Refactor ByteGroupValueBuilder to use MaybeNullBufferBuilder to track nulls

Are these changes tested?

Yes, by existing CI

Are there any user-facing changes?

No, this is an internal refactor

Performance (basically all changes within measurement threshold in my opinion)

--------------------
Benchmark clickbench_partitioned.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┓
┃ Query        ┃  main_base ┃ alamb_boolean_string_groups ┃       Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━┩
│ QQuery 0     │     2.19ms │                      2.47ms │ 1.12x slower │
│ QQuery 1     │    38.50ms │                     39.33ms │    no change │
│ QQuery 2     │    94.50ms │                     99.33ms │ 1.05x slower │
│ QQuery 3     │    99.87ms │                     99.86ms │    no change │
│ QQuery 4     │   932.23ms │                    931.81ms │    no change │
│ QQuery 5     │   970.52ms │                    989.52ms │    no change │
│ QQuery 6     │    37.22ms │                     36.55ms │    no change │
│ QQuery 7     │    42.96ms │                     42.71ms │    no change │
│ QQuery 8     │  1371.71ms │                   1354.54ms │    no change │
│ QQuery 9     │  1362.68ms │                   1360.57ms │    no change │
│ QQuery 10    │   342.18ms │                    344.68ms │    no change │
│ QQuery 11    │   395.77ms │                    395.40ms │    no change │
│ QQuery 12    │  1114.55ms │                   1086.39ms │    no change │
│ QQuery 13    │  1592.74ms │                   1714.17ms │ 1.08x slower │
│ QQuery 14    │  1242.36ms │                   1247.77ms │    no change │
│ QQuery 15    │  1087.20ms │                   1095.63ms │    no change │
│ QQuery 16    │  2570.55ms │                   2499.46ms │    no change │
│ QQuery 17    │  2351.69ms │                   2298.15ms │    no change │
│ QQuery 18    │  5035.87ms │                   4970.81ms │    no change │
│ QQuery 19    │    94.64ms │                     96.48ms │    no change │
│ QQuery 20    │  1744.71ms │                   1788.44ms │    no change │
│ QQuery 21    │  2006.06ms │                   2047.56ms │    no change │
│ QQuery 22    │  5202.56ms │                   5173.18ms │    no change │
│ QQuery 23    │ 10514.35ms │                  10580.71ms │    no change │
│ QQuery 24    │   600.74ms │                    609.46ms │    no change │
│ QQuery 25    │   493.64ms │                    516.33ms │    no change │
│ QQuery 26    │   666.07ms │                    666.23ms │    no change │
│ QQuery 27    │  2604.39ms │                   2585.89ms │    no change │
│ QQuery 28    │ 14826.24ms │                  14614.83ms │    no change │
│ QQuery 29    │   526.18ms │                    533.71ms │    no change │
│ QQuery 30    │  1044.06ms │                   1064.09ms │    no change │
│ QQuery 31    │  1137.63ms │                   1123.56ms │    no change │
│ QQuery 32    │  4267.90ms │                   4404.10ms │    no change │
│ QQuery 33    │  5117.07ms │                   5175.47ms │    no change │
│ QQuery 34    │  5179.28ms │                   5224.15ms │    no change │
│ QQuery 35    │  1907.07ms │                   1909.05ms │    no change │
│ QQuery 36    │   280.77ms │                    276.40ms │    no change │
│ QQuery 37    │   123.32ms │                    122.66ms │    no change │
│ QQuery 38    │   147.87ms │                    145.55ms │    no change │
│ QQuery 39    │   788.15ms │                    795.73ms │    no change │
│ QQuery 40    │    56.02ms │                     61.31ms │ 1.09x slower │
│ QQuery 41    │    47.95ms │                     50.08ms │    no change │
│ QQuery 42    │    62.18ms │                     66.27ms │ 1.07x slower │
└──────────────┴────────────┴─────────────────────────────┴──────────────┘

--------------------
Benchmark tpch_sf1.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query        ┃ main_base ┃ alamb_boolean_string_groups ┃        Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 1     │  190.34ms │                    189.37ms │     no change │
│ QQuery 2     │  123.05ms │                    120.34ms │     no change │
│ QQuery 3     │  129.96ms │                    121.71ms │ +1.07x faster │
│ QQuery 4     │   86.80ms │                     89.76ms │     no change │
│ QQuery 5     │  157.98ms │                    165.57ms │     no change │
│ QQuery 6     │   45.93ms │                     45.36ms │     no change │
│ QQuery 7     │  217.67ms │                    192.33ms │ +1.13x faster │
│ QQuery 8     │  176.63ms │                    148.94ms │ +1.19x faster │
│ QQuery 9     │  243.65ms │                    240.19ms │     no change │
│ QQuery 10    │  235.80ms │                    229.71ms │     no change │
│ QQuery 11    │   94.14ms │                     94.43ms │     no change │
│ QQuery 12    │  129.70ms │                    129.90ms │     no change │
│ QQuery 13    │  220.74ms │                    218.86ms │     no change │
│ QQuery 14    │   73.18ms │                     72.92ms │     no change │
│ QQuery 15    │  124.76ms │                    121.70ms │     no change │
│ QQuery 16    │   78.78ms │                     80.14ms │     no change │
│ QQuery 17    │  203.80ms │                    227.06ms │  1.11x slower │
│ QQuery 18    │  329.27ms │                    334.30ms │     no change │
│ QQuery 19    │  126.43ms │                    133.05ms │  1.05x slower │
│ QQuery 20    │  130.58ms │                    137.04ms │     no change │
│ QQuery 21    │  281.02ms │                    271.38ms │     no change │
│ QQuery 22    │   67.55ms │                     65.00ms │     no change │
└──────────────┴───────────┴─────────────────────────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━┓
┃ Benchmark Summary                          ┃           ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━┩
│ Total Time (main_base)                     │ 3467.77ms │
│ Total Time (alamb_boolean_string_groups)   │ 3429.05ms │
│ Average Time (main_base)                   │  157.63ms │
│ Average Time (alamb_boolean_string_groups) │  155.87ms │
│ Queries Faster                             │         3 │
│ Queries Slower                             │         2 │
│ Queries with No Change                     │        17 │
└────────────────────────────────────────────┴───────────┘


@github-actions github-actions bot added the physical-expr Physical Expressions label Sep 30, 2024
@alamb alamb marked this pull request as draft September 30, 2024 10:25
@Dandandan
Copy link
Contributor

Dandandan commented Sep 30, 2024

I left a comment here: 36a2003#r1781444375 , I think the code could be simplified a bit by using generics a bit more.

@alamb
Copy link
Contributor Author

alamb commented Sep 30, 2024

I left a comment here: 36a2003#r1781444375 , I think the code could be simplified a bit by using generics a bit more.

Thank you @Dandandan -- added to list on #12680

(note that link doesn't seem to work for me, but I found the relevant comment #12623 (comment))

@alamb alamb marked this pull request as ready for review October 1, 2024 14:52
@alamb
Copy link
Contributor Author

alamb commented Oct 1, 2024

This appears to have made another significant improvement in TPCH (presumably because the group keys aren't ever null)

@alamb alamb requested a review from jayzhan211 October 1, 2024 14:53
Copy link
Contributor

@Dandandan Dandandan left a comment

Choose a reason for hiding this comment

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

👌

@Dandandan
Copy link
Contributor

Really nice performance improvements 🚀

@Dandandan
Copy link
Contributor

Dandandan commented Oct 1, 2024

Hm actually looking at the performance change of TPCH sf 1,this doesn't seem correct to me (main seems out of date?).

@alamb
Copy link
Contributor Author

alamb commented Oct 1, 2024

Hm actually looking at the performance change of TPCH sf 1,this doesn't seem correct to me (main seems out of date?).

Yes, sorry -- I am rerunning the performance -- I think this is reporting based on main before @jayzhan211 's changes. Will update shortly

@alamb
Copy link
Contributor Author

alamb commented Oct 1, 2024

I re-ran benchmarks and posted them above -- TLDR is I don't think there are any significant differences in performance

@alamb
Copy link
Contributor Author

alamb commented Oct 1, 2024

I can potentially try the const NULLABLE thing too if we think that is worthwhile (I think it is likely that the special case for knowing about nullability is not super applicable to the real world -- aka anything other than TPCH)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants