From 2c334570c3b89848c1ad8745804ad51d0c1b023e Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Wed, 4 Jul 2018 17:39:13 +0200 Subject: [PATCH 1/6] run_tests.sh is a *bash* script --- run_tests.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/run_tests.sh b/run_tests.sh index b409db83..2b6a9523 100755 --- a/run_tests.sh +++ b/run_tests.sh @@ -1,4 +1,4 @@ -#!/bin/sh +#!/bin/bash OPT_CREATE_REDIS=yes # create the redis test environment OPT_CREATE_PGSQL=yes # create the PostgreSQL test environment From 6411556a9702a27eec550a47781d3eaea9a597d1 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Wed, 4 Jul 2018 18:33:02 +0200 Subject: [PATCH 2/6] Test for histogram bins beyond limits --- test/acceptance/dataviews/histogram.js | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/acceptance/dataviews/histogram.js b/test/acceptance/dataviews/histogram.js index 161fdef8..db558087 100644 --- a/test/acceptance/dataviews/histogram.js +++ b/test/acceptance/dataviews/histogram.js @@ -67,6 +67,27 @@ describe('histogram-dataview', function() { ] ); + it('should get bins with min >= start and max <= end', function(done) { + var params = { + bins: 3, + start: 50, + end: 500 + }; + + this.testClient = new TestClient(mapConfig, 1234); + this.testClient.getDataview('pop_max_histogram', params, function(err, dataview) { + assert.ok(!err, err); + + assert.ok(3 === dataview.bins_count, 'Unexpected bin count: ' + dataview.bins_count); + assert.ok(3 === dataview.bins.length, 'Unexpected number of bins: ' + dataview.bins.length); + dataview.bins.forEach(function(bin) { + assert.ok(bin.min >= params.start, 'bin min < start: ' + JSON.stringify(bin)); + assert.ok(bin.max <= params.end, 'bin max > end: ' + JSON.stringify(bin)); + }); + done(); + }); + }); + it('should get bin_width right when max > min in filter', function(done) { var params = { bins: 10, From d937ce498293798ef26f2f62aab261583e41c571 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 5 Jul 2018 11:56:26 +0200 Subject: [PATCH 3/6] Fix the min >= start and max <= end case (WIP) This fixes the 'should get bins with min >= start and max <= end' test case but probably breaks a number of other cases (those with no start and/or no end). --- lib/cartodb/models/dataview/histograms/numeric-histogram.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cartodb/models/dataview/histograms/numeric-histogram.js b/lib/cartodb/models/dataview/histograms/numeric-histogram.js index d3eedad9..11dbdf2b 100644 --- a/lib/cartodb/models/dataview/histograms/numeric-histogram.js +++ b/lib/cartodb/models/dataview/histograms/numeric-histogram.js @@ -135,7 +135,7 @@ SELECT END AS bin FROM ( - ${ctx.query} + SELECT * FROM (${ctx.query}) __ctx_query WHERE ${ctx.column} >= ${ctx.start} AND ${ctx.column} <= ${ctx.end} ) __cdb_filtered_source_query${extra_tables} GROUP BY 10${extra_groupby} ORDER BY 10;`; From a1807fd0c32ba44a69a4fcc75fe4f25f45da4d36 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 5 Jul 2018 12:39:26 +0200 Subject: [PATCH 4/6] A better solution to the start-end problem --- lib/cartodb/models/dataview/histograms/numeric-histogram.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/cartodb/models/dataview/histograms/numeric-histogram.js b/lib/cartodb/models/dataview/histograms/numeric-histogram.js index 11dbdf2b..2538fa54 100644 --- a/lib/cartodb/models/dataview/histograms/numeric-histogram.js +++ b/lib/cartodb/models/dataview/histograms/numeric-histogram.js @@ -99,6 +99,7 @@ module.exports = class NumericHistogram extends BaseHistogram { var extra_tables = ``; var extra_queries = ``; var extra_groupby = ``; + var extra_filter = ``; if (ctx.start >= ctx.end) { ctx.end = `__cdb_basics.__cdb_max_val`; @@ -106,6 +107,8 @@ module.exports = class NumericHistogram extends BaseHistogram { extra_groupby = `, __cdb_basics.__cdb_max_val, __cdb_basics.__cdb_min_val`; extra_tables = `, __cdb_basics`; extra_queries = `WITH ${irqQueryTpl(ctx)}`; + } else { + extra_filter = `WHERE ${ctx.column} >= ${ctx.start} AND ${ctx.column} <= ${ctx.end}`; } if (ctx.bins <= 0) { @@ -135,7 +138,7 @@ SELECT END AS bin FROM ( - SELECT * FROM (${ctx.query}) __ctx_query WHERE ${ctx.column} >= ${ctx.start} AND ${ctx.column} <= ${ctx.end} + SELECT * FROM (${ctx.query}) __ctx_query${extra_tables} ${extra_filter} ) __cdb_filtered_source_query${extra_tables} GROUP BY 10${extra_groupby} ORDER BY 10;`; From e247e45f9644272c134e64562de1f958a8237601 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 5 Jul 2018 17:21:35 +0200 Subject: [PATCH 5/6] Qualify columns and improve if/else style As suggested by Algunenano: qualify column names with the table/subquery/cte to avoid name clashing, and polish the code style a little. --- .../models/dataview/histograms/numeric-histogram.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/cartodb/models/dataview/histograms/numeric-histogram.js b/lib/cartodb/models/dataview/histograms/numeric-histogram.js index 2538fa54..f26a0f80 100644 --- a/lib/cartodb/models/dataview/histograms/numeric-histogram.js +++ b/lib/cartodb/models/dataview/histograms/numeric-histogram.js @@ -101,14 +101,17 @@ module.exports = class NumericHistogram extends BaseHistogram { var extra_groupby = ``; var extra_filter = ``; - if (ctx.start >= ctx.end) { + if (ctx.start < ctx.end) { + extra_filter = ` + WHERE __ctx_query.${ctx.column} >= ${ctx.start} + AND __ctx_query.${ctx.column} <= ${ctx.end} + `; + } else { ctx.end = `__cdb_basics.__cdb_max_val`; ctx.start = `__cdb_basics.__cdb_min_val`; extra_groupby = `, __cdb_basics.__cdb_max_val, __cdb_basics.__cdb_min_val`; extra_tables = `, __cdb_basics`; extra_queries = `WITH ${irqQueryTpl(ctx)}`; - } else { - extra_filter = `WHERE ${ctx.column} >= ${ctx.start} AND ${ctx.column} <= ${ctx.end}`; } if (ctx.bins <= 0) { From b59712ee10035b30922a16c17e2e61ba17539517 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 5 Jul 2018 17:24:29 +0200 Subject: [PATCH 6/6] Update NEWS.md with reference to fix --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index 468bd876..edcf57c5 100644 --- a/NEWS.md +++ b/NEWS.md @@ -45,6 +45,7 @@ Bug Fixes: - Static maps fails for unsupported formats - Handling errors extracting the column type on dataviews - Fix `meta.stats.estimatedFeatureCount` for aggregations and queries with tokens +- Fix numeric histogram bounds when `start` and `end` are specified (#991) - Static maps filters correctly if `layer` option is passed in the url. Announcements: