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

Make select Faster #154

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Make select Faster #154

wants to merge 6 commits into from

Conversation

mmontagna
Copy link
Contributor

@mmontagna mmontagna commented Jan 30, 2020

What

Use Psycopg2 directly for PG selects in order to improve performance.

Why

sqlalchemy ads ~1ms of additional latency for basic selects and more for queries which return large result sets.

Without sqlalchemy

➜  test git:(marco-make-select-faster) ✗ lore python sql_test.py
...
raw rows average runtime 0.23196000000000003 ms

With sqlalchemy

➜  test git:(marco-make-select-faster) ✗ lore python sql_test.py
...
raw rows average runtime 1.371959 ms

sql_test.py

import lore, lore.io
import time
import numpy as np
runtimes = []

main = lore.io.main


for item_id in range(0, 10000):
    st = time.time()
    results = main.select(sql='select 1 where 1 = 1;')
    runtime = round((time.time() - st)*1000.0, 2)
    runtimes.append(runtime)

avg_runtime = np.mean(runtimes)

print("raw rows average runtime {} ms".format(avg_runtime))

@mmontagna mmontagna marked this pull request as ready for review January 31, 2020 00:16
@@ -272,7 +276,21 @@ def select(self, sql=None, extract=None, filename=None, **kwargs):

@query_cached
def _select(self, sql, bindings):
return self.__execute(sql, bindings).fetchall()
if self._use_psycopg2:
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if there is value in falling back to SQL alchemy as a final try before giving up completely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker though. This looks good!

Copy link

@ilystsov ilystsov left a comment

Choose a reason for hiding this comment

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

If self._use_psycopg2 is False, we immediately return the result for the alternative execution.
Merged the two with statements into one line.
if the error isn't about "no results to fetch", the original exception will be automatically re-raised without needing to explicitly call raise e.

Comment on lines +441 to +453
def _connection_execute(self, sql, bindings):
if self._use_psycopg2:
with self._connection.engine.raw_connection().connection as conn:
with conn.cursor() as cursor:
cursor.execute(sql, bindings)
try:
return ResultWrapper(cursor.fetchall())
except psycopg2.ProgrammingError as e:
if 'no results to fetch' in str(e):
return None
raise e
else:
return self._connection.execute(sql, bindings)

Choose a reason for hiding this comment

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

Suggested change
def _connection_execute(self, sql, bindings):
if self._use_psycopg2:
with self._connection.engine.raw_connection().connection as conn:
with conn.cursor() as cursor:
cursor.execute(sql, bindings)
try:
return ResultWrapper(cursor.fetchall())
except psycopg2.ProgrammingError as e:
if 'no results to fetch' in str(e):
return None
raise e
else:
return self._connection.execute(sql, bindings)
def _connection_execute(self, sql, bindings):
if not self._use_psycopg2:
return self._connection.execute(sql, bindings)
try:
with self._connection.engine.raw_connection().connection as conn, conn.cursor() as cursor:
cursor.execute(sql, bindings)
return ResultWrapper(cursor.fetchall())
except psycopg2.ProgrammingError as e:
if 'no results to fetch' not in str(e):
raise

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.

3 participants