Add date_trunc function to mysql #17

andresgutgon · 2018-10-10T18:26:15Z

What is this?

Do a db_adapter to abstract date_trunc. This way I can implement in Ruby all the methods that generate the necessary methods to do date tuncation on SQL.

TODO

Test Mysql migration generator works on a Rails app
Pass all tests with DB='mysql' rake test
MySQL adapter with date_trunc

t27duck

Not sure if you were finished with this work, but wanted to point out the spelling issue in the readme.

t27duck · 2018-10-13T18:56:04Z

README.md

@@ -142,6 +142,8 @@ ActiveReporting::Configuration.setting = value

 `metric_lookup_class` - The name of a constant used to lookup prebuilt `Reporting::Metric` objects by name. The constant should define a class method called `#lookup` which can take a string or symbol of the metric name. (Default: `::Metric`)

+`db_adapter` - Each database has a diferent way of doing the same thing on SQL. This is an abstraction layer on each database. For example `PostGreSQL` have a native `date_trunc` while `MySQL` doesn't so we do the same in another way. Make sure to chosse your database's adapter (Default: `:sqlite3`). If you want MySQL put `:mysql2` and if you want PostGreSQL put `:postgresql`.


Misspelled "choose" here. Also, I've only known the database to be spelled "PostgreSQL". I'm not sure why the "G" is capitalized

Thanks for the corrections, I'm not an English speaker at all :)

andresgutgon · 2018-10-15T08:17:17Z

Yes, it's finish :). Do you see it ok?

…menting the same feature

masylum · 2018-10-15T13:31:13Z

README.md

