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

Change names of tables which don't follow ActiveRecord conventions #91

Open
jfly opened this issue Jun 3, 2015 · 14 comments
Open

Change names of tables which don't follow ActiveRecord conventions #91

jfly opened this issue Jun 3, 2015 · 14 comments
Labels
SERVICE: results Anything pertaining to how results are stored or displayed TEAM: wrt issues related to WRT TECH: database This issue requires interaction with a database

Comments

@jfly
Copy link
Contributor

jfly commented Jun 3, 2015

Generally, this isn't a problem, as we can override table names with ActiveRecord's table_name attribute. Rails appears to be pretty attached to its id column as an INTEGER, however. See http://stackoverflow.com/questions/1200568/using-rails-how-can-i-set-my-primary-key-to-not-be-an-integer-typed-column.

Potentially problematic tables:

/vagrant/WcaOnRails @development> grep '^ *`id`.[^i]' -B 1 ../secrets/wca_db/cubing.sql 
CREATE TABLE `Competitions` (
  `id` varchar(32) NOT NULL DEFAULT '',
--
CREATE TABLE `Continents` (
  `id` varchar(50) NOT NULL DEFAULT '',
--
CREATE TABLE `Countries` (
  `id` varchar(50) NOT NULL DEFAULT '',
--
CREATE TABLE `Events` (
  `id` varchar(6) NOT NULL DEFAULT '',
--
CREATE TABLE `Formats` (
  `id` char(1) NOT NULL DEFAULT '',
--
CREATE TABLE `InboxPersons` (
  `id` varchar(10) NOT NULL,
--
CREATE TABLE `InboxPersons_old` (
  `id` varchar(10) NOT NULL DEFAULT '',
--
CREATE TABLE `Persons` (
  `id` varchar(10) NOT NULL DEFAULT '',
--
CREATE TABLE `ResultsStatus` (
  `id` varchar(50) NOT NULL DEFAULT '',
--
CREATE TABLE `Rounds` (
  `id` char(1) NOT NULL DEFAULT '',
@fw42
Copy link

fw42 commented Jun 3, 2015

Why not just rename all the tables to follow the proper conventions?

@jfly
Copy link
Contributor Author

jfly commented Jun 3, 2015

To avoid touching the tons of existing PHP code that relies upon the tables the way they are right now.

jbmertens added a commit that referenced this issue Jun 28, 2015
Some cleanup on top of Luis's PR
@jfly jfly modified the milestone: Drop PHP Jun 29, 2015
@jfly
Copy link
Contributor Author

jfly commented Sep 30, 2015

Lets be sure to also add created_at and updated_at fields to each of these tables.

@jfly jfly mentioned this issue Dec 31, 2015
@jfly
Copy link
Contributor Author

jfly commented Jan 9, 2016

The CompetitionsMedia table has a type field that's making Rails sad: http://stackoverflow.com/a/27485416/1739415

@jfly
Copy link
Contributor Author

jfly commented Jan 15, 2016

From an internal discussion about getting rid of our static tables:

After all, it should be ok to move the "static" tables Continents, Countries, Events, Formats, Rounds away from the database. All of these do not require short-termed changes, and if a change is needed due to request by Board, WRC or results team, I am positive that it is goign to happen within a day or so, which should generally fulfil the purpose.

A minor request though - please add the following additional country "containers" so that we are set for the near future: "Multiple countries (North America)", "Multiple countries (Oceania)", "Multiple countries (Africa)", "Multiple countries"

-@SAuroux

@timhabermaas
Copy link
Contributor

After all, it should be ok to move the "static" tables Continents, Countries, Events, Formats, Rounds away from the database. All of these do not require short-termed changes, and if a change is needed due to request by Board, WRC or results team, I am positive that it is goign to happen within a day or so, which should generally fulfil the purpose.

I'm not a fan. For two reasons:
a) Performance: You can't join across RAM and an SQL server, so n+1 queries are bound to happen (and probably already exist). I haven't measured this yet, so this might or might not be a problem.
b) Consistency/Power: You need to be constantly aware about where data is stored. If you move some data to the RAM and some data to an SQL server there's no longer a common abstraction you can use to query it. E.g. competition.events.where(format: 'time') doesn't work.

Why was moving that data out of the _data_base even considered?

@jfly
Copy link
Contributor Author

jfly commented Jan 31, 2016

a) Performance: You can't join across RAM and an SQL server, so n+1 queries are bound to happen (and probably already exist). I haven't measured this yet, so this might or might not be a problem.

I don't think it's fair to call these n+1 queries. These will be constant time lookups in Ruby hashes. IMO, this is no different than formatting something like a Date. We don't store the string "February" in the database, instead we know that February is the second month of the year.

b) Consistency/Power: You need to be constantly aware about where data is stored. If you move some data to the RAM and some data to an SQL server there's no longer a common abstraction you can use to query it. E.g. competition.events.where(format: 'time') doesn't work.

Consistenty is a fair point, but not compelling enough for me to switch. Going back to my date analogy, if you want to query for all competitions that are in a month with exactly 30 days, how would you do it?

Why was moving that data out of the _data_base even considered?

It's a very small amount of information (easilly fits into hashmaps in RAM) that almost never changes. I think there is a huge benefit to storing this stuff in Git: it gives us a transparency that's even better than the database export, because you can see exactly when and why changes to events or countries were made. I believe this transparency outweighs any potential downsides.

@timhabermaas
Copy link
Contributor

I don't think it's fair to call these n+1 queries.

It depends on the way you look at the data. If I want to display the event names for all results, that's fine. But if I want to find the best 100 results for each Event, I have to take extra precautions to not mess this up.

To be honest: I don't get your Date analogy. a) Dates are not specific to our application, but universal. The countries and events we "support" are pretty specific to our application. b) "February" is related to internationalization which is usually not mixed with the pure data.

The Git history argument is a very strong one, though and it might indeed outweigh the downsides. I sometimes have day dreams about opening my terminal and being able to query for all past events like UserBecameDelegate and CompetitorMoved. This would be pretty awesome. ^^

@pedrosino
Copy link
Contributor

I'm just a noob, but was thinking about this and thought that countries and continents don't change that often.
The recent changes we made were "adding new countries" for the multi-country FM competitions.

I agree that keeping "our" stuff, like events and formats, in the code is good, but not sure about outside things.

@jfly
Copy link
Contributor Author

jfly commented Feb 2, 2016

Which countries the WCA chooses to recognize is a political thing that not everyone in the world agrees on. I still think it makes a lot of sense to keep them in version control.

@viroulep viroulep added the TEAM: wrt issues related to WRT label Nov 27, 2019
@dunkOnIT dunkOnIT removed this from the Drop PHP milestone Oct 11, 2022
@dunkOnIT dunkOnIT added this to the Migrate Away from PHP milestone Oct 31, 2022
@dunkOnIT
Copy link
Contributor

@gregorbg is this still relevant with PHP gone?

@FinnIckler
Copy link
Member

We fix this in #8077 I think

@gregorbg
Copy link
Member

gregorbg commented Sep 1, 2023

@gregorbg is this still relevant with PHP gone?

This is especially relevant now, precisely because PHP is gone.
The old column naming schema was there because PHP relied on it. Now that PHP is gone, we are finally free to change it.

@dunkOnIT dunkOnIT changed the title Results php tables names don't follow ActiveRecord conventions Change names of tables which don't follow ActiveRecord conventions Sep 4, 2023
@dunkOnIT dunkOnIT added the SERVICE: results Anything pertaining to how results are stored or displayed label Sep 4, 2023
@dunkOnIT
Copy link
Contributor

dunkOnIT commented Sep 4, 2023

Awesome thanks! Changed name to be more grok-able for my brain - let me know if I'm on the wrong track

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SERVICE: results Anything pertaining to how results are stored or displayed TEAM: wrt issues related to WRT TECH: database This issue requires interaction with a database
Projects
Status: No status
Development

No branches or pull requests

8 participants