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

Support Oracle 12c top-N query #319

Closed
wants to merge 22 commits into from
Closed

Support Oracle 12c top-N query #319

wants to merge 22 commits into from

Conversation

yahonda
Copy link
Member

@yahonda yahonda commented Sep 25, 2014

This pull request has two separate commits to support Oracle 12c better Top N query support using FETCH FIRST n ROWS ONLY and OFFSET n ROWS.

8f5e57f Extract visit_Arel_Nodes_SelectOptions

It extracts visit_Arel_Nodes_SelectOptions from visit_Arel_Nodes_SelectStatement method to prepare Oracle 12c support since it will be able to reuse most of super's visit_Arel_Nodes_SelectStatement method because Oracle requires to add collector in the following order.

offset
limit
lock

Unlike other database require in this order.

limit
offset
lock

This method is not called using visitor pattern, I'd like to hear visit_Arel_Nodes_SelectOptions is an appropreate method name.

371b642 Create Arel::Visitors::Oracle12 to provide better top-N query

Oracle Database 12c introduces better top-N query support using FETCH FIRST n ROWS ONLY and OFFSET n ROWS. Also to support older version of Oracle 11gR2 or lower version, this commit creates
another visitor named Arel::Visitors::Oracle12, 12 comes from the database version.

By changing oracle enhanced adapter as follows to execute each Arel visitor based on
if this top-n query feature supported. yahonda/oracle-enhanced@fcb22db

if supports_fetch_first_n_rows_and_offset?
  @visitor = Arel::Visitors::Oracle12.new self
else
  @visitor = Arel::Visitors::Oracle.new self
end

It looks Arel::Visitors just allows 1 database adapter to have 1 visitor, but my implementation works to switch two visitors just changing Arel::Visitors::Oracle12 or Arel::Visitors::Oracle based on its database version. It looks 1 database adapter can have 2 or more visitor(s). Are these lines can be removed?
https://github.com/yahonda/arel/blob/a19190ab06db4d68765fb08ec1a4dc6c8ebb2eb5/lib/arel/visitors.rb#L15-L41

module Arel
  module Visitors
    VISITORS = {
      'postgresql'      => Arel::Visitors::PostgreSQL,
      'mysql'           => Arel::Visitors::MySQL,
      'mysql2'          => Arel::Visitors::MySQL,
      'mssql'           => Arel::Visitors::MSSQL,
      'sqlserver'       => Arel::Visitors::MSSQL,
      'oracle_enhanced' => Arel::Visitors::Oracle,
      'sqlite'          => Arel::Visitors::SQLite,
      'sqlite3'         => Arel::Visitors::SQLite,
      'ibm_db'          => Arel::Visitors::IBM_DB,
      'informix'        => Arel::Visitors::Informix,
    }

    ENGINE_VISITORS = Hash.new do |hash, engine|
      pool         = engine.connection_pool
      adapter      = pool.spec.config[:adapter]
      hash[engine] = (VISITORS[adapter] || Visitors::ToSql).new(engine)
    end

    def self.visitor_for engine
      ENGINE_VISITORS[engine]
    end
    class << self; alias :for :visitor_for; end
  end
end

yahonda and others added 22 commits September 26, 2014 21:00
support using `FETCH FIRST n ROWS` and `OFFSET` for Oracle 12c database
This commit simply removes duplicated code by reusing the
existing `maybe_visit` method.
😓 I don't know why the tests did not fail, but to keep
the same syntax as before, `collector =` is required.

Maybe `visit` changes `collector` in-place, so the result is the
same, but since I'm not sure about the side effects, I think this
PR is needed to. Sorry! 😓
These methods are going to go through some heavy refactoring, and moving
logic around. This adds missing tests for each of the branches on the
predicate.
Currently, doing

```ruby
relation[:id].not_eq(4).and(relation[:id].not_in(1..3))
```

will generate

```sql
"id" != 4 AND "id" < 1 OR "id" > 3
```

Which would incorrectly include records with an id of 4, as the OR
statement has higher precidence than the AND statement. The `or`
method on `Node` properly groups the statement in parenthesis.
These methods duplicate a lot of logic from the other predications. We
can just use those methods directly, and only build nodes with the same
name in our method directly. We've already had one bug that came from
building nodes directly, rather than using the proper predicate.
The goal of these methods should be to generate in nodes, not handle
every possible permutation of more than one value. The `#between` and
`#not_between` methods have been extracted, which better represent the
semantics of handling ranges in SQL.
Given that we are going to remove casting from Arel in the near future,
having a single place nodes in predications will help.
This reverts commit 36836fa, reversing
changes made to 53bc842.
This reverts commit 9b92af7.

beta2 is out, and we've fixed the issue that this caused in Rails
It's not quite duck typed, but it will allow us to pass in our own
objects with additional logic (like type casting).
@yahonda
Copy link
Member Author

yahonda commented Nov 3, 2014

Failed to merge master branch. will close this pull request.

@yahonda yahonda closed this Nov 3, 2014
@yahonda yahonda deleted the support_oracle12_top_n branch July 26, 2016 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants