Cartodb: Review SQL errors for `select existing_column from...`

[Jesus89]

Context

This ticket is related to https://github.com/CartoDB/cartodb/pull/13631.

Steps to Reproduce

1. Open the SQL console
2. Enter: SELECT cartodb_id FROM or SELECT 1 as X FROM
3. Check the SQL Error

Current Result

Now it returns an Error in SQL query: ... error.

Expected result

They should return a The column does not exist warning. This may be related with the Maps API responses.

[rubenmoya]

The API is returning:

{
  "error": [
    "column \"the_geom_webmercator\" does not exist"
  ]
}

so that's what we're showing, should the API return something different?

[Jesus89]

This should show a warning.

For example as the SELECT the_geom FROM warning, without the Fix them windows.

[rubenmoya]

When using SELECT the_geom the query is applied, and fails later, when using SELECT cartodb_id the query fails before being applied.

Basically, one of them is failing at Builder level, and the other one at Cartodb.js level, we're handling the errors differently, also, we're using different Notifiers...

[Jesus89]

Yes, that is true.

Please talk to @ramiroaznar and the @CartoDB/solutions team to define the correct behavior for this situation.

[ramiroaznar]

When selecting cartodb_id or any thematic column should not fail but gives you a notification.

[ivanmalagon]

We're about to deploy in the next days a refactor that affects the state of layers (empty, no geom, geocodable...)

Please freeze this issue until that is deployed to avoid conflicts or provoking a regression.

Particularly with https://github.com/CartoDB/cartodb/issues/13642#issuecomment-372261001, we have a conflict.

In this ticket https://github.com/CartoDB/cartodb/issues/13478 the UI state for any layer that has no geometry but it's because an SQL or analysis is one where we have to show a particular text. This is not yet released so it's better to wait until this is merged https://github.com/CartoDB/cartodb/pull/13628

[ivanmalagon]

Blocked until https://github.com/CartoDB/cartodb/pull/13628 is merged.

[ivanmalagon]

Unblocked. This issue can continue its path.

[ramiroaznar]

[ivanmalagon]

I honestly don't find any problem in current behaviour.

I mean, you are selecting something that can't be digested by our platform because it lacks needed columns.

SELECT cartodb_id FROM shows ->

SELECT the_geom FROM shows ->

I get the point that they are two similar problems with different ways of communicating the users but in both cases there's an error that the user must correct and the app tells them.

I don't find this ticket worthy. I must dig into the code, change the way errors are now diggested with some probability of breaking something... and I don't feel it like broken.

What's your take, @ramiroaznar ?

[ramiroaznar]

Let me explain. Maybe the errors should be kept as they are. These errors tell you that there are missing fields that you need to render the map. But the thing is that when selecting fields different than the geometries such as cartodb_id and/or thematic data, the DATA panel should show you the fields.

Because with these fields you can play with analysises for instance...

[stevelewis99]

I don't want to hijack or speak out of turn, but I hope this is useful idea at least - it would be awesome if some of text from the error message was a clickable link ... to a page in our documentation, which explains the required columns and how the user can fix it etc.

New users may not have a clue what they are supposed to do to fix it, and if we can steer them in the right direction and make them feel less stupid, I think we would get better experience in these cases?

[ramiroaznar]

@stevelewis99 BUILDER messaging that's something we need to give a lot of love, already discussed with @alonsogarciapablo

[ivanmalagon]

Understood @ramiroaznar

Taking a look but I'm 99% sure that we can't show the data fields. We retrieve them from SQL API and the calls to it (since the SQL is badly formed) return error. So no schema is returned and we can't assure what fields the table has.

Let me check if I can do something about it.

[ivanmalagon]

So... I'll leave it here for the next responder.

The issue is

When a user wants to filter only certain columns, they must show up in the list of columns.

So if I edit the SQL and apply...

... even if the layer is wrong (we can't visualize the points because there are no geom columns, users expect those columns to appear in the list.

It's a bit tricky since the layer is wrong and that won't have the same behaviour for all combinations.

I leave ongoing work on the branch https://github.com/CartoDB/cartodb/tree/13642-sql-messages

Bear in mind the acceptance of previous tasks related to this one:
https://github.com/CartoDB/cartodb/pull/13631
https://github.com/CartoDB/cartodb/pull/13628 (Acceptance of user flow)

[rjimenezda]

If the user does not select any geometry, the query is not being saved at all. There's server side validation for this, and it will return a 400 error if there's no geometry.

If we hack the frontend to ignore the response and save the query anyways, the map breaks.

If the user selects geometry, but does not select cartodb_id, the map breaks, and widgets won't work (they rely on cartodb_id). Even if we show the columns on the data tab, and let the user add them, they won't work, because apparently everything relies on cartodb_id. The query is persisted in this case because cartodb_id is not checked apparently.

So I don't think it's too bad that the user gets prompted with an error if they are not providing the correct columns. We could definitely improve the usability though.

If the user provides all the required columns and extra ones, they should show up there.

[ramiroaznar]

If the user does not select any geometry, the query is not being saved at all. There's server side validation for this, and it will return a 400 error if there's no geometry.

You are right...

So I don't think it's too bad that the user gets prompted with an error if they are not providing the correct columns. We could definitely improve the usability though.

What do you suggest?

[rjimenezda]

For me it's obvious that a column is missing, because the error is shown below. Although the first time that this happens to you, you have to go through several errors until you have a working query.

It might be enough providing a link to some docs stating what columns are mandatory, and maybe improving the copy? Like stating that the_geom_webmercator and cartodb_id are mandatory. I think the_geom is not 100% required as long as there's the_geom_webmercator.

We could handhold the user even more, and automagically select cartodb_id and geometry if they don't, but I'm sure this has not been done for a very good reason.

[stevelewis99]

automagically select cartodb_id and geometry if they don't

🙏 😄

but for sure, the documentation idea sounds like a big improvement even, as it's not that obvious unless _you know_

[ramiroaznar]

I completely disagree about automatically selecting everything. Let's go for pointing to the docs. But I think if the query is not saved, this ticket has lost its initial goal.

[ramiroaznar]

And...

I think the_geom is not 100% required as long as there's the_geom_webmercator.

AFAIK in BUILDER, the_geom is mandatory to be used in analysis and widgets.

[ramiroaznar]

@Jesus89 @ivanmalagon After talking to @rjimenezda, we are going to close this issue. Better errors should be given to explain to the users how to remake their SQL queries. But this could be added to bigger project as started here.