rails icon indicating copy to clipboard operation
rails copied to clipboard

Clarify Common Table Expression (CTE) support

Open fig opened this issue 1 year ago • 4 comments

Summary

[change to documentation only] I've attempted to provide some more comprehensive documentation by listing the databases that do support CTEs. In creating the list, I've included those databases whose adapter responds truthy to #supports_common_table_expressions?

This is intended to enhance #45586

For reference, these are renderings of the documentation before and after this commit.

Before

before

After

after

fig avatar Jul 17 '22 19:07 fig

These method definitions provides the enogh information. IMO, So I do not thik we need to provide the version information as comments.

https://github.com/rails/rails/blob/9994d38bc6c1a8662e510d26c0e9ca8c24ab1189/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb#L125-L131

https://github.com/rails/rails/blob/9994d38bc6c1a8662e510d26c0e9ca8c24ab1189/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L418-L420

https://github.com/rails/rails/blob/9994d38bc6c1a8662e510d26c0e9ca8c24ab1189/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb#L158-L160

yahonda avatar Jul 17 '22 23:07 yahonda

I do not thik we need to provide the version information as comments.

The comment gets rendered in api.rubyonrails.com (and other sites that lift the API docs), right on the method you'll be reading about to understand how to use CTEs.

The implementation in each connection adapter is located pretty deep into ActiveRecord's implementation, and likely not as accessible to newcomers to the framework as the API docs.

Since there is no risk of this comment falling out of date (i.e. the minimum version that supports CTEs on a given engine is not going to change), what is the harm in keeping the comments? 🤔

foca avatar Jul 20 '22 04:07 foca

Thank you, @foca . Additionally, my contribution merely enhances the documentation that is already there. The current documentation deals only with MySQL. My proposal expands that documentation to list all supported databases and their respective minimum versions.

fig avatar Jul 20 '22 04:07 fig

Thanks for the comment.

As far as I understand, the comment previously added via https://github.com/rails/rails/commit/3c7e190ee8e3474e4ddf00ae0ac3a7c21d6f3d41 to clarify because Rails CI used to run MySQL 8.0 which supports CTE and did not cover the MySQL 5.7. This document does enough job why this commit is needed.

By clicking Source: show or on GitHub link, readers can see which versions of databases support common table expressions easily.

https://edgeapi.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/AbstractAdapter.html#method-i-supports_common_table_expressions-3F

I prefer not to add comments supported database versions for every supports_foobar? methods in different locations, method definitions and documents becaue the current https://edgeapi.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/AbstractAdapter.html#method-i-supports_common_table_expressions-3F is clear.

yahonda avatar Jul 25 '22 12:07 yahonda