@@ -235,7 +237,6 @@ end

 When creating a metric, ActiveReporting will recognize implicit hierarchies for this dimension. The hierarchies correspond to the [values](https://www.postgresql.org/docs/8.1/static/functions-datetime.html#FUNCTIONS-DATETIME-TRUNC) supported by PostgreSQL. (See example under the metric section, below.)

-*NOTE*: PRs welcomed to support this functionality in other databases.


I guess PRs are still welcome to support other databases 😉

LOL, true but the rest of databases are SQLitle which I guess nobody use in production. I doubt someone will do that

t27duck · 2018-10-15T21:09:58Z

lib/active_reporting/database_adapters/mysql_adapter.rb

+      # @return [String]
+      def date_trunc(interval, field)
+        clean_sql(
+          <<-SQL


The "squiggly heredoc" has been in Ruby since 2.3, I recommend using that instead.
https://ruby-doc.org/core-2.3.0/doc/syntax/literals_rdoc.html#label-Here+Documents

Also, I would say I personally am not concerned by multi-line SQL being generated, so I don't think #clean_sql is really necessary.

t27duck · 2018-10-15T21:11:28Z

lib/active_reporting/database_adapters/factory.rb

+      class << self
+        ADAPTERS = {
+          sqlite3: SqliteAdapter,
+          mysql2: MysqlAdapter,


Let's go with just sqlite and mysql here. The use of "3" and "2" in various Rails projects references the adaptor and version of the gem they use. In the case of this gem, we shouldn't care.

t27duck · 2018-10-15T21:14:27Z

README.md

@@ -142,6 +142,8 @@ ActiveReporting::Configuration.setting = value

 `metric_lookup_class` - The name of a constant used to lookup prebuilt `Reporting::Metric` objects by name. The constant should define a class method called `#lookup` which can take a string or symbol of the metric name. (Default: `::Metric`)

+`db_adapter` - Each database has a diferent way of doing the same thing on SQL. This is an abstraction layer on each database. For example `PostgreSQL` have a native `date_trunc` while `MySQL` doesn't so we do the same in another way. Make sure to choose your database's adapter (Default: `:sqlite3`). If you want MySQL put `:mysql2` and if you want PostGreSQL put `:postgresql`.


We should add a note here explaining that while it is encouraged to set this value, it is not required if the user does not with to implement database-specific features such as the date functions.

niborg · 2018-10-17T18:52:11Z

lib/active_reporting/database_adapters/mysql_adapter.rb

+            )
+          SQL
+        )
+      end


I've been playing around with different variants of replicating date_trunc, including this one. This approach is about 2x faster according to my primitive metrics, but it falls short in truly replicating date_trunc, mostly patently shown by the limit on supported datetime_hierarchies.

Take a look at this code and consider if it would be useful. It is ugly but may provide a better parallel in functionality.

niborg · 2018-10-17T18:55:22Z

test/active_reporting/report_test.rb

+        report = ActiveReporting::Report.new(metric)
+      end
+    end
+  end


I would propose that you might want to add some more comprehensive tests for the MySQL date_trunc adaptation. You can run into a lot of edge cases. See here for some examples you could include.

andresgutgon force-pushed the feature/trunc-date-mysql-function-implementation branch 2 times, most recently from 442a3c6 to c0ec8d5 Compare October 11, 2018 09:21

andresgutgon mentioned this pull request Oct 13, 2018

Add date_trunc function to MySQL andresgutgon/active_reporting#1

Open

andresgutgon force-pushed the feature/trunc-date-mysql-function-implementation branch 3 times, most recently from c797a4c to c513381 Compare October 13, 2018 09:27

t27duck reviewed Oct 13, 2018

View reviewed changes

andresgutgon force-pushed the feature/trunc-date-mysql-function-implementation branch from c513381 to 609de71 Compare October 15, 2018 07:53

andresgutgon force-pushed the feature/trunc-date-mysql-function-implementation branch from 37716cd to d43f34f Compare October 15, 2018 10:25

andresgutgon added 2 commits October 15, 2018 12:53

Implement database adapters to be able to have diferent ways of imple…

8d8a56e

…menting the same feature

Fixed bug introduced on PR: 16

4553066

andresgutgon force-pushed the feature/trunc-date-mysql-function-implementation branch from d43f34f to 4553066 Compare October 15, 2018 10:54

masylum reviewed Oct 15, 2018

View reviewed changes

geclos approved these changes Oct 15, 2018

View reviewed changes

t27duck reviewed Oct 15, 2018

View reviewed changes

niborg reviewed Oct 17, 2018

View reviewed changes

t27duck added the stale label Oct 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add date_trunc function to mysql #17

Add date_trunc function to mysql #17

andresgutgon commented Oct 10, 2018 •

edited

Loading

t27duck left a comment

t27duck Oct 13, 2018

andresgutgon Oct 14, 2018

andresgutgon commented Oct 15, 2018

masylum Oct 15, 2018

andresgutgon Oct 15, 2018 •

edited

Loading

t27duck Oct 15, 2018

t27duck Oct 15, 2018

t27duck Oct 15, 2018

niborg Oct 17, 2018

niborg Oct 17, 2018 •

edited

Loading

		@@ -142,6 +142,8 @@ ActiveReporting::Configuration.setting = value

		`metric_lookup_class` - The name of a constant used to lookup prebuilt `Reporting::Metric` objects by name. The constant should define a class method called `#lookup` which can take a string or symbol of the metric name. (Default: `::Metric`)

		`db_adapter` - Each database has a diferent way of doing the same thing on SQL. This is an abstraction layer on each database. For example `PostGreSQL` have a native `date_trunc` while `MySQL` doesn't so we do the same in another way. Make sure to chosse your database's adapter (Default: `:sqlite3`). If you want MySQL put `:mysql2` and if you want PostGreSQL put `:postgresql`.

		@@ -235,7 +237,6 @@ end

		When creating a metric, ActiveReporting will recognize implicit hierarchies for this dimension. The hierarchies correspond to the [values](https://www.postgresql.org/docs/8.1/static/functions-datetime.html#FUNCTIONS-DATETIME-TRUNC) supported by PostgreSQL. (See example under the metric section, below.)

		NOTE: PRs welcomed to support this functionality in other databases.

Add date_trunc function to mysql #17

Are you sure you want to change the base?

Add date_trunc function to mysql #17

Conversation

andresgutgon commented Oct 10, 2018 • edited Loading

What is this?

TODO

t27duck left a comment

Choose a reason for hiding this comment

t27duck Oct 13, 2018

Choose a reason for hiding this comment

andresgutgon Oct 14, 2018

Choose a reason for hiding this comment

andresgutgon commented Oct 15, 2018

masylum Oct 15, 2018

Choose a reason for hiding this comment

andresgutgon Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

t27duck Oct 15, 2018

Choose a reason for hiding this comment

t27duck Oct 15, 2018

Choose a reason for hiding this comment

t27duck Oct 15, 2018

Choose a reason for hiding this comment

niborg Oct 17, 2018

Choose a reason for hiding this comment

niborg Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

andresgutgon commented Oct 10, 2018 •

edited

Loading

andresgutgon Oct 15, 2018 •

edited

Loading

niborg Oct 17, 2018 •

edited

Loading