Skip to content

Comments

refactor: change some hashbrown RawTable uses to HashTable#13514

Merged
jayzhan211 merged 3 commits intoapache:mainfrom
crepererum:crepererum/issue13256_a
Nov 22, 2024
Merged

refactor: change some hashbrown RawTable uses to HashTable#13514
jayzhan211 merged 3 commits intoapache:mainfrom
crepererum:crepererum/issue13256_a

Conversation

@crepererum
Copy link
Contributor

@crepererum crepererum commented Nov 21, 2024

Which issue does this PR close?

For #13256, but only parts of it.

Rationale for this change

Prepare hashbrown 0.15 upgrade.

What changes are included in this PR?

  • HashTableAllocExt
  • a few migrations

Are these changes tested?

Existing tests pass.

Are there any user-facing changes?

No.

@github-actions github-actions bot added physical-expr Changes to the physical-expr crates common Related to common crate execution Related to the execution crate labels Nov 21, 2024
@crepererum crepererum changed the title Crepererum/issue13256 a refactor: remove some RawTable uses Nov 21, 2024
alamb
alamb reviewed Nov 21, 2024
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks good -- I am running benchmarks and will report back

@alamb
Copy link
Contributor

alamb commented Nov 21, 2024 

--------------------
Benchmark clickbench_partitioned.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query        ┃  main_base ┃ crepererum_issue13256_a ┃        Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 0     │     2.34ms │                  2.37ms │     no change │
│ QQuery 1     │    40.18ms │                 39.59ms │     no change │
│ QQuery 2     │    99.42ms │                100.48ms │     no change │
│ QQuery 3     │   106.39ms │                106.12ms │     no change │
│ QQuery 4     │   933.43ms │                927.13ms │     no change │
│ QQuery 5     │   989.52ms │                972.64ms │     no change │
│ QQuery 6     │    38.58ms │                 38.39ms │     no change │
│ QQuery 7     │    45.45ms │                 43.92ms │     no change │
│ QQuery 8     │  1026.53ms │               1012.94ms │     no change │
│ QQuery 9     │  1342.47ms │               1363.91ms │     no change │
│ QQuery 10    │   288.16ms │                284.89ms │     no change │
│ QQuery 11    │   322.18ms │                321.03ms │     no change │
│ QQuery 12    │   991.20ms │                991.09ms │     no change │
│ QQuery 13    │  1325.87ms │               1479.59ms │  1.12x slower │
│ QQuery 14    │   905.00ms │                899.24ms │     no change │
│ QQuery 15    │  1091.14ms │               1112.17ms │     no change │
│ QQuery 16    │  2013.62ms │               1974.57ms │     no change │
│ QQuery 17    │  1864.66ms │               1856.42ms │     no change │
│ QQuery 18    │  4241.95ms │               4269.15ms │     no change │
│ QQuery 19    │    96.79ms │                 97.41ms │     no change │
│ QQuery 20    │  1260.72ms │               1263.79ms │     no change │
│ QQuery 21    │  1520.02ms │               1485.24ms │     no change │
│ QQuery 22    │  2689.61ms │               2604.13ms │     no change │
│ QQuery 23    │  8695.66ms │               8545.35ms │     no change │
│ QQuery 24    │   519.05ms │                508.12ms │     no change │
│ QQuery 25    │   432.17ms │                419.79ms │     no change │
│ QQuery 26    │   584.90ms │                568.44ms │     no change │
│ QQuery 27    │  1896.34ms │               1880.11ms │     no change │
│ QQuery 28    │ 12967.65ms │              13275.59ms │     no change │
│ QQuery 29    │   537.12ms │                517.73ms │     no change │
│ QQuery 30    │   911.13ms │                893.50ms │     no change │
│ QQuery 31    │   963.30ms │                951.89ms │     no change │
│ QQuery 32    │  3965.29ms │               4077.55ms │     no change │
│ QQuery 33    │  4087.85ms │               4074.45ms │     no change │
│ QQuery 34    │  4098.94ms │               4100.04ms │     no change │
│ QQuery 35    │  1314.92ms │               1338.07ms │     no change │
│ QQuery 36    │   228.25ms │                241.17ms │  1.06x slower │
│ QQuery 37    │    96.82ms │                 98.40ms │     no change │
│ QQuery 38    │   141.01ms │                138.94ms │     no change │
│ QQuery 39    │   456.13ms │                457.21ms │     no change │
│ QQuery 40    │    57.77ms │                 54.53ms │ +1.06x faster │
│ QQuery 41    │    49.43ms │                 47.19ms │     no change │
│ QQuery 42    │    63.20ms │                 64.30ms │     no change │
└──────────────┴────────────┴─────────────────────────┴───────────────┘

I will run it again and see if it is reproducable

