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

SQL updates for the planes table #1211

Merged
merged 3 commits into from
Jul 9, 2023

Conversation

chrisrosset
Copy link
Collaborator

This updates create.sql with the schema changes that were brought in through clean-duplicate-planes.sql in 36b3ee9.

Additionally, this updates load-data.sql to seed the DB with the data present in data/planes.dat.

Resolves #1210.

This updates `create.sql` with the schema changes that were brought in
through `clean-duplicate-planes.sql` in jpatokal@36b3ee9.

Additionally, this updates `load-data.sql` to seed the DB with the data
present in `data/planes.dat`.

Resolves jpatokal#1210.
@chrisrosset
Copy link
Collaborator Author

@jpatokal If you ever get a moment, perhaps you could share a schema dump (I'm not a MySQL expert but I think mysqldump --no-data flightdb2 would do the trick) of all the tables in the database?

I remember @reedy updated create.sql for countries, airports and airlines based on the schema you shared in #1137 (comment). I would gladly update all the other ones.

@chrisrosset
Copy link
Collaborator Author

I just noticed that #1148 did not include the frequency column from #1137 (comment). @reedy, was this on purpose?

@reedy
Copy link
Collaborator

reedy commented Jun 26, 2023

I just noticed that #1148 did not include the frequency column from #1137 (comment). @reedy, was this on purpose?

Not particularly. Probably mostly because it wasn't throwing an error...

I'd be curious if it's even used, as it seems to be (maybe) updated uses a script that pulls it in from VRS - https://github.com/jpatokal/openflights/blob/2f86a25d09c0721a2b37aa464bd7c474244fdeab/tools/import-vrs.py

@reedy
Copy link
Collaborator

reedy commented Jun 26, 2023

Though as per #1148 "create.sql: Integrate *-upgrade.sql", not "update to match live schema"...

https://github.com/jpatokal/openflights/blob/4ff8baf/sql/airlines-upgrade.sql doesn't have a frequency column.

@chrisrosset
Copy link
Collaborator Author

Though as per #1148 "create.sql: Integrate *-upgrade.sql", not "update to match live schema"...

Fair enough.

The clean-duplicate-*.sql scripts bring in more columns (iata, icao, and frequency):

https://github.com/jpatokal/openflights/blob/master/sql/clean-duplicate-airlines.sql#L28
https://github.com/jpatokal/openflights/blob/master/sql/clean-duplicate-planes.sql#L33

I'd be curious if it's even used, as it seems to be (maybe) updated uses a script that pulls it in from VRS - https://github.com/jpatokal/openflights/blob/2f86a25d09c0721a2b37aa464bd7c474244fdeab/tools/import-vrs.py

I don't know if they're kept up to date but they are read by the code and produce SQL errors when not present:

@reedy
Copy link
Collaborator

reedy commented Jun 26, 2023

import wasn't broken (at least, not with an SQL error related to this) on my dev wiki, even without (presumably) having the frequency column (I have just checked my live schema, and it's indeed not there, so not apparently added by a random patch I applied).

@chrisrosset
Copy link
Collaborator Author

chrisrosset commented Jun 26, 2023

import wasn't broken (at least, not with an SQL error related to this) on my dev wiki, even without (presumably) having the frequency column (I have just checked my live schema, and it's indeed not there, so not apparently added by a random patch I applied).

This code only runs when the import is trying to match names based on strings. I don't know if you saw #1197 (comment) but I'm guessing you were using an import file with ID columns populated by the export/backup.

Here's an example SQL warning:

PHP Warning: PDOStatement::execute(): SQLSTATE[42S22]: Column not found: 1054 Unknown column 'frequency' in 'order clause' in /var/www/openflights/php/import.php on line 172

@reedy
Copy link
Collaborator

reedy commented Jun 26, 2023

import wasn't broken (at least, not with an SQL error related to this) on my dev wiki, even without (presumably) having the frequency column (I have just checked my live schema, and it's indeed not there, so not apparently added by a random patch I applied).

This code only runs when the import is trying to match names based on strings. I don't know if you saw #1197 (comment) but I'm guessing you were using an import file with ID columns populated by the export/backup.

Anything running the "newer" code would indeed have the column populated.

I don't know from what version of openflights I would've originally exported the backup from.

But TLDR is that I was using my "live" data from openflights.org, into my dev install.

Makes it easy to compare changes...

I probably need to wipe the entire database anyway

@jpatokal
Copy link
Owner

@chrisrosset The frequency column is not used by the live website, IIRC it was there to try to sort out duplicates. Here's what the table looks like on the live system:

