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

use redis pipeline to write CopyToString #46

Open
wants to merge 3 commits into
base: unstable
Choose a base branch
from

Conversation

Fajrinmk
Copy link

@Fajrinmk Fajrinmk commented Oct 1, 2021

To solve issue #45

before data pipelining

> go test -v -db mysql -rows 10000
Preparing Test...
=== RUN   TestCopyToString
--- PASS: TestCopyToString (11.00s)
=== RUN   TestCopyToList
--- PASS: TestCopyToList (1.25s)
=== RUN   TestCopyToHash
--- PASS: TestCopyToHash (1.25s)
PASS
ok      github.com/DGKSK8LIFE/redisql   18.056s

after data pipelining

> go test -v -db mysql -rows 10000
Preparing Test...
=== RUN   TestCopyToString
--- PASS: TestCopyToString (1.31s)
=== RUN   TestCopyToList
--- PASS: TestCopyToList (1.05s)
=== RUN   TestCopyToHash
--- PASS: TestCopyToHash (1.15s)
PASS
ok      github.com/DGKSK8LIFE/redisql   7.865s

Copy link
Owner

@DGKSK8LIFE DGKSK8LIFE 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 so far. Can you replicate pipelining for lists and hashes as well? Also, please run the tests w/ 50k rows or more (this is more realistic), with Postgres as well if possible. Want to experiment with Redis transactions after pipeline implementation too.

@Fajrinmk
Copy link
Author

Fajrinmk commented Oct 1, 2021

Should we replicate the pipelining for lists and hashes? I think we could try to exec the pipeline after it reads through all the rows rather than just column.

@Fajrinmk
Copy link
Author

Fajrinmk commented Oct 1, 2021

I made the pipeline executed after we scanned through all rows rather than just columns.

pipeline executed after column scan (10k rows)

go test -v -db mysql -rows 10000
Preparing Test...
=== RUN   TestCopyToString
--- PASS: TestCopyToString (1.31s)
=== RUN   TestCopyToList
--- PASS: TestCopyToList (1.05s)
=== RUN   TestCopyToHash
--- PASS: TestCopyToHash (1.15s)
PASS
ok      github.com/DGKSK8LIFE/redisql   7.865s

pipeline executed after row scan (10k rows)

go test -v -db mysql -rows 10000
Preparing Test...
=== RUN   TestCopyToString
--- PASS: TestCopyToString (0.39s)
=== RUN   TestCopyToList
--- PASS: TestCopyToList (0.25s)
=== RUN   TestCopyToHash
--- PASS: TestCopyToHash (0.25s)
PASS
ok      github.com/DGKSK8LIFE/redisql   5.085s

pipeline executed after column scan (50k rows)

go test -v -db mysql -rows 50000
Preparing Test...
=== RUN   TestCopyToString
--- PASS: TestCopyToString (7.76s)
=== RUN   TestCopyToList
--- PASS: TestCopyToList (6.75s)
=== RUN   TestCopyToHash
--- PASS: TestCopyToHash (6.88s)
PASS
ok      github.com/DGKSK8LIFE/redisql   42.491s

pipeline executed after row scan (50k rows)

go test -v -db mysql -rows 50000
Preparing Test...
=== RUN   TestCopyToString
--- PASS: TestCopyToString (2.68s)
=== RUN   TestCopyToList
--- PASS: TestCopyToList (1.67s)
=== RUN   TestCopyToHash
--- PASS: TestCopyToHash (1.30s)
PASS
ok      github.com/DGKSK8LIFE/redisql   25.817s

haven't test on postgres yet, will update you later

@DGKSK8LIFE
Copy link
Owner

DGKSK8LIFE commented Oct 1, 2021

Should we replicate the pipelining for lists and hashes? I think we could try to exec the pipeline after it reads through all the rows rather than just column.

Agreed. Yeah, bulk insertion into Redis is better overall (IMO we should do pipelining for all datatypes). Try to implement it similarly across all data types and optimize it accordingly if possible. Really great contributions so far 👍🏻. The goal all in all is <5 seconds per function for 1million+ rows. Please join the Discord so that we can communicate more efficiently, thanks.

Copy link
Owner

@DGKSK8LIFE DGKSK8LIFE left a comment

Choose a reason for hiding this comment

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

Good work, but I prefer @cappe987's solution, so most likely going to merge it. I will approve and add the tag so that this PR is counted in your hacktoberfest profile (I won't close). Let me know if you have any questions, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants