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

editoast: fix: diesel max params on big infra #2334

Merged
merged 3 commits into from
Nov 23, 2022
Merged

Conversation

Khoyo
Copy link
Contributor

@Khoyo Khoyo commented Nov 14, 2022

Diesel only accepts 65535 parameters per query, so we need to chunk our insertions when importing big infras.

Fixes #2328.
Fixes #2301.

@Khoyo Khoyo self-assigned this Nov 14, 2022
@Khoyo
Copy link
Contributor Author

Khoyo commented Nov 14, 2022

TODO:

  • A test
  • We need to refactor these methods, they pretty much do 11 time the same thing

@Khoyo Khoyo marked this pull request as draft November 14, 2022 10:30
@codecov
Copy link

codecov bot commented Nov 14, 2022

Codecov Report

Merging #2334 (ea818a0) into dev (0517a18) will decrease coverage by 0.73%.
The diff coverage is n/a.

❗ Current head ea818a0 differs from pull request most recent head ede0ae3. Consider uploading reports for the commit ede0ae3 to get more accurate results

@@             Coverage Diff              @@
##                dev    #2334      +/-   ##
============================================
- Coverage     80.15%   79.42%   -0.74%     
- Complexity     1650     1742      +92     
============================================
  Files           237      284      +47     
  Lines          7443     7942     +499     
  Branches        931      976      +45     
============================================
+ Hits           5966     6308     +342     
- Misses         1143     1279     +136     
- Partials        334      355      +21     
Flag Coverage Δ
editoast 69.04% <ø> (-1.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
editoast/src/schema/buffer_stop.rs 76.92% <ø> (-5.69%) ⬇️
editoast/src/schema/catenary.rs 100.00% <ø> (+5.88%) ⬆️
editoast/src/schema/detector.rs 76.92% <ø> (-5.69%) ⬇️
editoast/src/schema/operational_point.rs 76.92% <ø> (-5.69%) ⬇️
editoast/src/schema/railjson.rs 86.20% <ø> (ø)
editoast/src/schema/route.rs 100.00% <ø> (+5.88%) ⬆️
editoast/src/schema/signal.rs 76.92% <ø> (-5.69%) ⬇️
editoast/src/schema/speed_section.rs 72.72% <ø> (-8.23%) ⬇️
editoast/src/schema/switch.rs 76.92% <ø> (-5.69%) ⬇️
editoast/src/schema/switch_type.rs 100.00% <ø> (+5.88%) ⬆️
... and 66 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Khoyo Khoyo force-pushed the yk/fix-diesel-max-param branch 2 times, most recently from 1f22033 to ea818a0 Compare November 18, 2022 09:17
@Khoyo
Copy link
Contributor Author

Khoyo commented Nov 18, 2022

About the refactor:

Why not a generic function? Mostly because diesel goes insane when you try to write it. If someone is able too, please do.

Why not raw SQL? Because AFAIK we don't have a good way to escape SQL in diesel (basically we must call the unsafe fn PQescapeLiteral()), and we'd loose the compile time type safety provided by diesel. Still, I'm open to it if people think a macro is too big a gun, I know I can reach for them to fast :)

Codecov is complaining, but does it really make sense to move the test into the macro or duplicate it 11 times ?

@Khoyo Khoyo requested a review from flomonster November 18, 2022 09:51
@Khoyo Khoyo marked this pull request as ready for review November 18, 2022 09:51
@Khoyo Khoyo changed the title [WIP] editoast: fix: diesel max params on big infra editoast: fix: diesel max params on big infra Nov 18, 2022
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

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

Great PR!

@Khoyo Khoyo force-pushed the yk/fix-diesel-max-param branch from ea818a0 to 68f41e3 Compare November 21, 2022 13:16
@Khoyo Khoyo requested a review from flomonster November 21, 2022 13:49
flomonster
flomonster previously approved these changes Nov 23, 2022
Diesel only accepts 65535 parameters per query, so we need to chunk our
insertions when importing big infras.

Fixes #2328.
@flomonster flomonster force-pushed the yk/fix-diesel-max-param branch from 769131b to ede0ae3 Compare November 23, 2022 14:35
@flomonster flomonster merged commit 7cba96b into dev Nov 23, 2022
@flomonster flomonster deleted the yk/fix-diesel-max-param branch November 23, 2022 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import railjson command fails with big infra Refactor persist batch functions in editoast
2 participants