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

query_decorator #13

Merged
merged 3 commits into from Feb 7, 2020
Merged

query_decorator #13

merged 3 commits into from Feb 7, 2020

Conversation

@ermolaev
Copy link
Contributor

ermolaev commented Jan 30, 2020

reworked #7

@slimdave
Copy link

slimdave commented Jan 30, 2020

Is decorating the results of a query difficult at the moment? I've been thinking about doing this sort of thing with ProductDecorator.decorate_collection(conn.query), for example.

@ermolaev
Copy link
Contributor Author

ermolaev commented Jan 30, 2020

Is decorating the results of a query difficult at the moment? I've been thinking about doing this sort of thing with ProductDecorator.decorate_collection(conn.query), for example.

please see PR code https://github.com/discourse/mini_sql/pull/13/files

I added info to readme and benchmarks

@SamSaffron
Copy link
Member

SamSaffron commented Jan 30, 2020

I really like the design here and feel it keeps with the design of mini_racer and performance goals.

Mixed on just supporting conn.query("select a 1", decorator: SomeModule) cause technically this is a fraction slower on 2.6 and older

@davidtaylorhq what do you think about this change?

@ermolaev
Copy link
Contributor Author

ermolaev commented Feb 2, 2020

Mixed on just supporting conn.query("select a 1", decorator: SomeModule) cause technically this is a fraction slower on 2.6 and older

2.5.5

Comparison:
     query_decorator:  7061150.6 i/s
              query2:  6442567.4 i/s - 1.10x  (± 0.02) slower
                   with 99.0% confidence

2.6.5

Comparison:
     query_decorator:  7758839.7 i/s
              query2:  6936802.5 i/s - 1.12x  (± 0.02) slower
                   with 99.0% confidence

2.7.0

Comparison:
     query_decorator:  7132525.6 i/s
              query2:  6437102.3 i/s - 1.11x  (± 0.02) slower
                   with 99.0% confidence

code

module MiniSql
  module Postgres
    class Connection < MiniSql::Connection
      def query2(sql, *params, decorator: nil)
        [sql, params, decorator]
      end

      def query_decorator(decorator, sql, *params)
        [sql, params, decorator]
      end
    end
  end
end

Benchmark.ips do |r|
  r.stats = :bootstrap
  r.confidence = 99

  r.report('query2') do |n|
    while n > 0
      $mini_sql.query2('select id, title from topics order by id limit 1000', decorator: TopicDecorator)
      n -= 1
    end
  end
  r.report('query_decorator') do |n|
    while n > 0
      $mini_sql.query_decorator(TopicDecorator, 'select id, title from topics order by id limit 1000')
      n -= 1
    end
  end
  r.compare!
end
@SamSaffron
Copy link
Member

SamSaffron commented Feb 3, 2020

Nice @ermolaev , I agree the non kwarg api is the way to go.

@ermolaev
Copy link
Contributor Author

ermolaev commented Feb 6, 2020

also I think that query_decorator more expressive for developers

$mini_sql.query_decorator(TopicDecorator, <<~SQL, param1: 'val1', param2: 'val2', param3: 'val3', param4: 'val4')
  big sql
SQL

$mini_sql.query(<<~SQL, param1: 'val1', param2: 'val2', param3: 'val3', param4: 'val4', decorator: TopicDecorator)
  big sql
SQL
@SamSaffron SamSaffron merged commit 2b147e8 into discourse:master Feb 7, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@SamSaffron
Copy link
Member

SamSaffron commented Feb 7, 2020

Thanks, yeah I agree, this api does read nicely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.