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

Add support for generated columns in SQLite3 adapter #49346

Merged
merged 1 commit into from Dec 14, 2023

Conversation

fractaledmind
Copy link
Contributor

Motivation / Background

SQLite is a feature-rich database engine, and production usage is only growing. We need Rails' support to offer developers the range and scope of its features.

Both the MySQL and PostgreSQL adapters support virtual columns, and SQLite itself has support generated columns since 2020.

Detail

PR #41856 introduced virtual columns to the PostgreSQLAdapter. This is effectively a clone of that feature, with some necessary tweaks.

create_table :users do |t|
  t.string :name
  t.virtual :name_upper, type: :string, as: 'UPPER(name)'
  t.virtual :name_lower, type: :string, as: 'LOWER(name)', stored: true
end

Firstly, SQLite supported both STORED and VIRTUAL generated columns, while PostgreSQL only supports VIRTUAL. Secondly, SQLite doesn't support ALTER TABLE commands with generated columns.

In this PR, I am adding full support for the SQLite3Adapter by implementing:

ActiveRecord::ConnectionAdapters::SQLite3::Column#virtual?
ActiveRecord::ConnectionAdapters::SQLite3Adapter#supports_virtual_columns?

and altering:

ActiveRecord::ConnectionAdapters::SQLite3::SchemaCreation#add_column_options
ActiveRecord::ConnectionAdapters::SQLite3::SchemaDefinitions#new_column_definition
ActiveRecord::ConnectionAdapters::SQLite3::SchemaDumper#prepare_column_options
ActiveRecord::ConnectionAdapters::SQLite3::SchemaStatements#new_column_from_field
ActiveRecord::ConnectionAdapters::SQLite3Adapter#table_structure
ActiveRecord::ConnectionAdapters::SQLite3Adapter#invalid_alter_table_type?
ActiveRecord::ConnectionAdapters::SQLite3Adapter#table_structure_with_collation

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

Copy link
Member

@skipkayhil skipkayhil left a comment

Choose a reason for hiding this comment

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

Small comment, but generally looks good

Copy link
Contributor

@nvasilevski nvasilevski left a comment

Choose a reason for hiding this comment

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

👍

@fractaledmind
Copy link
Contributor Author

@skipkayhil Addressed your one comment. Everything should be good to go now

@zzak zzak added this to the 7.1.0 milestone Sep 27, 2023
@rafaelfranca rafaelfranca removed this from the 7.1.0 milestone Sep 27, 2023
@fractaledmind fractaledmind force-pushed the ar-sqlite-virtual-columns branch 3 times, most recently from d2803e3 to 09b1be4 Compare November 2, 2023 08:58
@flavorjones flavorjones added ready PRs ready to merge and removed ready PRs ready to merge labels Dec 14, 2023
Copy link
Member

@byroot byroot left a comment

Choose a reason for hiding this comment

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

Two minor nitpick. I also rebased your branch to replace the merge commit.

Given how small the changes are I'll apply them myself.

@@ -20,6 +21,18 @@ def auto_incremented_by_db?
auto_increment? || rowid
end

def virtual?
@generated_type.present?
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@generated_type.present?
@generated_type

.present? isn't a "to_boolean" cast, it has fairly peculiar rules, and is rarely what you want.

Here we can just return @generated_type, or if you are adamant about returning a boolean, you can use !!@generated_type or !@generated_type.nil?

@@ -503,7 +511,8 @@ def has_default_function?(default_value, default)
# SQLite has an additional restriction on the ALTER TABLE statement
def invalid_alter_table_type?(type, options)
type.to_sym == :primary_key || options[:primary_key] ||
options[:null] == false && options[:default].nil?
options[:null] == false && options[:default].nil? ||
(type.to_sym == :virtual && options[:stored])
Copy link
Member

Choose a reason for hiding this comment

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

We should do the to_sym casting in add_column

Generated columns (both stored and dynamic) are supported since version 3.31.0 of SQLite.
This adds support for those to the SQLite3 adapter.

```ruby
create_table :users do |t|
  t.string :name
  t.virtual :name_upper, type: :string, as: 'UPPER(name)'
  t.virtual :name_lower, type: :string, as: 'LOWER(name)', stored: true
end
```
@byroot byroot merged commit 9064735 into rails:main Dec 14, 2023
4 checks passed
@fractaledmind fractaledmind deleted the ar-sqlite-virtual-columns branch December 16, 2023 00:13
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.

None yet

8 participants