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: change some hashbrown RawTable uses to HashTable #13514

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

crepererum
Copy link
Contributor

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 Physical Expressions 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
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

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
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
27 checks passed
@jayzhan211
Copy link
Contributor

Thanks @crepererum @alamb

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

Successfully merging this pull request may close these issues.

3 participants