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.

Sign up for GitHub

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

Jump to bottom

Merge Arel visitors #2002

Merged
merged 13 commits into from Apr 15, 2020
Merged

Merge Arel visitors #2002

merged 13 commits into from Apr 15, 2020

Conversation

koic
Copy link
Collaborator

@koic koic commented Apr 15, 2020

Follow up of rails/rails#38946 and #2001.

This PR merges Arel visitors for Oracle to Active Record Oracle enhanced adapter. It includes visitors tests written in Arel::Spec (Minitest::Spec), so it should be migrated to RSpec in future.

matthewd and others added 13 commits April 15, 2020 14:48
@matthewd @koic
Merge Arel into Active Record
be42e8c
@matthewd @koic
Arel: rubocop -a
1029090
@matthewd @koic
Arel: :nodoc: all
db3ae64
@kpumuk @koic
Arel: Implemented DB-aware NULL-safe comparison (#34451)
e692334 
* Arel: Implemented DB-aware NULL-safe comparison

* Fixed where clause inversion for NULL-safe comparison

* Renaming "null_safe_eq" to "is_not_distinct_from", "null_safe_not_eq" to "is_distinct_from"

[Dmytro Shteflyuk + Rafael Mendonça França]
@kamipo @koic
Enable Lint/UselessAssignment cop to avoid unused variable warnings…
590972e 
… (#34904)

* Enable `Lint/UselessAssignment` cop to avoid unused variable warnings

Since we've addressed the warning "assigned but unused variable"
frequently.

370537de05092aeea552146b42042833212a1acc
3040446cece8e7a6d9e29219e636e13f180a1e03
5ed618e192e9788094bd92c51255dda1c4fd0eae
76ebafe594fc23abc3764acc7a3758ca473799e5

And also, I've found the unused args in c1b14ad which raises no warnings
by the cop, it shows the value of the cop.
@yahonda @koic
Address ORA-01795: maximum number of expressions in a list is 1000
041995a 
* To address this error, this commit splits expressions by slices of 1000 elements.

* "Oracle Database Error Messages 18c"
https://docs.oracle.com/en/database/oracle/oracle-database/18/errmg/

```
ORA-01795: maximum number of expressions in a list is 1000
Cause: Number of expressions in the query exceeded than 1000. Note that unused column/expressions are also counted Maximum number of expressions that are allowed are 1000.
```

* This commit addresses this ORA-01795 error

Note: Actually addressing this error raises another "ORA-00913: too many values"
Number of values Oracle database allows is 65535 regardless bind values or literal values.

```ruby
$ ARCONN=oracle bin/test test/cases/bind_parameter_test.rb -n test_too_many_binds

... snip ...

Error:
ActiveRecord::BindParameterTest#test_too_many_binds:
ActiveRecord::StatementInvalid: OCIError: ORA-01795: maximum number of expressions in a list is 1000
    stmt.c:267:in oci8lib_260.so
    /home/yahonda/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/ruby-oci8-2.2.7/lib/oci8/cursor.rb:131:in `exec'
    /home/yahonda/git/oracle-enhanced/lib/active_record/connection_adapters/oracle_enhanced/oci_connection.rb:142:in `exec'
    /home/yahonda/git/oracle-enhanced/lib/active_record/connection_adapters/oracle_enhanced/database_statements.rb:41:in `block in exec_query'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:676:in `block (2 levels) in log'
    /home/yahonda/.rbenv/versions/2.6.2/lib/ruby/2.6.0/monitor.rb:230:in `mon_synchronize'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:675:in `block in log'
    /home/yahonda/git/rails/activesupport/lib/active_support/notifications/instrumenter.rb:24:in `instrument'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:666:in `log'
    /home/yahonda/git/oracle-enhanced/lib/active_record/connection_adapters/oracle_enhanced/dbms_output.rb:36:in `log'
    /home/yahonda/git/oracle-enhanced/lib/active_record/connection_adapters/oracle_enhanced/database_statements.rb:24:in `exec_query'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:484:in `select'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:70:in `select_all'
    /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb:106:in `select_all'
    /home/yahonda/git/rails/activerecord/lib/active_record/relation/calculations.rb:299:in `block in execute_simple_calculation'
    /home/yahonda/git/rails/activerecord/lib/active_record/relation.rb:755:in `skip_query_cache_if_necessary'
    /home/yahonda/git/rails/activerecord/lib/active_record/relation/calculations.rb:299:in `execute_simple_calculation'
    /home/yahonda/git/rails/activerecord/lib/active_record/relation/calculations.rb:251:in `perform_calculation'
    /home/yahonda/git/rails/activerecord/lib/active_record/relation/calculations.rb:141:in `calculate'
    /home/yahonda/git/rails/activerecord/lib/active_record/relation/calculations.rb:49:in `count'
    /home/yahonda/git/rails/activerecord/test/cases/bind_parameter_test.rb:113:in `test_too_many_binds'

bin/test test/cases/bind_parameter_test.rb:109

.............................F

```
@kamipo @koic
Fix sliced IN clauses to be grouped
8666547 
Follow up of #35838.

And also this refactors `in_clause_length` handling is entirely
integrated in Arel visitor.
@kamipo @koic
Enable Layout/EmptyLinesAroundAccessModifier cop
4a96ba3 
We sometimes say "✂️ newline after `private`" in a code review (e.g.
rails/rails#18546 (comment),
rails/rails#34832 (comment)).

Now `Layout/EmptyLinesAroundAccessModifier` cop have new enforced style
`EnforcedStyle: only_before` (rubocop/rubocop#7059).

That cop and enforced style will reduce the our code review cost.
@amatsuda @koic
Use match? where we don't need MatchData
053e83a 
We're already running Performance/RegexpMatch cop, but it seems like the cop is not always =~ justice
@kamipo @koic
Address to rubocop offences
8ab91ce
@amatsuda @koic
Use Regext#match? where MatchData is not needed
61becb6
@yahonda @koic
Address `no implicit conversion of Arel::Attributes::Attribute into S…
74edc91 
…tring`

rails/rails@4a9ef5e triggers this failure,
then restored the original code for `activerecord/lib/arel/visitors/oracle.rb` .

```
$ bundle exec rspec ./spec/active_record/connection_adapters/oracle_enhanced/context_index_spec.rb:92
==> Loading config from ENV or use default
==> Running specs with MRI version 2.6.5
==> Effective ActiveRecord version 6.1.0.alpha
... snip ...
Run options: include {:locations=>{"./spec/active_record/connection_adapters/oracle_enhanced/context_index_spec.rb"=>[92]}}
F

Failures:

  1) OracleEnhancedAdapter context index on single table should create single VARCHAR2 column index
     Failure/Error: expect(Post.contains(:title, word).to_a).to eq([@POST2, @post1])

     TypeError:
       no implicit conversion of Arel::Attributes::Attribute into String
     # /home/yahonda/git/rails/activerecord/lib/arel/visitors/oracle.rb:107:in `match?'
     # /home/yahonda/git/rails/activerecord/lib/arel/visitors/oracle.rb:107:in `block (2 levels) in order_hacks'
     # /home/yahonda/git/rails/activerecord/lib/arel/visitors/oracle.rb:106:in `any?'
     # /home/yahonda/git/rails/activerecord/lib/arel/visitors/oracle.rb:106:in `block in order_hacks'
     # /home/yahonda/git/rails/activerecord/lib/arel/visitors/oracle.rb:105:in `any?'
     # /home/yahonda/git/rails/activerecord/lib/arel/visitors/oracle.rb:105:in `order_hacks'
     # /home/yahonda/git/rails/activerecord/lib/arel/visitors/oracle.rb:8:in `visit_Arel_Nodes_SelectStatement'
     # /home/yahonda/git/rails/activerecord/lib/arel/visitors/visitor.rb:30:in `visit'
     # /home/yahonda/git/rails/activerecord/lib/arel/visitors/visitor.rb:11:in `accept'
     # /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/determine_if_preparable_visitor.rb:10:in `accept'
     # /home/yahonda/git/rails/activerecord/lib/arel/visitors/to_sql.rb:18:in `compile'
     # /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:25:in `to_sql_and_binds'
     # /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:63:in `select_all'
     # /home/yahonda/git/rails/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb:107:in `select_all'
     # /home/yahonda/git/rails/activerecord/lib/active_record/querying.rb:46:in `find_by_sql'
     # /home/yahonda/git/rails/activerecord/lib/active_record/relation.rb:826:in `block in exec_queries'
     # /home/yahonda/git/rails/activerecord/lib/active_record/relation.rb:844:in `skip_query_cache_if_necessary'
     # /home/yahonda/git/rails/activerecord/lib/active_record/relation.rb:811:in `exec_queries'
     # /home/yahonda/git/rails/activerecord/lib/active_record/relation.rb:629:in `load'
     # /home/yahonda/git/rails/activerecord/lib/active_record/relation.rb:250:in `records'
     # /home/yahonda/git/rails/activerecord/lib/active_record/relation.rb:245:in `to_ary'
     # ./spec/active_record/connection_adapters/oracle_enhanced/context_index_spec.rb:95:in `block (4 levels) in <top (required)>'
     ... snip ...
```
@koic
Require Arel visitors
af14bb0
@koic koic mentioned this pull request Apr 15, 2020
Closed
3 tasks
@yahonda yahonda self-requested a review April 15, 2020 07:51
This was referenced Apr 15, 2020
Closed
Closed
@yahonda yahonda merged commit cac033f into rsim:master Apr 15, 2020
@yahonda
Copy link
Collaborator

yahonda commented Apr 15, 2020

Thanks for the great work.

@yahonda yahonda mentioned this pull request Apr 15, 2020
Merged
@koic koic deleted the merge_arel_visitors branch April 15, 2020 10:38
@koic
Copy link
Collaborator Author

koic commented Apr 15, 2020

Thank you too, always!

This was referenced Nov 27, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yahonda yahonda yahonda approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@koic @yahonda @matthewd @kpumuk @kamipo @amatsuda