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

Fixing value name. Adding updates #335

Merged
merged 11 commits into from
Oct 3, 2017
Merged

Conversation

antoniocarlon
Copy link
Contributor

Fixing value name (per_sq_km)
Adding UPDATE queries

Closes #310

Copy link

@javitonino javitonino left a comment

Choose a reason for hiding this comment

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

Left a comment about the column name.

Aside from that, I would prefer to put the UPDATE query before the SELECT one. We prefer clients to use UPDATE (less hits to the DO database) so let's make it more visible if you agree.

@@ -192,16 +277,36 @@
"normalization": "predenominated"}]'
, 1, 1) meta
FROM data)
SELECT id, (data->0->>'value')::{{ col.type }} as per_sq_km
SELECT id, (data->0->>'value')::{{ col.type }} as raw_count

Choose a reason for hiding this comment

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

Uhm, I'm not sure if it's available in the RSTs, but the optimal solution would be to use the same column name we autogenerate for Builder, i.e: the suggested_name column of the GetMeta result.

If not, we can probably add it to the query to generate rsts, or replicate this logic in python:
https://github.com/CartoDB/observatory-extension/blob/develop/src/pg/sql/41_observatory_augmentation.sql#L249-L257

@antoniocarlon
Copy link
Contributor Author

Putting the UPDATE before the SELECT was my first idea because nobody needs to only see the data but do something with it. I leaved the SELECT first to be more conservative and trying not to surprise the user but it's clear that we agree that putting the UPDATEs first is more natural so I'm changing it.

Copy link

@javitonino javitonino left a comment

Choose a reason for hiding this comment

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

Code looks nice, let's see how it works!

@javitonino
Copy link

For reference, this is how this looks:

screenshot_20171002_102515

Some questions/suggestions:

  • For updating a table, the WITH data part should probably take data from a table, something like:
WITH data AS (
  SELECT cartodb_id as id, the_geom FROM <table_to_update>
)
  • We can probably replace <id_col> with cartodb_id.
  • Maybe we can replace <field_to_update> with the suggested name. This feels like it may be too much.
  • Is it possible to use the suggested name in all tabs? In Builder this works and shows names like male_pop_per_sq_km_2015

@antoniocarlon
Copy link
Contributor Author

antoniocarlon commented Oct 2, 2017

Answering your suggestions:

  1. Totally. The query with a fixed point doesn't make sense at all
  2. 99% of the time <id_col> will be cartodb_id. I'm changing it
  3. I prefer keeping the <field_to_update> placeholder. The suggested name is great for the SELECT as it gives you context about the result, but for some users it may be hard to infere that they need a column pop_18_25 (for example) in their table
  4. I think it's possible and it would be nice. I'll take a look

@javitonino
Copy link

Very nice!

screenshot_20171002_134212

One minor nitpick before letting this go. We should capitalize AS in SELECT cartodb_id as id, the_geom FROM <table_to_update>, for consistency. After that, this is a 🚀

@javitonino javitonino merged commit cf3f507 into master Oct 3, 2017
@javitonino javitonino deleted the 310-Fixing_value_naming branch October 3, 2017 09:08
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.

2 participants