@crepererum
Copy link
Contributor Author

crepererum commented Nov 21, 2024

I would be surprised if there's a notable difference, esp. for this change. Under the hood, it's basically all the same, see

https://docs.rs/hashbrown/0.14.5/src/hashbrown/table.rs.html

@alamb
Copy link
Contributor

alamb commented Nov 21, 2024

I would be surprised if there's a notable difference, esp. for this change. Under the hood, it's basically all the same, see

I agree - I just want to be sure

@alamb
Copy link
Contributor

alamb commented Nov 21, 2024

I ran the benchmarks again and see no change. Thus I conclude this PR does not change performance significantly

--------------------
Benchmark clickbench_partitioned.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┓
┃ Query        ┃  main_base ┃ crepererum_issue13256_a ┃       Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━┩
│ QQuery 0     │     2.40ms │                  2.35ms │    no change │
│ QQuery 1     │    40.13ms │                 40.60ms │    no change │
│ QQuery 2     │    98.55ms │                 96.76ms │    no change │
│ QQuery 3     │   108.03ms │                110.34ms │    no change │
│ QQuery 4     │   946.21ms │                911.31ms │    no change │
│ QQuery 5     │   988.30ms │                956.43ms │    no change │
│ QQuery 6     │    39.03ms │                 37.15ms │    no change │
│ QQuery 7     │    42.98ms │                 43.53ms │    no change │
│ QQuery 8     │  1022.74ms │                993.36ms │    no change │
│ QQuery 9     │  1350.46ms │               1325.77ms │    no change │
│ QQuery 10    │   289.45ms │                287.22ms │    no change │
│ QQuery 11    │   324.72ms │                314.36ms │    no change │
│ QQuery 12    │  1005.26ms │                976.73ms │    no change │
│ QQuery 13    │  1467.81ms │               1440.12ms │    no change │
│ QQuery 14    │   925.44ms │                880.82ms │    no change │
│ QQuery 15    │  1095.23ms │               1078.33ms │    no change │
│ QQuery 16    │  2049.77ms │               1997.86ms │    no change │
│ QQuery 17    │  1875.25ms │               1845.67ms │    no change │
│ QQuery 18    │  4307.22ms │               4249.00ms │    no change │
│ QQuery 19    │    96.50ms │                 95.52ms │    no change │
│ QQuery 20    │  1245.65ms │               1248.43ms │    no change │
│ QQuery 21    │  1529.59ms │               1456.04ms │    no change │
│ QQuery 22    │  2693.39ms │               2597.35ms │    no change │
│ QQuery 23    │  8735.35ms │               8469.37ms │    no change │
│ QQuery 24    │   530.10ms │                510.30ms │    no change │
│ QQuery 25    │   436.66ms │                418.76ms │    no change │
│ QQuery 26    │   590.06ms │                573.77ms │    no change │
│ QQuery 27    │  1904.35ms │               1848.69ms │    no change │
│ QQuery 28    │ 13237.28ms │              13498.55ms │    no change │
│ QQuery 29    │   533.29ms │                531.26ms │    no change │
│ QQuery 30    │   916.57ms │                887.13ms │    no change │
│ QQuery 31    │   962.64ms │                934.63ms │    no change │
│ QQuery 32    │  4134.49ms │               4093.42ms │    no change │
│ QQuery 33    │  4093.70ms │               4044.19ms │    no change │
│ QQuery 34    │  4152.12ms │               4067.67ms │    no change │
│ QQuery 35    │  1357.88ms │               1346.74ms │    no change │
│ QQuery 36    │   240.73ms │                228.88ms │    no change │
│ QQuery 37    │    96.36ms │                 97.53ms │    no change │
│ QQuery 38    │   136.34ms │                134.69ms │    no change │
│ QQuery 39    │   442.49ms │                439.20ms │    no change │
│ QQuery 40    │    55.78ms │                 60.72ms │ 1.09x slower │
│ QQuery 41    │    46.95ms │                 46.35ms │    no change │
│ QQuery 42    │    63.32ms │                 63.70ms │    no change │

@alamb alamb changed the title refactor: remove some RawTable uses refactor: change some hashbrown RawTable uses to HashTable Nov 21, 2024
alamb
alamb approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @crepererum

@jayzhan211 jayzhan211 merged commit 207e855 into apache:main Nov 22, 2024
@jayzhan211
Copy link
Contributor

jayzhan211 commented Nov 22, 2024

Thanks @crepererum @alamb

@crepererum crepererum mentioned this pull request Nov 22, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

@jayzhan211 jayzhan211 jayzhan211 approved these changes

Assignees

No one assigned

Labels

common Related to common crate execution Related to the execution crate physical-expr Changes to the physical-expr crates

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@crepererum @alamb @jayzhan211