mysql> describe planes;
+-----------+-------------+------+-----+---------+----------------+
| Field     | Type        | Null | Key | Default | Extra          |
+-----------+-------------+------+-----+---------+----------------+
| name      | varchar(80) | YES  | UNI | NULL    |                |
| abbr      | text        | YES  |     | NULL    |                |
| speed     | double      | YES  |     | NULL    |                |
| plid      | int(11)     | NO   | PRI | NULL    | auto_increment |
| public    | char(1)     | YES  |     | N       |                |
| iata      | text        | YES  |     | NULL    |                |
| icao      | text        | YES  |     | NULL    |                |
| frequency | int(11)     | YES  |     | 0       |                |
+-----------+-------------+------+-----+---------+----------------+
8 rows in set (0.00 sec)

sql/create.sql Outdated
PRIMARY KEY (`plid`)
`iata` text default NULL,
`icao` text default NULL,
`frequency` int default 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like this should be int(11) to match the live schema

sql/create.sql Outdated
@@ -155,12 +157,16 @@ CREATE TABLE `locales` (

DROP TABLE IF EXISTS `planes`;
CREATE TABLE `planes` (
`name` text,
`name` varchar(80),
`abbr` text,
`speed` double default NULL,
`plid` int(11) NOT NULL auto_increment,
`public` char(1) default NULL,
Copy link
Collaborator

Choose a reason for hiding this comment

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

default 'n'?

@chrisrosset
Copy link
Collaborator Author

@jpatokal, many thanks for posting the schema.

@chrisrosset The frequency column is not used by the live website, IIRC it was there to try to sort out duplicates. Here's what the table looks like on the live system:

The columns are used! I've even already posted the links to where they're used above. :) I'll list them in more detail now:

  1. Airline frequency is used during import to sort airlines when matching by name.
    $sql = "select name,alias,alid from airlines
    where ((name like ? or alias like ?) and (iata != '')) or (name = ?) order by frequency
  2. Plane frequency is used for autocomplete. The code and the comment both reference frequency.
     // Autocompletion for plane types
     // First match against major types with IATA codes, then pad to max 6 by matching against frequency of use
    $query = $_POST['plane'];
    $name = "%$query%";
    $query = "(name LIKE :name OR iata LIKE :name) ";
    $sql = "(SELECT name,plid FROM planes WHERE " . $query . " AND iata IS NOT NULL ORDER BY name LIMIT 6) UNION " .
    "(SELECT name,plid FROM planes WHERE " . $query . " AND iata IS NULL ORDER BY frequency DESC LIMIT 6) LIMIT 6";

Based on them being used, I think we should include them in create.sql. I'll update this PR to match the live schemas posted here and in the previous PR.

It'd be nice to get a mysqldump --no-data to confirm we haven't missed anything. :)

@reedy
Copy link
Collaborator

reedy commented Jun 28, 2023

I'd be curious if any indexes are out of line too

@chrisrosset
Copy link
Collaborator Author

@reedy, anything else here? If we do get a full schema dump, I can do another pass in a separate PR. I'd like to get this merged because the Docker PR (#1206) has this changed merged in already.

Copy link
Collaborator

@reedy reedy left a comment

Choose a reason for hiding this comment

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

There's a little bit of change in the "functionality" of create.sql now... Not that I'm against it.

But I think

Create empty OpenFlights database schema
and maybe
<p>Once you have these set up, the <tt>sql/create.sql</tt> script can be used to generate the OpenFlights
database schema and the <tt>sql/load_data.sql</tt> script can be used to load in the standard
airport, airline and route databases:</p>
should reference that it is destructive (I know it already was, as it would drop each table)

sql/create.sql Outdated Show resolved Hide resolved
sql/create.sql Outdated Show resolved Hide resolved
@chrisrosset chrisrosset requested a review from reedy July 8, 2023 16:55
@chrisrosset
Copy link
Collaborator Author

and maybe

<p>Once you have these set up, the <tt>sql/create.sql</tt> script can be used to generate the OpenFlights
database schema and the <tt>sql/load_data.sql</tt> script can be used to load in the standard
airport, airline and route databases:</p>

should reference that it is destructive (I know it already was, as it would drop each table)

I think the widget stuff is all dead code. I opened #1224 to ask @jpatokal about removing it.

@reedy reedy merged commit 2c389c1 into jpatokal:master Jul 9, 2023
1 check passed
@chrisrosset chrisrosset deleted the planes-sql-changes branch July 9, 2023 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

load-data.sql does not load seed the planes table
3 participants