CartoDB / Windshaft-cartodb

Windshaft tailored for CARTO
BSD 3-Clause "New" or "Revised" License
72 stars 58 forks source link

Fixes 1160: Prevent using cast column as part of __ctx_query #1161

Closed manmorjim closed 4 years ago

manmorjim commented 4 years ago

Prevent using cast column when it is date type as part of the alias __ctx_query for numeric histograms by keeping the original column.

Fixes #1160

Algunenano commented 4 years ago

Why not add a new column to the query with the casted date (__cdb_casted_date) and use that for the aggregation?

Also, we should add a test for this.

manmorjim commented 4 years ago

Why not add a new column to the query with the casted date (__cdb_casted_date) and use that for the aggregation?

:thinking: We have no control over the query of the ctx object in this class because it is given as parameter from an external call. However it could be addressed by modifying the line 139 with:

`
FROM
(
    SELECT * FROM (SELECT *, ${ctx.column} as __cdb_cast_date FROM (${ctx.query})) __ctx_query${extraTables} ${extraFilter}
)
`

And then in extraFilter:

extraFilter = `
  WHERE __ctx_query.__cdb_cast_date >= ${ctx.start}
    AND __ctx_query.__cdb_cast_date <= ${ctx.end}
`;

What do you think about this approach?

Also, we should add a test for this.

You're right. I'll mark this PR as draft until I add a test :+1:

Algunenano commented 4 years ago

thinking We have no control over the query of the ctx object in this class because it is given as parameter from an external call. However it could be addressed by modifying the line 139 with:

I'd modify https://github.com/CartoDB/Windshaft-cartodb/blob/fix_parsing_columns_histograms_1160/lib/models/dataview/histograms/numeric-histogram.js#L61 if we need it and set https://github.com/CartoDB/Windshaft-cartodb/blob/bb9d8f1385c817e925f35da43486a171c932c6c5/lib/models/dataview/histograms/numeric-histogram.js#L57 (column) to the new created column, that way there is no need to modify the query and we avoid the constant calls to the cast, right?

manmorjim commented 4 years ago

Thank you both! my first contribution to windshaft \o/ :tada: :rocket:

dgaubert commented 4 years ago

:shipit: