Schema Viewer Drawer

[emtwo]

This is a fresh PR with the code from #2990 rebased and linted.

It is ready for review now. This PR is the first of a series of PRs for schema enhancements. I will link the subsequent PRs here as they become available.

[1] Schema viewer drawer #3291 (this one)
[2] Schema admin configuration #3292
[3] Schema query samples #3293
[4] Data source descriptions #3401

[emtwo]

Note: the way that schema updates work now is through a periodic celery task that runs the queries to get column names and types etc. The results are stored in the new schema tables. Whenever the schema is fetched from the UI, it just directly queries the data in these tables.

Since the schema is set to refresh only every 30 min (https://github.com/getredash/redash/blob/master/redash/settings/__init__.py#L48), this is likely why the percy/redash visual error shows up.

We can either increase the frequency of schema update (quicker option, but not as good) or have a one-off schema refresh that is done on init so that the schema is available. I'll look into the latter.

[gabrieldutra]

I've created db-seed.js with this purpose 🤔, so this kind of dependency could be created by using npm run cypress db-seed prior to all tests and this would be avoided among then:

// create_query_spec.js - a few upper lines that were not shown
  const pg = {
      name: 'test',
      options: {
        dbname: 'postgres',
        host: 'postgres',
        password: 'postgres',
        user: 'postgres',
      },
      type: 'pg',
    };

LMK what you think haha

PS: if you are just testing, ignore this 😅

[emtwo]

@gabrieldutra I was in fact, just testing. Though I could use some help. I cannot reproduce this percy issue locally that shows up here. In fact, when I run the create_query_spec.js test on master locally, the DOM snapshots seem to be missing the shema data (included screenshot below) And on the other hand, the snapshot for this PR seems to show the schema locally (screenshot also included below).

Any idea what might be going on here or how I can reproduce this?

Screenshot from master

Screenshot from this PR:

[gabrieldutra]

I have faced some issues when I was doing changes in frontend and handling Cypress in development mode. The frontend code seems not to be shared within the docker container. I'll handle this further, but a quick fix to make it respond properly is to, after start cypress server just like you did, run npm run start for webpack development server and open cypress with CYPRESS_baseUrl=http://localhost:8080 npm run cypress open

[gabrieldutra]

Also, I don't know if it's related, but I noticed the Chinook data source is not showing schema info in the preview.

I'll try to reproduce this locally and give you some help with Percy anyway

Edit: the Chinook issue is probably related to the missing schema queue in one of the files (docker-compose.production.yml perhaps)

[arikfr]

I have faced some issues when I was doing changes in frontend and handling Cypress in development mode. The frontend code seems not to be shared within the docker container.

Does the Cypress Docker Compose configuration use VOLUMEs?

[gabrieldutra]

Cypress is using docker-compose.cypress.yml when in CI and the development docker-compose.yml when not.
Edit: Forgot to mention about the volumes haha, but the first one doesn't use and the second one does.

However it uses http://localhost:5000, which I guess doesn't use webpack to watch files, so frontend in this case only updates after a rebuild. The two options I see to make it friendlier to the developer would be either adding a npm run start to a frontend container in docker-compose.yml or adding this outside docker in cypress scripts.

[arikfr]

I think we should add to the instructions to run npm run build before running Cypress tests. Running npm in the container is not possible, because the container will not have Node (currently it does, but it's a temporary thing).

[gabrieldutra]

Got it haha, there are other docker-compose.yml files inside .circleci, just add this to them and Percy should do fine 🚀

Edit: Only docker-compose.cypress.yml affects the Percy screenshots
Edit2: There are probably other files where this may be necessary:

[emtwo]

Aweosme! Updating docker-compose.cypress.yml did the trick! I didn't realize cypress had its own yml file. Thank you for your help @gabrieldutra!

[gabrieldutra]

You're welcome @emtwo! 🙂

[emtwo]

I've rebased the PR again and the original percy issue is fixed. Note that currently percy is failing for an expected reason - there are 2 new tables added - column_metadata and table_metadata that show up in the schema viewer.

[gabrieldutra]

I've rebased the PR again and the original percy issue is fixed. Note that currently percy is failing for an expected reason - there are 2 new tables added - column_metadata and table_metadata that show up in the schema viewer.

Don't forget to add the schema queue in the other files (such as docker-compose.production.yml) as this could cause some bugs in the future 😁

[emtwo]

I've rebased the PR again and the original percy issue is fixed. Note that currently percy is failing for an expected reason - there are 2 new tables added - column_metadata and table_metadata that show up in the schema viewer.

Don't forget to add the schema queue in the other files (such as docker-compose.production.yml) as this could cause some bugs in the future 😁

I've added the schemas queue in a couple of other spots as you suggested. However, I was hesitant at first to add it since the schemas queue was used in the code prior to this PR but it did not appear in any of the docker files. So without knowing the use cases of all the docker files, it's hard to tell if it's required in them. Though, I'm sure it doesn't hurt to add them. Perhaps @arikfr might have more insight into this.

[arikfr]

I will do a review of all the Docker Compose files and add schemas where needed.

I do realize now that everyone who are using the AMIs we build, use a Docker Compose setup without this queue. Which means that: 1) this queue is growing in size, but nothing is processing it; 2) they don't get schema refreshes. 🤦‍♂️

[arikfr]

Change of plans: #3325.

[washort]

Is removing this redis caching of schema information intentional? Is there a performance impact?

[emtwo]

Thanks for pointing this out @washort!

It was intentional because from what I recall back in the Berlin work-week, I think @arikfr was saying he felt that using redis to store schema was a bit of a hack and he would prefer it stored in a table. Of course, we could be storing the data in tables and have additional caching for performance, but I felt this added complexity of maintaining both a cache and tables for the same data was perhaps not worth the performance gain.

I did a quick test on my machine and with 5 runs of the old vs. the new get_schema() function, the redis one averages 7.2ms per call and this one (from this pr) averages 44ms per call. It's a big relative difference, but 44ms isn't so bad. Though of course this could be worse in different scenarios, e.g. slower network/machine or more data. I suppose I will defer this decision to @arikfr

[washort]

Great, just curious.

[washort]

you'll need a migration to create these tables

[emtwo]

Ah! I had missed this, thank you!

[qlty-cloud-legacy]

Similar blocks of code found in 2 locations. Consider refactoring.

[qlty-cloud-legacy]

Similar blocks of code found in 2 locations. Consider refactoring.

[qlty-cloud-legacy]

Missing whitespace around operator

[arikfr]

Looks like there is no re-open button? I guess a new PR will be needed.

Btw, the migrations has a bug. The foreign key references of org_id needs to reference organizations.id and not organizations.id.id.

[jezdez]

Yeah, @emtwo would you mind opening a new PR please?