From 8ebd22bc26702aa5c21430f5c0e64f5e5a07810f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 9 Jul 2018 14:09:42 +0200 Subject: [PATCH 01/31] Fixes error message check --- test/integration/test_street_functions.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index d1fdbd2..12f5112 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -30,13 +30,14 @@ class TestStreetFunctions(TestStreetFunctionsSetUp): assert_not_equal(geometry['geometry'], None) def test_if_select_with_street_without_api_key_raise_error(self): + table = self.env_variables['table_name'] query = "SELECT cdb_geocode_street_point(street) " \ - "as geometry FROM {0} LIMIT 1".format( - self.env_variables['table_name']) + "as geometry FROM {0} LIMIT 1".format(table) try: IntegrationTestHelper.execute_query(self.sql_api_url, query) except Exception as e: - assert_equal(e.message[0], "The api_key must be provided") + assert_equal(e.message[0], + "permission denied for relation {}".format(table)) class TestBulkStreetFunctions(TestStreetFunctionsSetUp): From 8e430ce1c102f1911ea80caa6a67c28c55b18f1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 9 Jul 2018 15:28:28 +0200 Subject: [PATCH 02/31] Google geocoder works better concatenating all components --- .../cartodb_services/geocoder.py | 4 ++++ .../cartodb_services/google/geocoder.py | 8 +++++--- test/integration/test_street_functions.py | 18 +++++++++++++++++- 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index 6a14594..6c2ce43 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -6,6 +6,10 @@ from collections import namedtuple import json +def compose_address(street, city=None, state=None, country=None): + return ', '.join(filter(None, [street, city, state, country])) + + def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, orgname, searches): plpy.execute("SELECT cdb_dataservices_server._get_logger_config()") logger_config = GD["logger_config"] diff --git a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py index 1cc61c9..956d09e 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py @@ -6,6 +6,7 @@ from urlparse import parse_qs from exceptions import MalformedResult from cartodb_services import StreetPointBulkGeocoder +from cartodb_services.geocoder import compose_address from cartodb_services.google.exceptions import InvalidGoogleCredentials from client_factory import GoogleMapsClientFactory @@ -36,8 +37,9 @@ class GoogleMapsGeocoder(StreetPointBulkGeocoder): def geocode(self, searchtext, city=None, state=None, country=None): try: + address = compose_address(searchtext, city, state, country) opt_params = self._build_optional_parameters(city, state, country) - results = self.geocoder.geocode(address=searchtext, + results = self.geocoder.geocode(address=address, components=opt_params) if results: return self._extract_lng_lat_from_result(results[0]) @@ -50,10 +52,10 @@ class GoogleMapsGeocoder(StreetPointBulkGeocoder): bulk_results = {} pool = Pool(processes=self.PARALLEL_PROCESSES) for search in searches: - (search_id, address, city, state, country) = search + (search_id, street, city, state, country) = search opt_params = self._build_optional_parameters(city, state, country) # Geocoding works better if components are also inside the address - address = ', '.join(filter(None, [address, city, state, country])) + address = compose_address(street, city, state, country) if address: self._logger.debug('async geocoding --> {} {}'.format(address.encode('utf-8'), opt_params)) result = pool.apply_async(async_geocoder, diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index 12f5112..d939de1 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -18,11 +18,18 @@ class TestStreetFunctionsSetUp(TestCase): self.env_variables['api_key'] ) + def _run_authenticated(self, query): + authenticated_query = "{}&api_key={}".format(query, + self.env_variables[ + 'api_key']) + return IntegrationTestHelper.execute_query_raw(self.sql_api_url, + authenticated_query) + class TestStreetFunctions(TestStreetFunctionsSetUp): def test_if_select_with_street_point_is_ok(self): - query = "SELECT cdb_geocode_street_point(street) " \ + query = "SELECT cdb_dataservices_client.cdb_geocode_street_point(street) " \ "as geometry FROM {0} LIMIT 1&api_key={1}".format( self.env_variables['table_name'], self.env_variables['api_key']) @@ -39,6 +46,15 @@ class TestStreetFunctions(TestStreetFunctionsSetUp): assert_equal(e.message[0], "permission denied for relation {}".format(table)) + def test_component_aggregation(self): + query = "select st_x(the_geom), st_y(the_geom) from (" \ + "select cdb_dataservices_client.cdb_geocode_street_point( " \ + "'Plaza España 1', 'Barcelona', null, 'Spain') as the_geom) _x" + response = self._run_authenticated(query) + row = response['rows'][0] + x_y = [row['st_x'], row['st_y']] + # Wrong coordinates (Plaza España, Madrid): [-3.7138975, 40.4256762] + assert_close_enough(x_y, [2.1482563, 41.375485]) class TestBulkStreetFunctions(TestStreetFunctionsSetUp): provider = None From 1ffe3658fea0bb45734d78e44d0a4d666ae67815 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 10 Jul 2018 12:28:16 +0200 Subject: [PATCH 03/31] Revert "maxresults depends on batch" This reverts commit bf8b76b5fe4a079fd4405bfd615e493bca322e7f. --- .../cartodb_services/cartodb_services/here/bulk_geocoder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py index d652891..9a688f4 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py @@ -24,7 +24,7 @@ class HereMapsBulkGeocoder(HereMapsGeocoder, StreetPointBulkGeocoder): MIN_BATCHED_SEARCH = 100 # Under this, serial will be used JOB_FINAL_STATES = ['completed', 'cancelled', 'deleted', 'failed'] - def __init__(self, app_id, app_code, logger, service_params=None, maxresults=MAX_BATCH_SIZE): + def __init__(self, app_id, app_code, logger, service_params=None, maxresults=HereMapsGeocoder.DEFAULT_MAXRESULTS): HereMapsGeocoder.__init__(self, app_id, app_code, logger, service_params, maxresults) self.session = requests.Session() self.session.mount(self.BATCH_URL, From f6b7c13dde9010e54fc40959105b8e539edd46bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 10 Jul 2018 13:25:40 +0200 Subject: [PATCH 04/31] GoogleMapsBulkGeocoder extraction --- .../cdb_dataservices_server--0.32.0.sql | 4 +- .../extension/sql/21_bulk_geocode_street.sql | 4 +- .../cartodb_services/google/__init__.py | 1 + .../cartodb_services/google/bulk_geocoder.py | 56 +++++++++++++++++++ .../cartodb_services/google/geocoder.py | 54 +----------------- test/integration/test_street_functions.py | 2 +- 6 files changed, 63 insertions(+), 58 deletions(-) create mode 100644 server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py diff --git a/server/extension/cdb_dataservices_server--0.32.0.sql b/server/extension/cdb_dataservices_server--0.32.0.sql index 8543e7f..a013f3c 100644 --- a/server/extension/cdb_dataservices_server--0.32.0.sql +++ b/server/extension/cdb_dataservices_server--0.32.0.sql @@ -2387,10 +2387,10 @@ CREATE OR REPLACE FUNCTION cdb_dataservices_server._cdb_bulk_google_geocode_stre RETURNS SETOF cdb_dataservices_server.geocoding AS $$ from cartodb_services import run_street_point_geocoder from cartodb_services.tools import LegacyServiceManager - from cartodb_services.google import GoogleMapsGeocoder + from cartodb_services.google import GoogleMapsBulkGeocoder service_manager = LegacyServiceManager('geocoder', username, orgname, GD) - geocoder = GoogleMapsGeocoder(service_manager.config.google_client_id, service_manager.config.google_api_key, service_manager.logger) + geocoder = GoogleMapsBulkGeocoder(service_manager.config.google_client_id, service_manager.config.google_api_key, service_manager.logger) return run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, orgname, searches) $$ LANGUAGE plpythonu STABLE PARALLEL RESTRICTED; diff --git a/server/extension/sql/21_bulk_geocode_street.sql b/server/extension/sql/21_bulk_geocode_street.sql index adee498..309d323 100644 --- a/server/extension/sql/21_bulk_geocode_street.sql +++ b/server/extension/sql/21_bulk_geocode_street.sql @@ -44,10 +44,10 @@ CREATE OR REPLACE FUNCTION cdb_dataservices_server._cdb_bulk_google_geocode_stre RETURNS SETOF cdb_dataservices_server.geocoding AS $$ from cartodb_services import run_street_point_geocoder from cartodb_services.tools import LegacyServiceManager - from cartodb_services.google import GoogleMapsGeocoder + from cartodb_services.google import GoogleMapsBulkGeocoder service_manager = LegacyServiceManager('geocoder', username, orgname, GD) - geocoder = GoogleMapsGeocoder(service_manager.config.google_client_id, service_manager.config.google_api_key, service_manager.logger) + geocoder = GoogleMapsBulkGeocoder(service_manager.config.google_client_id, service_manager.config.google_api_key, service_manager.logger) return run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, orgname, searches) $$ LANGUAGE plpythonu STABLE PARALLEL RESTRICTED; diff --git a/server/lib/python/cartodb_services/cartodb_services/google/__init__.py b/server/lib/python/cartodb_services/cartodb_services/google/__init__.py index 4e8cd6b..7570e31 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/__init__.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/__init__.py @@ -1 +1,2 @@ from geocoder import GoogleMapsGeocoder +from bulk_geocoder import GoogleMapsBulkGeocoder diff --git a/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py new file mode 100644 index 0000000..aa174b8 --- /dev/null +++ b/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py @@ -0,0 +1,56 @@ +from multiprocessing import Pool +from exceptions import MalformedResult +from cartodb_services import StreetPointBulkGeocoder +from cartodb_services.geocoder import compose_address +from cartodb_services.google import GoogleMapsGeocoder + + +def async_geocoder(geocoder, address, components): + results = geocoder.geocode(address=address, components=components) + return results if results else [] + + +class GoogleMapsBulkGeocoder(GoogleMapsGeocoder, StreetPointBulkGeocoder): + """A Google Maps Geocoder wrapper for python""" + PARALLEL_PROCESSES = 13 + + def __init__(self, client_id, client_secret, logger): + GoogleMapsGeocoder.__init__(self, client_id, client_secret, logger) + + def _bulk_geocode(self, searches): + bulk_results = {} + pool = Pool(processes=self.PARALLEL_PROCESSES) + for search in searches: + (search_id, street, city, state, country) = search + opt_params = self._build_optional_parameters(city, state, country) + # Geocoding works better if components are also inside the address + address = compose_address(street, city, state, country) + if address: + self._logger.debug('async geocoding --> {} {}'.format(address.encode('utf-8'), opt_params)) + result = pool.apply_async(async_geocoder, + (self.geocoder, address, opt_params)) + else: + result = [] + bulk_results[search_id] = result + pool.close() + pool.join() + + try: + results = [] + for search_id, bulk_result in bulk_results.items(): + try: + result = bulk_result.get() + except Exception as e: + self._logger.error('Error at Google async_geocoder', e) + result = [] + + lng_lat = self._extract_lng_lat_from_result(result[0]) if result else [] + results.append((search_id, lng_lat, [])) + return results + except KeyError as e: + self._logger.error('KeyError error', exception=e) + raise MalformedResult() + except Exception as e: + self._logger.error('General error', exception=e) + raise e + diff --git a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py index 956d09e..aa8877c 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py @@ -1,30 +1,15 @@ #!/usr/local/bin/python # -*- coding: utf-8 -*- -import googlemaps from urlparse import parse_qs from exceptions import MalformedResult -from cartodb_services import StreetPointBulkGeocoder from cartodb_services.geocoder import compose_address from cartodb_services.google.exceptions import InvalidGoogleCredentials from client_factory import GoogleMapsClientFactory -from multiprocessing import Pool, TimeoutError -import time, random - -def async_geocoder(geocoder, address, components): - # TODO: clean this and previous import - # time.sleep(.3 + random.random()) - # return [{ 'geometry': { 'location': { 'lng': 1, 'lat': 2 } } }] - - results = geocoder.geocode(address=address, components=components) - return results if results else [] - -class GoogleMapsGeocoder(StreetPointBulkGeocoder): - """A Google Maps Geocoder wrapper for python""" - PARALLEL_PROCESSES = 13 +class GoogleMapsGeocoder(): def __init__(self, client_id, client_secret, logger): if client_id is None: @@ -48,43 +33,6 @@ class GoogleMapsGeocoder(StreetPointBulkGeocoder): except KeyError: raise MalformedResult() - def _bulk_geocode(self, searches): - bulk_results = {} - pool = Pool(processes=self.PARALLEL_PROCESSES) - for search in searches: - (search_id, street, city, state, country) = search - opt_params = self._build_optional_parameters(city, state, country) - # Geocoding works better if components are also inside the address - address = compose_address(street, city, state, country) - if address: - self._logger.debug('async geocoding --> {} {}'.format(address.encode('utf-8'), opt_params)) - result = pool.apply_async(async_geocoder, - (self.geocoder, address, opt_params)) - else: - result = [] - bulk_results[search_id] = result - pool.close() - pool.join() - - try: - results = [] - for search_id, bulk_result in bulk_results.items(): - try: - result = bulk_result.get() - except Exception as e: - self._logger.error('Error at Google async_geocoder', e) - result = [] - - lng_lat = self._extract_lng_lat_from_result(result[0]) if result else [] - results.append((search_id, lng_lat, [])) - return results - except KeyError as e: - self._logger.error('KeyError error', exception=e) - raise MalformedResult() - except Exception as e: - self._logger.error('General error', exception=e) - raise e - def _extract_lng_lat_from_result(self, result): location = result['geometry']['location'] longitude = location['lng'] diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index d939de1..c35e1d5 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -38,7 +38,7 @@ class TestStreetFunctions(TestStreetFunctionsSetUp): def test_if_select_with_street_without_api_key_raise_error(self): table = self.env_variables['table_name'] - query = "SELECT cdb_geocode_street_point(street) " \ + query = "SELECT cdb_dataservices_client.cdb_geocode_street_point(street) " \ "as geometry FROM {0} LIMIT 1".format(table) try: IntegrationTestHelper.execute_query(self.sql_api_url, query) From a6c5c211312d4a5d2d60db27ba3f6f81c6e9fe4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 10 Jul 2018 13:45:01 +0200 Subject: [PATCH 05/31] Serial geocode for Google bulk --- .../cartodb_services/google/bulk_geocoder.py | 41 +++++++++++++++---- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py index aa174b8..206fa54 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py @@ -12,32 +12,59 @@ def async_geocoder(geocoder, address, components): class GoogleMapsBulkGeocoder(GoogleMapsGeocoder, StreetPointBulkGeocoder): """A Google Maps Geocoder wrapper for python""" + MAX_BATCH_SIZE = 1000 + MIN_BATCHED_SEARCH = 2 # Batched is a parallelization PARALLEL_PROCESSES = 13 def __init__(self, client_id, client_secret, logger): GoogleMapsGeocoder.__init__(self, client_id, client_secret, logger) def _bulk_geocode(self, searches): + if len(searches) > self.MAX_BATCH_SIZE: + raise Exception("Batch size can't be larger than {}".format(self.MAX_BATCH_SIZE)) + if self._should_use_batch(searches): + self._logger.debug('--> Batch geocode') + return self._batch_geocode(searches) + else: + self._logger.debug('--> Serial geocode') + return self._serial_geocode(searches) + + def _should_use_batch(self, searches): + return len(searches) >= self.MIN_BATCHED_SEARCH + + def _serial_geocode(self, searches): + results = [] + for search in searches: + (cartodb_id, street, city, state, country) = search + address = compose_address(street, city, state, country) + components = self._build_optional_parameters(city, state, country) + result = self.geocoder.geocode(address=address, components=components) + lng_lat = self._extract_lng_lat_from_result(result[0]) if result else [] + self._logger.debug('--> lng_lat: {}'.format(lng_lat)) + results.append((cartodb_id, lng_lat, [])) + return results + + def _batch_geocode(self, searches): bulk_results = {} pool = Pool(processes=self.PARALLEL_PROCESSES) for search in searches: - (search_id, street, city, state, country) = search - opt_params = self._build_optional_parameters(city, state, country) + (cartodb_id, street, city, state, country) = search + components = self._build_optional_parameters(city, state, country) # Geocoding works better if components are also inside the address address = compose_address(street, city, state, country) if address: - self._logger.debug('async geocoding --> {} {}'.format(address.encode('utf-8'), opt_params)) + self._logger.debug('async geocoding --> {} {}'.format(address.encode('utf-8'), components)) result = pool.apply_async(async_geocoder, - (self.geocoder, address, opt_params)) + (self.geocoder, address, components)) else: result = [] - bulk_results[search_id] = result + bulk_results[cartodb_id] = result pool.close() pool.join() try: results = [] - for search_id, bulk_result in bulk_results.items(): + for cartodb_id, bulk_result in bulk_results.items(): try: result = bulk_result.get() except Exception as e: @@ -45,7 +72,7 @@ class GoogleMapsBulkGeocoder(GoogleMapsGeocoder, StreetPointBulkGeocoder): result = [] lng_lat = self._extract_lng_lat_from_result(result[0]) if result else [] - results.append((search_id, lng_lat, [])) + results.append((cartodb_id, lng_lat, [])) return results except KeyError as e: self._logger.error('KeyError error', exception=e) From 286a75fa8e43f64b7db57083ba82c22e997ee0e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 10 Jul 2018 15:17:14 +0200 Subject: [PATCH 06/31] _bulk_geocode logic extraction --- client/cdb_dataservices_client--0.25.0.sql | 2 +- client/sql/21_bulk_geocoding_functions.sql | 2 +- .../cartodb_services/geocoder.py | 31 +++- .../cartodb_services/google/bulk_geocoder.py | 10 -- .../cartodb_services/here/bulk_geocoder.py | 14 +- .../cartodb_services/mapbox/bulk_geocoder.py | 10 -- .../cartodb_services/tomtom/bulk_geocoder.py | 14 +- test/integration/test_street_functions.py | 155 ++++++++++-------- 8 files changed, 115 insertions(+), 123 deletions(-) diff --git a/client/cdb_dataservices_client--0.25.0.sql b/client/cdb_dataservices_client--0.25.0.sql index e810dbc..d21bb5f 100644 --- a/client/cdb_dataservices_client--0.25.0.sql +++ b/client/cdb_dataservices_client--0.25.0.sql @@ -1987,7 +1987,7 @@ CREATE OR REPLACE FUNCTION cdb_dataservices_client._DST_DisconnectUserTable( TARGET cdb_dataservices_server._DST_DisconnectUserTable; $$ LANGUAGE plproxy VOLATILE PARALLEL UNSAFE; CREATE OR REPLACE FUNCTION cdb_dataservices_client.cdb_bulk_geocode_street_point (query text, - street_column text, city_column text default null, state_column text default null, country_column text default null, batch_size integer DEFAULT 100) + street_column text, city_column text default null, state_column text default null, country_column text default null, batch_size integer DEFAULT 50) RETURNS SETOF cdb_dataservices_client.geocoding AS $$ DECLARE query_row_count integer; diff --git a/client/sql/21_bulk_geocoding_functions.sql b/client/sql/21_bulk_geocoding_functions.sql index d0bcf5e..2e14965 100644 --- a/client/sql/21_bulk_geocoding_functions.sql +++ b/client/sql/21_bulk_geocoding_functions.sql @@ -1,5 +1,5 @@ CREATE OR REPLACE FUNCTION cdb_dataservices_client.cdb_bulk_geocode_street_point (query text, - street_column text, city_column text default null, state_column text default null, country_column text default null, batch_size integer DEFAULT 100) + street_column text, city_column text default null, state_column text default null, country_column text default null, batch_size integer DEFAULT 50) RETURNS SETOF cdb_dataservices_client.geocoding AS $$ DECLARE query_row_count integer; diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index 6c2ce43..c25216c 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -51,7 +51,12 @@ StreetGeocoderSearch = namedtuple('StreetGeocoderSearch', 'id address city state class StreetPointBulkGeocoder: """ Classes extending StreetPointBulkGeocoder should implement: - * _bulk_geocode(street_geocoder_searches) + * _batch_geocode(street_geocoder_searches) + * MAX_BATCH_SIZE + + If they want to provide an alternative serial (for small batches): + * _should_use_batch(street_geocoder_searches) + * _serial_geocode(street_geocoder_searches) """ SEARCH_KEYS = ['id', 'address', 'city', 'state', 'country'] @@ -77,10 +82,22 @@ class StreetPointBulkGeocoder: street_geocoder_searches.append( StreetGeocoderSearch(search_id, address, city, state, country)) - return self._bulk_geocode(street_geocoder_searches) + if len(street_geocoder_searches) > self.MAX_BATCH_SIZE: + raise Exception("Batch size can't be larger than {}".format(self.MAX_BATCH_SIZE)) + if self._should_use_batch(street_geocoder_searches): + self._logger.debug('--> Batch geocode') + return self._batch_geocode(street_geocoder_searches) + else: + self._logger.debug('--> Serial geocode') + return self._serial_geocode(street_geocoder_searches) + + def _batch_geocode(self, street_geocoder_searches): + raise NotImplementedError('Subclasses must implement _batch_geocode') + + def _serial_geocode(self, street_geocoder_searches): + raise NotImplementedError('Subclasses must implement _serial_geocode') + + def _should_use_batch(self, street_geocoder_searches): + return True + - def _bulk_geocode(self, street_geocoder_searches): - """ - Subclasses must implement _bulk_geocode - """ - raise NotImplementedError('Subclasses must implement _bulk_geocode') diff --git a/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py index 206fa54..e59e408 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py @@ -19,16 +19,6 @@ class GoogleMapsBulkGeocoder(GoogleMapsGeocoder, StreetPointBulkGeocoder): def __init__(self, client_id, client_secret, logger): GoogleMapsGeocoder.__init__(self, client_id, client_secret, logger) - def _bulk_geocode(self, searches): - if len(searches) > self.MAX_BATCH_SIZE: - raise Exception("Batch size can't be larger than {}".format(self.MAX_BATCH_SIZE)) - if self._should_use_batch(searches): - self._logger.debug('--> Batch geocode') - return self._batch_geocode(searches) - else: - self._logger.debug('--> Serial geocode') - return self._serial_geocode(searches) - def _should_use_batch(self, searches): return len(searches) >= self.MIN_BATCHED_SEARCH diff --git a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py index 9a688f4..e567e2e 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py @@ -15,13 +15,13 @@ from cartodb_services.tools.exceptions import ServiceException HereJobStatus = namedtuple('HereJobStatus', 'total_count processed_count status') class HereMapsBulkGeocoder(HereMapsGeocoder, StreetPointBulkGeocoder): - BATCH_URL = 'https://batch.geocoder.cit.api.here.com/6.2/jobs' MAX_BATCH_SIZE = 1000000 # From the docs + MIN_BATCHED_SEARCH = 100 # Under this, serial will be used + BATCH_URL = 'https://batch.geocoder.cit.api.here.com/6.2/jobs' # https://developer.here.com/documentation/batch-geocoder/topics/read-batch-request-output.html META_COLS = ['relevance', 'matchType', 'matchCode', 'matchLevel', 'matchQualityStreet'] MAX_STALLED_RETRIES = 100 BATCH_RETRY_SLEEP_S = 5 - MIN_BATCHED_SEARCH = 100 # Under this, serial will be used JOB_FINAL_STATES = ['completed', 'cancelled', 'deleted', 'failed'] def __init__(self, app_id, app_code, logger, service_params=None, maxresults=HereMapsGeocoder.DEFAULT_MAXRESULTS): @@ -34,16 +34,6 @@ class HereMapsBulkGeocoder(HereMapsGeocoder, StreetPointBulkGeocoder): 'app_code': self.app_code, } - def _bulk_geocode(self, searches): - if len(searches) > self.MAX_BATCH_SIZE: - raise Exception("Batch size can't be larger than {}".format(self.MAX_BATCH_SIZE)) - if self._should_use_batch(searches): - self._logger.debug('--> Batch geocode') - return self._batch_geocode(searches) - else: - self._logger.debug('--> Serial geocode') - return self._serial_geocode(searches) - def _should_use_batch(self, searches): return len(searches) >= self.MIN_BATCHED_SEARCH diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py index 5c502e3..63f5e72 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py @@ -21,16 +21,6 @@ class MapboxBulkGeocoder(MapboxGeocoder, StreetPointBulkGeocoder): self.max_retries = service_params.get('max_retries', self.MAX_RETRIES) self.session = requests.Session() - def _bulk_geocode(self, searches): - if len(searches) > self.MAX_BATCH_SIZE: - raise Exception("Batch size can't be larger than {}".format(self.MAX_BATCH_SIZE)) - if self._should_use_batch(searches): - self._logger.debug('--> Batch geocode') - return self._batch_geocode(searches) - else: - self._logger.debug('--> Serial geocode') - return self._serial_geocode(searches) - def _should_use_batch(self, searches): return len(searches) >= self.MIN_BATCHED_SEARCH diff --git a/server/lib/python/cartodb_services/cartodb_services/tomtom/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/tomtom/bulk_geocoder.py index 8036667..a3d8583 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tomtom/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/tomtom/bulk_geocoder.py @@ -6,12 +6,12 @@ from cartodb_services.tools.exceptions import ServiceException class TomTomBulkGeocoder(TomTomGeocoder, StreetPointBulkGeocoder): + MAX_BATCH_SIZE = 1000000 # From the docs + MIN_BATCHED_SEARCH = 10 # Batch API is really fast BASE_URL = 'https://api.tomtom.com' BATCH_URL = BASE_URL + '/search/2/batch.json' - MAX_BATCH_SIZE = 1000000 # From the docs MAX_STALLED_RETRIES = 100 BATCH_RETRY_SLEEP_S = 5 - MIN_BATCHED_SEARCH = 10 # Batch API is really fast READ_TIMEOUT = 60 CONNECT_TIMEOUT = 10 MAX_RETRIES = 1 @@ -26,16 +26,6 @@ class TomTomBulkGeocoder(TomTomGeocoder, StreetPointBulkGeocoder): self.session.mount(self.BATCH_URL, HTTPAdapter(max_retries=self.max_retries)) - def _bulk_geocode(self, searches): - if len(searches) > self.MAX_BATCH_SIZE: - raise Exception("Batch size can't be larger than {}".format(self.MAX_BATCH_SIZE)) - if self._should_use_batch(searches): - self._logger.debug('--> Batch geocode') - return self._batch_geocode(searches) - else: - self._logger.debug('--> Serial geocode') - return self._serial_geocode(searches) - def _should_use_batch(self, searches): return len(searches) >= self.MIN_BATCHED_SEARCH diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index c35e1d5..d66df30 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -8,6 +8,67 @@ from ..helpers.integration_test_helper import IntegrationTestHelper from ..helpers.integration_test_helper import assert_close_enough class TestStreetFunctionsSetUp(TestCase): + provider = None + fixture_points = None + + GOOGLE_POINTS = { + 'Plaza Mayor, Valladolid': [-4.728252, 41.6517025], + 'Paseo Zorrilla, Valladolid': [-4.7404453, 41.6314339], + '1900 amphitheatre parkway': [-122.0875324, 37.4227968], + '1901 amphitheatre parkway': [-122.0885504, 37.4238657], + '1902 amphitheatre parkway': [-122.0876674, 37.4235729], + 'Valladolid': [-4.7245321, 41.652251], + 'Valladolid, Spain': [-4.7245321, 41.652251], + 'Valladolid, Mexico': [-88.2022488, 20.68964], + 'Madrid': [-3.7037902, 40.4167754], + 'Logroño, Spain': [-2.4449852, 42.4627195], + 'Logroño, Argentina': [-61.6961807, -29.5031057], + 'Plaza España 1, Barcelona': [2.1482563, 41.375485] + } + + HERE_POINTS = { + 'Plaza Mayor, Valladolid': [-4.72979, 41.65258], + 'Paseo Zorrilla, Valladolid': [-4.73869, 41.63817], + '1900 amphitheatre parkway': [-122.0879468, 37.4234763], + '1901 amphitheatre parkway': [-122.0879253, 37.4238725], + '1902 amphitheatre parkway': [-122.0879531, 37.4234775], + 'Valladolid': [-4.73214, 41.6542], + 'Valladolid, Spain': [-4.73214, 41.6542], + 'Valladolid, Mexico': [-88.20117, 20.69021], + 'Madrid': [-3.70578, 40.42028], + 'Logroño, Spain': [-2.45194, 42.46592], + 'Logroño, Argentina': [-61.69604, -29.50425], + 'Plaza España 1, Barcelona': [2.1735699, 41.3823] # TODO: not ideal + } + + TOMTOM_POINTS = HERE_POINTS.copy() + TOMTOM_POINTS.update({ + 'Plaza Mayor, Valladolid': [-4.72183, 41.5826], + 'Paseo Zorrilla, Valladolid': [-4.74031, 41.63181], + 'Valladolid': [-4.72838, 41.6542], + 'Valladolid, Spain': [-4.72838, 41.6542], + 'Madrid': [-3.70035, 40.42028], + 'Logroño, Spain': [-2.44998, 42.46592], + 'Plaza España 1, Barcelona': [2.07479, 41.36818] # TODO: not ideal + }) + + MAPBOX_POINTS = GOOGLE_POINTS.copy() + MAPBOX_POINTS.update({ + 'Logroño, Spain': [-2.44556, 42.47], + 'Logroño, Argentina': [-70.687195, -33.470901], # TODO: huge mismatch + 'Valladolid': [-4.72856, 41.652251], + 'Valladolid, Spain': [-4.72856, 41.652251], + '1902 amphitheatre parkway': [-118.03, 34.06], # TODO: huge mismatch + 'Madrid': [-3.69194, 40.4167754], + 'Plaza España 1, Barcelona': [2.245969, 41.452483] # TODO: not ideal + }) + + FIXTURE_POINTS = { + 'google': GOOGLE_POINTS, + 'heremaps': HERE_POINTS, + 'tomtom': TOMTOM_POINTS, + 'mapbox': MAPBOX_POINTS + } def setUp(self): self.env_variables = IntegrationTestHelper.get_environment_variables() @@ -18,6 +79,15 @@ class TestStreetFunctionsSetUp(TestCase): self.env_variables['api_key'] ) + if not self.fixture_points: + query = "select provider from " \ + "cdb_dataservices_client.cdb_service_quota_info() " \ + "where service = 'hires_geocoder'" + response = self._run_authenticated(query) + provider = response['rows'][0]['provider'] + self.fixture_points = self.FIXTURE_POINTS[provider] + + def _run_authenticated(self, query): authenticated_query = "{}&api_key={}".format(query, self.env_variables[ @@ -54,77 +124,9 @@ class TestStreetFunctions(TestStreetFunctionsSetUp): row = response['rows'][0] x_y = [row['st_x'], row['st_y']] # Wrong coordinates (Plaza España, Madrid): [-3.7138975, 40.4256762] - assert_close_enough(x_y, [2.1482563, 41.375485]) + assert_close_enough(x_y, self.fixture_points['Plaza España 1, Barcelona']) class TestBulkStreetFunctions(TestStreetFunctionsSetUp): - provider = None - fixture_points = None - - GOOGLE_POINTS = { - 'Plaza Mayor, Valladolid': [-4.728252, 41.6517025], - 'Paseo Zorrilla, Valladolid': [-4.7404453, 41.6314339], - '1900 amphitheatre parkway': [-122.0875324, 37.4227968], - '1901 amphitheatre parkway': [-122.0885504, 37.4238657], - '1902 amphitheatre parkway': [-122.0876674, 37.4235729], - 'Valladolid': [-4.7245321, 41.652251], - 'Valladolid, Spain': [-4.7245321, 41.652251], - 'Valladolid, Mexico': [-88.2022488, 20.68964], - 'Madrid': [-3.7037902, 40.4167754], - 'Logroño, Spain': [-2.4449852, 42.4627195], - 'Logroño, Argentina': [-61.6961807, -29.5031057] - } - - HERE_POINTS = { - 'Plaza Mayor, Valladolid': [-4.72979, 41.65258], - 'Paseo Zorrilla, Valladolid': [-4.73869, 41.63817], - '1900 amphitheatre parkway': [-122.0879468, 37.4234763], - '1901 amphitheatre parkway': [-122.0879253, 37.4238725], - '1902 amphitheatre parkway': [-122.0879531, 37.4234775], - 'Valladolid': [-4.73214, 41.6542], - 'Valladolid, Spain': [-4.73214, 41.6542], - 'Valladolid, Mexico': [-88.20117, 20.69021], - 'Madrid': [-3.70578, 40.42028], - 'Logroño, Spain': [-2.45194, 42.46592], - 'Logroño, Argentina': [-61.69604, -29.50425] - } - - TOMTOM_POINTS = HERE_POINTS.copy() - TOMTOM_POINTS.update({ - 'Plaza Mayor, Valladolid': [-4.72183, 41.5826], - 'Paseo Zorrilla, Valladolid': [-4.74031, 41.63181], - 'Valladolid': [-4.72838, 41.6542], - 'Valladolid, Spain': [-4.72838, 41.6542], - 'Madrid': [-3.70035, 40.42028], - 'Logroño, Spain': [-2.44998, 42.46592], - }) - - MAPBOX_POINTS = GOOGLE_POINTS.copy() - MAPBOX_POINTS.update({ - 'Logroño, Spain': [-2.44556, 42.47], - 'Logroño, Argentina': [-70.687195, -33.470901], # TODO: huge mismatch - 'Valladolid': [-4.72856, 41.652251], - 'Valladolid, Spain': [-4.72856, 41.652251], - '1902 amphitheatre parkway': [-118.03, 34.06], # TODO: huge mismatch - 'Madrid': [-3.69194, 40.4167754], - }) - - FIXTURE_POINTS = { - 'google': GOOGLE_POINTS, - 'heremaps': HERE_POINTS, - 'tomtom': TOMTOM_POINTS, - 'mapbox': MAPBOX_POINTS - } - - def setUp(self): - TestStreetFunctionsSetUp.setUp(self) - - if not self.fixture_points: - query = "select provider from " \ - "cdb_dataservices_client.cdb_service_quota_info() " \ - "where service = 'hires_geocoder'" - response = self._run_authenticated(query) - provider = response['rows'][0]['provider'] - self.fixture_points = self.FIXTURE_POINTS[provider] def test_full_spec(self): query = "select cartodb_id, st_x(the_geom), st_y(the_geom) " \ @@ -257,7 +259,7 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): """ Useful just to test a good batch size """ - n = 10 + n = 50 streets = [] for i in range(0, n): streets.append('{{"cartodb_id": {}, "address": "{} Yonge Street, ' \ @@ -292,6 +294,19 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): x_y_by_cartodb_id = self._x_y_by_cartodb_id(response) assert_equal(x_y_by_cartodb_id[1], x_y_by_cartodb_id[2]) + # "'Plaza España 1', 'Barcelona', null, 'Spain') as the_geom) _x" + def test_component_aggregation(self): + query = "select cartodb_id, st_x(the_geom), st_y(the_geom) " \ + "FROM cdb_dataservices_client.cdb_bulk_geocode_street_point(" \ + "'select 1 as cartodb_id, ''Spain'' as country, " \ + "''Barcelona'' as city, " \ + "''Plaza España 1'' as street' " \ + ", 'street', 'city', NULL, 'country')" + response = self._run_authenticated(query) + + assert_close_enough(self._x_y_by_cartodb_id(response)[1], + self.fixture_points['Plaza España 1, Barcelona']) + def _run_authenticated(self, query): authenticated_query = "{}&api_key={}".format(query, self.env_variables[ From 531ad281589eaa838a76cd6fcd1d18c606b200d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 10 Jul 2018 19:06:49 +0200 Subject: [PATCH 07/31] Send optimal batch size --- client/cdb_dataservices_client--0.25.0.sql | 64 +++++++++++-------- client/sql/16_custom_types.sql | 3 +- client/sql/21_bulk_geocoding_functions.sql | 61 ++++++++++-------- .../21_bulk_geocoding_functions_test.out | 20 +++--- .../sql/21_bulk_geocoding_functions_test.sql | 18 +++--- .../cdb_dataservices_server--0.32.0.sql | 17 +++-- server/extension/sql/200_quotas.sql | 17 +++-- .../cartodb_services/bulk_geocoders.py | 11 ++++ .../cartodb_services/here/bulk_geocoder.py | 13 ++-- test/integration/test_street_functions.py | 19 +++++- 10 files changed, 153 insertions(+), 90 deletions(-) create mode 100644 server/lib/python/cartodb_services/cartodb_services/bulk_geocoders.py diff --git a/client/cdb_dataservices_client--0.25.0.sql b/client/cdb_dataservices_client--0.25.0.sql index d21bb5f..2bed351 100644 --- a/client/cdb_dataservices_client--0.25.0.sql +++ b/client/cdb_dataservices_client--0.25.0.sql @@ -112,7 +112,8 @@ CREATE TYPE cdb_dataservices_client.service_quota_info AS ( monthly_quota NUMERIC, used_quota NUMERIC, soft_limit BOOLEAN, - provider TEXT + provider TEXT, + max_batch_size NUMERIC ); -- -- Public dataservices API function @@ -1987,25 +1988,36 @@ CREATE OR REPLACE FUNCTION cdb_dataservices_client._DST_DisconnectUserTable( TARGET cdb_dataservices_server._DST_DisconnectUserTable; $$ LANGUAGE plproxy VOLATILE PARALLEL UNSAFE; CREATE OR REPLACE FUNCTION cdb_dataservices_client.cdb_bulk_geocode_street_point (query text, - street_column text, city_column text default null, state_column text default null, country_column text default null, batch_size integer DEFAULT 50) + street_column text, city_column text default null, state_column text default null, country_column text default null, batch_size integer DEFAULT NULL) RETURNS SETOF cdb_dataservices_client.geocoding AS $$ DECLARE query_row_count integer; enough_quota boolean; remaining_quota integer; + max_batch_size integer; cartodb_id_batch integer; batches_n integer; DEFAULT_BATCH_SIZE CONSTANT numeric := 100; - MAX_BATCH_SIZE CONSTANT numeric := 10000; + MAX_SAFE_BATCH_SIZE CONSTANT numeric := 5000; current_row_count integer ; temp_table_name text; BEGIN + SELECT csqi.monthly_quota - csqi.used_quota AS remaining_quota, csqi.max_batch_size + INTO remaining_quota, max_batch_size + FROM cdb_dataservices_client.cdb_service_quota_info() csqi + WHERE service = 'hires_geocoder'; + RAISE DEBUG 'remaining_quota: %; max_batch_size: %', remaining_quota, max_batch_size; + IF batch_size IS NULL THEN - RAISE EXCEPTION 'batch_size can''t be null'; - ELSIF batch_size > MAX_BATCH_SIZE THEN - RAISE EXCEPTION 'batch_size must be lower than %', MAX_BATCH_SIZE + 1; + batch_size := max_batch_size; + ELSIF batch_size > max_batch_size THEN + RAISE EXCEPTION 'batch_size must be lower than %', max_batch_size + 1; + END IF; + + IF batch_size > MAX_SAFE_BATCH_SIZE THEN + batch_size := MAX_SAFE_BATCH_SIZE; END IF; EXECUTE format('SELECT COUNT(1) from (%s) _x', query) INTO query_row_count; @@ -2013,11 +2025,7 @@ BEGIN RAISE DEBUG 'cdb_bulk_geocode_street_point --> query_row_count: %; query: %; country: %; state: %; city: %; street: %', query_row_count, query, country_column, state_column, city_column, street_column; SELECT cdb_dataservices_client.cdb_enough_quota('hires_geocoder', query_row_count) INTO enough_quota; - IF enough_quota IS NOT NULL AND NOT enough_quota THEN - SELECT csqi.monthly_quota - csqi.used_quota AS remaining_quota - INTO remaining_quota - FROM cdb_dataservices_client.cdb_service_quota_info() csqi - WHERE service = 'hires_geocoder'; + IF remaining_quota < query_row_count THEN RAISE EXCEPTION 'Remaining quota: %. Estimated cost: %', remaining_quota, query_row_count; END IF; @@ -2036,25 +2044,27 @@ BEGIN coalesce(state_column, ''''''), coalesce(country_column, '''''') into street_column, city_column, state_column, country_column; - FOR cartodb_id_batch in 0..(batches_n - 1) - LOOP + IF batches_n > 0 THEN + FOR cartodb_id_batch in 0..(batches_n - 1) + LOOP - EXECUTE format( - 'WITH geocoding_data as (' || - ' SELECT ' || - ' json_build_object(''id'', cartodb_id, ''address'', %s, ''city'', %s, ''state'', %s, ''country'', %s) as data , ' || - ' floor((cartodb_id-1)::float/$1) as batch' || - ' FROM (%s) _x' || - ') ' || - 'INSERT INTO %s SELECT (cdb_dataservices_client._cdb_bulk_geocode_street_point(jsonb_agg(data))).* ' || - 'FROM geocoding_data ' || - 'WHERE batch = $2', street_column, city_column, state_column, country_column, query, temp_table_name) - USING batch_size, cartodb_id_batch; + EXECUTE format( + 'WITH geocoding_data as (' || + ' SELECT ' || + ' json_build_object(''id'', cartodb_id, ''address'', %s, ''city'', %s, ''state'', %s, ''country'', %s) as data , ' || + ' floor((cartodb_id-1)::float/$1) as batch' || + ' FROM (%s) _x' || + ') ' || + 'INSERT INTO %s SELECT (cdb_dataservices_client._cdb_bulk_geocode_street_point(jsonb_agg(data))).* ' || + 'FROM geocoding_data ' || + 'WHERE batch = $2', street_column, city_column, state_column, country_column, query, temp_table_name) + USING batch_size, cartodb_id_batch; - GET DIAGNOSTICS current_row_count = ROW_COUNT; - RAISE DEBUG 'Batch % --> %', cartodb_id_batch, current_row_count; + GET DIAGNOSTICS current_row_count = ROW_COUNT; + RAISE DEBUG 'Batch % --> %', cartodb_id_batch, current_row_count; - END LOOP; + END LOOP; + END IF; RETURN QUERY EXECUTE 'SELECT * FROM ' || quote_ident(temp_table_name); END; diff --git a/client/sql/16_custom_types.sql b/client/sql/16_custom_types.sql index 1b74cac..7458ffa 100644 --- a/client/sql/16_custom_types.sql +++ b/client/sql/16_custom_types.sql @@ -39,5 +39,6 @@ CREATE TYPE cdb_dataservices_client.service_quota_info AS ( monthly_quota NUMERIC, used_quota NUMERIC, soft_limit BOOLEAN, - provider TEXT + provider TEXT, + max_batch_size NUMERIC ); diff --git a/client/sql/21_bulk_geocoding_functions.sql b/client/sql/21_bulk_geocoding_functions.sql index 2e14965..f9ba824 100644 --- a/client/sql/21_bulk_geocoding_functions.sql +++ b/client/sql/21_bulk_geocoding_functions.sql @@ -1,23 +1,34 @@ CREATE OR REPLACE FUNCTION cdb_dataservices_client.cdb_bulk_geocode_street_point (query text, - street_column text, city_column text default null, state_column text default null, country_column text default null, batch_size integer DEFAULT 50) + street_column text, city_column text default null, state_column text default null, country_column text default null, batch_size integer DEFAULT NULL) RETURNS SETOF cdb_dataservices_client.geocoding AS $$ DECLARE query_row_count integer; enough_quota boolean; remaining_quota integer; + max_batch_size integer; cartodb_id_batch integer; batches_n integer; DEFAULT_BATCH_SIZE CONSTANT numeric := 100; - MAX_BATCH_SIZE CONSTANT numeric := 10000; + MAX_SAFE_BATCH_SIZE CONSTANT numeric := 5000; current_row_count integer ; temp_table_name text; BEGIN + SELECT csqi.monthly_quota - csqi.used_quota AS remaining_quota, csqi.max_batch_size + INTO remaining_quota, max_batch_size + FROM cdb_dataservices_client.cdb_service_quota_info() csqi + WHERE service = 'hires_geocoder'; + RAISE DEBUG 'remaining_quota: %; max_batch_size: %', remaining_quota, max_batch_size; + IF batch_size IS NULL THEN - RAISE EXCEPTION 'batch_size can''t be null'; - ELSIF batch_size > MAX_BATCH_SIZE THEN - RAISE EXCEPTION 'batch_size must be lower than %', MAX_BATCH_SIZE + 1; + batch_size := max_batch_size; + ELSIF batch_size > max_batch_size THEN + RAISE EXCEPTION 'batch_size must be lower than %', max_batch_size + 1; + END IF; + + IF batch_size > MAX_SAFE_BATCH_SIZE THEN + batch_size := MAX_SAFE_BATCH_SIZE; END IF; EXECUTE format('SELECT COUNT(1) from (%s) _x', query) INTO query_row_count; @@ -25,11 +36,7 @@ BEGIN RAISE DEBUG 'cdb_bulk_geocode_street_point --> query_row_count: %; query: %; country: %; state: %; city: %; street: %', query_row_count, query, country_column, state_column, city_column, street_column; SELECT cdb_dataservices_client.cdb_enough_quota('hires_geocoder', query_row_count) INTO enough_quota; - IF enough_quota IS NOT NULL AND NOT enough_quota THEN - SELECT csqi.monthly_quota - csqi.used_quota AS remaining_quota - INTO remaining_quota - FROM cdb_dataservices_client.cdb_service_quota_info() csqi - WHERE service = 'hires_geocoder'; + IF remaining_quota < query_row_count THEN RAISE EXCEPTION 'Remaining quota: %. Estimated cost: %', remaining_quota, query_row_count; END IF; @@ -48,25 +55,27 @@ BEGIN coalesce(state_column, ''''''), coalesce(country_column, '''''') into street_column, city_column, state_column, country_column; - FOR cartodb_id_batch in 0..(batches_n - 1) - LOOP + IF batches_n > 0 THEN + FOR cartodb_id_batch in 0..(batches_n - 1) + LOOP - EXECUTE format( - 'WITH geocoding_data as (' || - ' SELECT ' || - ' json_build_object(''id'', cartodb_id, ''address'', %s, ''city'', %s, ''state'', %s, ''country'', %s) as data , ' || - ' floor((cartodb_id-1)::float/$1) as batch' || - ' FROM (%s) _x' || - ') ' || - 'INSERT INTO %s SELECT (cdb_dataservices_client._cdb_bulk_geocode_street_point(jsonb_agg(data))).* ' || - 'FROM geocoding_data ' || - 'WHERE batch = $2', street_column, city_column, state_column, country_column, query, temp_table_name) - USING batch_size, cartodb_id_batch; + EXECUTE format( + 'WITH geocoding_data as (' || + ' SELECT ' || + ' json_build_object(''id'', cartodb_id, ''address'', %s, ''city'', %s, ''state'', %s, ''country'', %s) as data , ' || + ' floor((cartodb_id-1)::float/$1) as batch' || + ' FROM (%s) _x' || + ') ' || + 'INSERT INTO %s SELECT (cdb_dataservices_client._cdb_bulk_geocode_street_point(jsonb_agg(data))).* ' || + 'FROM geocoding_data ' || + 'WHERE batch = $2', street_column, city_column, state_column, country_column, query, temp_table_name) + USING batch_size, cartodb_id_batch; - GET DIAGNOSTICS current_row_count = ROW_COUNT; - RAISE DEBUG 'Batch % --> %', cartodb_id_batch, current_row_count; + GET DIAGNOSTICS current_row_count = ROW_COUNT; + RAISE DEBUG 'Batch % --> %', cartodb_id_batch, current_row_count; - END LOOP; + END LOOP; + END IF; RETURN QUERY EXECUTE 'SELECT * FROM ' || quote_ident(temp_table_name); END; diff --git a/client/test/expected/21_bulk_geocoding_functions_test.out b/client/test/expected/21_bulk_geocoding_functions_test.out index 7ca70db..d2f268b 100644 --- a/client/test/expected/21_bulk_geocoding_functions_test.out +++ b/client/test/expected/21_bulk_geocoding_functions_test.out @@ -1,21 +1,21 @@ \set VERBOSITY terse --- Test bulk size mandatory -SELECT cdb_dataservices_client.cdb_bulk_geocode_street_point('select 1 as cartodb_id', '''Valladolid, Spain''', null, null, null, null); -ERROR: batch_size can't be null --- Test quota check by mocking quota 0 +ALTER FUNCTION cdb_dataservices_client.cdb_service_quota_info() RENAME TO cdb_service_quota_info_mocked; +CREATE FUNCTION cdb_dataservices_client.cdb_service_quota_info () +RETURNS SETOF cdb_dataservices_client.service_quota_info AS $$ + SELECT 'hires_geocoder'::cdb_dataservices_client.service_type AS service, 0::NUMERIC AS monthly_quota, 0::NUMERIC AS used_quota, FALSE AS soft_limit, 'google' AS provider, 1::NUMERIC AS max_batch_size; +$$ LANGUAGE SQL; ALTER FUNCTION cdb_dataservices_client.cdb_enough_quota (service TEXT ,input_size NUMERIC) RENAME TO cdb_enough_quota_mocked; CREATE FUNCTION cdb_dataservices_client.cdb_enough_quota (service TEXT ,input_size NUMERIC) RETURNS BOOLEAN as $$ SELECT FALSE; $$ LANGUAGE SQL; -ALTER FUNCTION cdb_dataservices_client.cdb_service_quota_info() RENAME TO cdb_service_quota_info_mocked; -CREATE FUNCTION cdb_dataservices_client.cdb_service_quota_info () -RETURNS SETOF cdb_dataservices_client.service_quota_info AS $$ - SELECT 'hires_geocoder'::cdb_dataservices_client.service_type AS service, 0::NUMERIC AS monthly_quota, 0::NUMERIC AS used_quota, FALSE AS soft_limit, 'google' AS provider; -$$ LANGUAGE SQL; +-- Test bulk size not mandatory (it will get the optimal) +SELECT cdb_dataservices_client.cdb_bulk_geocode_street_point('select 1 as cartodb_id', '''Valladolid, Spain''', null, null, null, null); +ERROR: Remaining quota: 0. Estimated cost: 1 +-- Test quota check by mocking quota 0 SELECT cdb_dataservices_client.cdb_bulk_geocode_street_point('select 1 as cartodb_id', '''Valladolid, Spain'''); ERROR: Remaining quota: 0. Estimated cost: 1 DROP FUNCTION cdb_dataservices_client.cdb_service_quota_info; DROP FUNCTION cdb_dataservices_client.cdb_enough_quota; -ALTER FUNCTION cdb_dataservices_client.cdb_service_quota_info_mocked() RENAME TO cdb_service_quota_info; ALTER FUNCTION cdb_dataservices_client.cdb_enough_quota_mocked (service TEXT ,input_size NUMERIC) RENAME TO cdb_enough_quota; +ALTER FUNCTION cdb_dataservices_client.cdb_service_quota_info_mocked() RENAME TO cdb_service_quota_info; diff --git a/client/test/sql/21_bulk_geocoding_functions_test.sql b/client/test/sql/21_bulk_geocoding_functions_test.sql index 5433bd7..d17f470 100644 --- a/client/test/sql/21_bulk_geocoding_functions_test.sql +++ b/client/test/sql/21_bulk_geocoding_functions_test.sql @@ -1,26 +1,26 @@ \set VERBOSITY terse --- Test bulk size mandatory -SELECT cdb_dataservices_client.cdb_bulk_geocode_street_point('select 1 as cartodb_id', '''Valladolid, Spain''', null, null, null, null); +ALTER FUNCTION cdb_dataservices_client.cdb_service_quota_info() RENAME TO cdb_service_quota_info_mocked; +CREATE FUNCTION cdb_dataservices_client.cdb_service_quota_info () +RETURNS SETOF cdb_dataservices_client.service_quota_info AS $$ + SELECT 'hires_geocoder'::cdb_dataservices_client.service_type AS service, 0::NUMERIC AS monthly_quota, 0::NUMERIC AS used_quota, FALSE AS soft_limit, 'google' AS provider, 1::NUMERIC AS max_batch_size; +$$ LANGUAGE SQL; --- Test quota check by mocking quota 0 ALTER FUNCTION cdb_dataservices_client.cdb_enough_quota (service TEXT ,input_size NUMERIC) RENAME TO cdb_enough_quota_mocked; CREATE FUNCTION cdb_dataservices_client.cdb_enough_quota (service TEXT ,input_size NUMERIC) RETURNS BOOLEAN as $$ SELECT FALSE; $$ LANGUAGE SQL; -ALTER FUNCTION cdb_dataservices_client.cdb_service_quota_info() RENAME TO cdb_service_quota_info_mocked; -CREATE FUNCTION cdb_dataservices_client.cdb_service_quota_info () -RETURNS SETOF cdb_dataservices_client.service_quota_info AS $$ - SELECT 'hires_geocoder'::cdb_dataservices_client.service_type AS service, 0::NUMERIC AS monthly_quota, 0::NUMERIC AS used_quota, FALSE AS soft_limit, 'google' AS provider; -$$ LANGUAGE SQL; +-- Test bulk size not mandatory (it will get the optimal) +SELECT cdb_dataservices_client.cdb_bulk_geocode_street_point('select 1 as cartodb_id', '''Valladolid, Spain''', null, null, null, null); +-- Test quota check by mocking quota 0 SELECT cdb_dataservices_client.cdb_bulk_geocode_street_point('select 1 as cartodb_id', '''Valladolid, Spain'''); DROP FUNCTION cdb_dataservices_client.cdb_service_quota_info; DROP FUNCTION cdb_dataservices_client.cdb_enough_quota; -ALTER FUNCTION cdb_dataservices_client.cdb_service_quota_info_mocked() RENAME TO cdb_service_quota_info; ALTER FUNCTION cdb_dataservices_client.cdb_enough_quota_mocked (service TEXT ,input_size NUMERIC) RENAME TO cdb_enough_quota; +ALTER FUNCTION cdb_dataservices_client.cdb_service_quota_info_mocked() RENAME TO cdb_service_quota_info; diff --git a/server/extension/cdb_dataservices_server--0.32.0.sql b/server/extension/cdb_dataservices_server--0.32.0.sql index a013f3c..dc82322 100644 --- a/server/extension/cdb_dataservices_server--0.32.0.sql +++ b/server/extension/cdb_dataservices_server--0.32.0.sql @@ -1861,7 +1861,8 @@ BEGIN monthly_quota NUMERIC, used_quota NUMERIC, soft_limit BOOLEAN, - provider TEXT + provider TEXT, + max_batch_size NUMERIC ); END IF; END $$; @@ -1872,6 +1873,7 @@ CREATE OR REPLACE FUNCTION cdb_dataservices_server.cdb_service_quota_info( RETURNS SETOF cdb_dataservices_server.service_quota_info AS $$ from cartodb_services.metrics.user import UserMetricsService from datetime import date + from cartodb_services.bulk_geocoders import BATCH_GEOCODER_CLASS_BY_PROVIDER plpy.execute("SELECT cdb_dataservices_server._connect_to_redis('{0}')".format(username)) redis_conn = GD["redis_connection_{0}".format(username)]['redis_metrics_connection'] @@ -1889,7 +1891,7 @@ RETURNS SETOF cdb_dataservices_server.service_quota_info AS $$ used_quota = user_service.used_quota(user_isolines_config.service_type, today) soft_limit = user_isolines_config.soft_isolines_limit provider = user_isolines_config.provider - ret += [[service, monthly_quota, used_quota, soft_limit, provider]] + ret += [[service, monthly_quota, used_quota, soft_limit, provider, 1]] #-- Hires Geocoder service = 'hires_geocoder' @@ -1901,7 +1903,12 @@ RETURNS SETOF cdb_dataservices_server.service_quota_info AS $$ used_quota = user_service.used_quota(user_geocoder_config.service_type, today) soft_limit = user_geocoder_config.soft_geocoding_limit provider = user_geocoder_config.provider - ret += [[service, monthly_quota, used_quota, soft_limit, provider]] + batch_geocoder_class = BATCH_GEOCODER_CLASS_BY_PROVIDER.get(provider, None) + if batch_geocoder_class and hasattr(batch_geocoder_class, 'MAX_BATCH_SIZE'): + max_batch_size = batch_geocoder_class.MAX_BATCH_SIZE + else: + max_batch_size = 1 + ret += [[service, monthly_quota, used_quota, soft_limit, provider, max_batch_size]] #-- Routing service = 'routing' @@ -1913,7 +1920,7 @@ RETURNS SETOF cdb_dataservices_server.service_quota_info AS $$ used_quota = user_service.used_quota(user_routing_config.service_type, today) soft_limit = user_routing_config.soft_limit provider = user_routing_config.provider - ret += [[service, monthly_quota, used_quota, soft_limit, provider]] + ret += [[service, monthly_quota, used_quota, soft_limit, provider, 1]] #-- Observatory service = 'observatory' @@ -1925,7 +1932,7 @@ RETURNS SETOF cdb_dataservices_server.service_quota_info AS $$ used_quota = user_service.used_quota(user_obs_config.service_type, today) soft_limit = user_obs_config.soft_limit provider = user_obs_config.provider - ret += [[service, monthly_quota, used_quota, soft_limit, provider]] + ret += [[service, monthly_quota, used_quota, soft_limit, provider, 1]] return ret $$ LANGUAGE plpythonu STABLE PARALLEL RESTRICTED; diff --git a/server/extension/sql/200_quotas.sql b/server/extension/sql/200_quotas.sql index 28ee724..6f34083 100644 --- a/server/extension/sql/200_quotas.sql +++ b/server/extension/sql/200_quotas.sql @@ -22,7 +22,8 @@ BEGIN monthly_quota NUMERIC, used_quota NUMERIC, soft_limit BOOLEAN, - provider TEXT + provider TEXT, + max_batch_size NUMERIC ); END IF; END $$; @@ -33,6 +34,7 @@ CREATE OR REPLACE FUNCTION cdb_dataservices_server.cdb_service_quota_info( RETURNS SETOF cdb_dataservices_server.service_quota_info AS $$ from cartodb_services.metrics.user import UserMetricsService from datetime import date + from cartodb_services.bulk_geocoders import BATCH_GEOCODER_CLASS_BY_PROVIDER plpy.execute("SELECT cdb_dataservices_server._connect_to_redis('{0}')".format(username)) redis_conn = GD["redis_connection_{0}".format(username)]['redis_metrics_connection'] @@ -50,7 +52,7 @@ RETURNS SETOF cdb_dataservices_server.service_quota_info AS $$ used_quota = user_service.used_quota(user_isolines_config.service_type, today) soft_limit = user_isolines_config.soft_isolines_limit provider = user_isolines_config.provider - ret += [[service, monthly_quota, used_quota, soft_limit, provider]] + ret += [[service, monthly_quota, used_quota, soft_limit, provider, 1]] #-- Hires Geocoder service = 'hires_geocoder' @@ -62,7 +64,12 @@ RETURNS SETOF cdb_dataservices_server.service_quota_info AS $$ used_quota = user_service.used_quota(user_geocoder_config.service_type, today) soft_limit = user_geocoder_config.soft_geocoding_limit provider = user_geocoder_config.provider - ret += [[service, monthly_quota, used_quota, soft_limit, provider]] + batch_geocoder_class = BATCH_GEOCODER_CLASS_BY_PROVIDER.get(provider, None) + if batch_geocoder_class and hasattr(batch_geocoder_class, 'MAX_BATCH_SIZE'): + max_batch_size = batch_geocoder_class.MAX_BATCH_SIZE + else: + max_batch_size = 1 + ret += [[service, monthly_quota, used_quota, soft_limit, provider, max_batch_size]] #-- Routing service = 'routing' @@ -74,7 +81,7 @@ RETURNS SETOF cdb_dataservices_server.service_quota_info AS $$ used_quota = user_service.used_quota(user_routing_config.service_type, today) soft_limit = user_routing_config.soft_limit provider = user_routing_config.provider - ret += [[service, monthly_quota, used_quota, soft_limit, provider]] + ret += [[service, monthly_quota, used_quota, soft_limit, provider, 1]] #-- Observatory service = 'observatory' @@ -86,7 +93,7 @@ RETURNS SETOF cdb_dataservices_server.service_quota_info AS $$ used_quota = user_service.used_quota(user_obs_config.service_type, today) soft_limit = user_obs_config.soft_limit provider = user_obs_config.provider - ret += [[service, monthly_quota, used_quota, soft_limit, provider]] + ret += [[service, monthly_quota, used_quota, soft_limit, provider, 1]] return ret $$ LANGUAGE plpythonu STABLE PARALLEL RESTRICTED; diff --git a/server/lib/python/cartodb_services/cartodb_services/bulk_geocoders.py b/server/lib/python/cartodb_services/cartodb_services/bulk_geocoders.py new file mode 100644 index 0000000..6dfd555 --- /dev/null +++ b/server/lib/python/cartodb_services/cartodb_services/bulk_geocoders.py @@ -0,0 +1,11 @@ +from google import GoogleMapsBulkGeocoder +from here import HereMapsBulkGeocoder +from tomtom import TomTomBulkGeocoder +from mapbox import MapboxBulkGeocoder + +BATCH_GEOCODER_CLASS_BY_PROVIDER = { + 'google': GoogleMapsBulkGeocoder, + 'heremaps': HereMapsBulkGeocoder, + 'tomtom': TomTomBulkGeocoder, + 'mapbox': MapboxBulkGeocoder +} diff --git a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py index e567e2e..3724bcd 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py @@ -16,7 +16,7 @@ HereJobStatus = namedtuple('HereJobStatus', 'total_count processed_count status' class HereMapsBulkGeocoder(HereMapsGeocoder, StreetPointBulkGeocoder): MAX_BATCH_SIZE = 1000000 # From the docs - MIN_BATCHED_SEARCH = 100 # Under this, serial will be used + MIN_BATCHED_SEARCH = 1000 # Under this, serial will be used BATCH_URL = 'https://batch.geocoder.cit.api.here.com/6.2/jobs' # https://developer.here.com/documentation/batch-geocoder/topics/read-batch-request-output.html META_COLS = ['relevance', 'matchType', 'matchCode', 'matchLevel', 'matchQualityStreet'] @@ -55,14 +55,17 @@ class HereMapsBulkGeocoder(HereMapsGeocoder, StreetPointBulkGeocoder): while True: job_info = self._job_status(request_id) if job_info.processed_count == last_processed: + self._logger.debug('--> no progress ({})'.format(last_processed)) stalled_retries += 1 if stalled_retries > self.MAX_STALLED_RETRIES: raise Exception('Too many retries for job {}'.format(request_id)) else: + self._logger.debug('--> progress ({} != {})'.format(job_info.processed_count, last_processed)) stalled_retries = 0 last_processed = job_info.processed_count - self._logger.debug('--> Job poll check: {}'.format(job_info)) + self._logger.debug('--> Job poll check ({}): {}'.format( + stalled_retries, job_info)) if job_info.status in self.JOB_FINAL_STATES: break else: @@ -95,7 +98,7 @@ class HereMapsBulkGeocoder(HereMapsGeocoder, StreetPointBulkGeocoder): request_params.update({ 'gen': 8, 'action': 'run', - #'mailto': 'juanignaciosl@carto.com', + # 'mailto': 'juanignaciosl@carto.com', 'header': 'true', 'inDelim': '|', 'outDelim': '|', @@ -121,8 +124,8 @@ class HereMapsBulkGeocoder(HereMapsGeocoder, StreetPointBulkGeocoder): timeout=(self.connect_timeout, self.read_timeout)) polling_root = ET.fromstring(polling_r.text) return HereJobStatus( - total_count=polling_root.find('./Response/TotalCount').text, - processed_count=polling_root.find('./Response/ProcessedCount').text, + total_count=int(polling_root.find('./Response/TotalCount').text), + processed_count=int(polling_root.find('./Response/ProcessedCount').text), status=polling_root.find('./Response/Status').text) def _download_results(self, job_id): diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index d66df30..52ffbe0 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -259,7 +259,8 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): """ Useful just to test a good batch size """ - n = 50 + n = 110 + batch_size = 'NULL' # NULL for optimal streets = [] for i in range(0, n): streets.append('{{"cartodb_id": {}, "address": "{} Yonge Street, ' \ @@ -270,7 +271,7 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): "'select * from jsonb_to_recordset(''[" \ "{}" \ "]''::jsonb) as (cartodb_id integer, address text)', " \ - "'address', null, null, null, {})".format(','.join(streets), n) + "'address', null, null, null, {})".format(','.join(streets), batch_size) response = self._run_authenticated(query) assert_equal(n - 1, len(response['rows'])) @@ -307,6 +308,20 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): assert_close_enough(self._x_y_by_cartodb_id(response)[1], self.fixture_points['Plaza España 1, Barcelona']) + def _test_known_table(self): + subquery = 'select * from known_table where cartodb_id < 1100' + subquery_count = 'select count(1) from ({}) _x'.format(subquery) + count = self._run_authenticated(subquery_count)['rows'][0]['count'] + + query = "select cartodb_id, st_x(the_geom), st_y(the_geom) " \ + "FROM cdb_dataservices_client.cdb_bulk_geocode_street_point(" \ + "'{}' " \ + ", 'street', 'city', NULL, 'country')".format(subquery) + response = self._run_authenticated(query) + assert_equal(len(response['rows']), count) + assert_not_equal(response['rows'][0]['st_x'], None) + + def _run_authenticated(self, query): authenticated_query = "{}&api_key={}".format(query, self.env_variables[ From 34e622b8092b8e37ab8a6d9d5856126a529ffe69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 10 Jul 2018 20:30:01 +0200 Subject: [PATCH 08/31] Relevance metadata for HERE --- .../cartodb_services/geocoder.py | 10 ++++++++-- .../cartodb_services/here/bulk_geocoder.py | 12 ++++++++---- .../cartodb_services/here/geocoder.py | 17 +++++++++++++---- test/integration/test_street_functions.py | 15 ++++++++++++--- 4 files changed, 41 insertions(+), 13 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index c25216c..cea8ef4 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -22,12 +22,18 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org if geocode_results: results = [] for result in geocode_results: + if len(result) > 2: + metadata = json.dumps(result[2]) + else: + logger.warning('Geocoding for {} without metadata'.format(username)) + metadata = '{}' + if result[1] and len(result[1]) == 2: plan = plpy.prepare("SELECT ST_SetSRID(ST_MakePoint($1, $2), 4326) as the_geom; ", ["double precision", "double precision"]) point = plpy.execute(plan, result[1], 1)[0] - results.append([result[0], point['the_geom'], None]) + results.append([result[0], point['the_geom'], metadata]) else: - results.append([result[0], None, None]) + results.append([result[0], None, metadata]) service_manager.quota_service.increment_success_service_use(len(results)) return results else: diff --git a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py index 3724bcd..dd47fda 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py @@ -16,7 +16,7 @@ HereJobStatus = namedtuple('HereJobStatus', 'total_count processed_count status' class HereMapsBulkGeocoder(HereMapsGeocoder, StreetPointBulkGeocoder): MAX_BATCH_SIZE = 1000000 # From the docs - MIN_BATCHED_SEARCH = 1000 # Under this, serial will be used + MIN_BATCHED_SEARCH = 100 # Under this, serial will be used BATCH_URL = 'https://batch.geocoder.cit.api.here.com/6.2/jobs' # https://developer.here.com/documentation/batch-geocoder/topics/read-batch-request-output.html META_COLS = ['relevance', 'matchType', 'matchCode', 'matchLevel', 'matchQualityStreet'] @@ -41,8 +41,8 @@ class HereMapsBulkGeocoder(HereMapsGeocoder, StreetPointBulkGeocoder): results = [] for search in searches: (search_id, address, city, state, country) = search - coordinates = self.geocode(searchtext=address, city=city, state=state, country=country) - results.append((search_id, coordinates, [])) + result = self.geocode_meta(searchtext=address, city=city, state=state, country=country) + results.append((search_id, result[0], result[1])) return results def _batch_geocode(self, searches): @@ -140,7 +140,11 @@ class HereMapsBulkGeocoder(HereMapsGeocoder, StreetPointBulkGeocoder): reader = csv.DictReader(root_zip.open(name), delimiter='|') for row in reader: if row['SeqNumber'] == '1': # First per requested data - results.append((row['recId'], [row['displayLongitude'], row['displayLatitude']])) + results.append((row['recId'], + [row['displayLongitude'], row['displayLatitude']], + { + 'relevance': float(row['relevance']) + })) return results diff --git a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py index 6c3b81c..8139e1a 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py @@ -65,12 +65,15 @@ class HereMapsGeocoder(Traceable): self.max_retries = service_params.get('max_retries', self.MAX_RETRIES) def geocode(self, **kwargs): + return self.geocode_meta(**kwargs)[0] + + def geocode_meta(self, **kwargs): params = {} for key, value in kwargs.iteritems(): if value and value.strip(): params[key] = value if not params: - return [] + return [[], {}] return self._execute_geocode(params) def _execute_geocode(self, params): @@ -78,10 +81,11 @@ class HereMapsGeocoder(Traceable): raise BadGeocodingParams(params) try: response = self._perform_request(params) - results = response['Response']['View'][0]['Result'][0] - return self._extract_lng_lat_from_result(results) + result = response['Response']['View'][0]['Result'][0] + return [self._extract_lng_lat_from_result(result), + self._extract_metadata_from_result(result)] except IndexError: - return [] + return [[], {}] except KeyError: raise MalformedResult() @@ -118,3 +122,8 @@ class HereMapsGeocoder(Traceable): latitude = location['DisplayPosition']['Latitude'] return [longitude, latitude] + + def _extract_metadata_from_result(self, result): + return { + 'relevance': result['Relevance'] + } diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index 52ffbe0..e842396 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -2,10 +2,9 @@ # -*- coding: utf-8 -*- from unittest import TestCase -from nose.tools import assert_raises -from nose.tools import assert_not_equal, assert_equal +from nose.tools import assert_not_equal, assert_equal, assert_true from ..helpers.integration_test_helper import IntegrationTestHelper -from ..helpers.integration_test_helper import assert_close_enough +from ..helpers.integration_test_helper import assert_close_enough, isclose class TestStreetFunctionsSetUp(TestCase): provider = None @@ -321,6 +320,16 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): assert_equal(len(response['rows']), count) assert_not_equal(response['rows'][0]['st_x'], None) + def test_relevance(self): + query = "select metadata " \ + "FROM cdb_dataservices_client.cdb_bulk_geocode_street_point(" \ + "'select 1 as cartodb_id, ''Spain'' as country, " \ + "''Barcelona'' as city, " \ + "''Plaza España 1'' as street' " \ + ", 'street', 'city', NULL, 'country')" + response = self._run_authenticated(query) + + assert_true(isclose(response['rows'][0]['metadata']['relevance'], 1)) def _run_authenticated(self, query): authenticated_query = "{}&api_key={}".format(query, From 2af92045427318f1b6d6463a21b93043996b033b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 10 Jul 2018 21:21:42 +0200 Subject: [PATCH 09/31] Relevance metadata for TomTom --- .../cartodb_services/tomtom/bulk_geocoder.py | 10 ++++---- .../cartodb_services/tomtom/geocoder.py | 25 ++++++++++++++----- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/tomtom/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/tomtom/bulk_geocoder.py index a3d8583..c8ad803 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tomtom/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/tomtom/bulk_geocoder.py @@ -38,18 +38,18 @@ class TomTomBulkGeocoder(TomTomGeocoder, StreetPointBulkGeocoder): state = state.encode('utf-8') if state else None country = country.encode('utf-8') if country else None self._logger.debug('--> Sending serial search: {}'.format(search)) - coordinates = self.geocode(searchtext=address, city=city, + result = self.geocode_meta(searchtext=address, city=city, state_province=state, country=country) self._logger.debug('--> result sent') - results.append((search_id, coordinates, [])) + results.append((search_id, result[0], result[1])) return results def _batch_geocode(self, searches): location = self._send_batch(searches) - xy_results = self._download_results(location) + full_results = self._download_results(location) results = [] - for s, r in zip(searches, xy_results): - results.append((s[0], r, [])) + for s, r in zip(searches, full_results): + results.append((s[0], r[0], r[1])) self._logger.debug('--> results: {}'.format(results)) return results diff --git a/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py index b4e82cb..b064ea5 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py @@ -18,6 +18,7 @@ ENTRY_RESULTS = 'results' ENTRY_POSITION = 'position' ENTRY_LON = 'lon' ENTRY_LAT = 'lat' +EMPTY_RESPONSE = [[], {}] class TomTomGeocoder(Traceable): @@ -62,6 +63,11 @@ class TomTomGeocoder(Traceable): @qps_retry(qps=5) def geocode(self, searchtext, city=None, state_province=None, country=None): + return self.geocode_meta(searchtext, city, state_province, country)[0] + + @qps_retry(qps=5) + def geocode_meta(self, searchtext, city=None, state_province=None, + country=None): if searchtext: searchtext = searchtext.decode('utf-8') if city: @@ -72,7 +78,7 @@ class TomTomGeocoder(Traceable): country = country.decode('utf-8') if not self._validate_input(searchtext, city, state_province, country): - return [] + return EMPTY_RESPONSE address = [] if searchtext and searchtext.strip(): @@ -98,15 +104,15 @@ class TomTomGeocoder(Traceable): # Don't raise the exception to continue with the geocoding job self._logger.error('Error connecting to TomTom geocoding server', exception=ce) - return [] + return EMPTY_RESPONSE def _parse_response(self, status_code, text): if status_code == requests.codes.ok: return self._parse_geocoder_response(text) elif status_code == requests.codes.bad_request: - return [] + return EMPTY_RESPONSE elif status_code == requests.codes.unprocessable_entity: - return [] + return EMPTY_RESPONSE else: msg = 'Unknown response {}: {}'.format(str(status_code), text) raise ServiceException(msg, None) @@ -117,7 +123,14 @@ class TomTomGeocoder(Traceable): if json_response and json_response[ENTRY_RESULTS]: result = json_response[ENTRY_RESULTS][0] - return self._extract_lng_lat_from_feature(result) + return [ + self._extract_lng_lat_from_feature(result), + self._extract_metadata_from_result(result) + ] else: - return [] + return EMPTY_RESPONSE + def _extract_metadata_from_result(self, result): + return { + 'relevance': result['score'] # TODO: normalize + } From 825e3b7ee82080b4998849d85bb7a76d8e1c2ab8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Wed, 11 Jul 2018 07:42:21 +0200 Subject: [PATCH 10/31] Relevance metadata for Mapbox --- .../cartodb_services/mapbox/bulk_geocoder.py | 19 +++----- .../cartodb_services/mapbox/geocoder.py | 46 +++++++++++++------ test/integration/test_street_functions.py | 40 +++++++++++----- 3 files changed, 68 insertions(+), 37 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py index 63f5e72..3ddd457 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py @@ -29,15 +29,10 @@ class MapboxBulkGeocoder(MapboxGeocoder, StreetPointBulkGeocoder): for search in searches: elements = self._encoded_elements(search) self._logger.debug('--> Sending serial search: {}'.format(search)) - coordinates = self._geocode_search(*elements) - results.append((search[0], coordinates, [])) - return results + result = self.geocode_meta(*elements) - def _geocode_search(self, address, city, state, country): - coordinates = self.geocode(searchtext=address, city=city, - state_province=state, country=country) - self._logger.debug('--> result sent') - return coordinates + results.append((search[0], result[0], result[1])) + return results def _encoded_elements(self, search): (search_id, address, city, state, country) = search @@ -58,11 +53,11 @@ class MapboxBulkGeocoder(MapboxGeocoder, StreetPointBulkGeocoder): frees.append(free) self._logger.debug('--> sending free search: {}'.format(frees)) - xy_results = self.geocode_free_text(frees) + full_results = self.geocode_free_text_meta(frees) results = [] - self._logger.debug('--> searches: {}; xy: {}'.format(searches, xy_results)) - for s, r in zip(searches, xy_results): - results.append((s[0], r, [])) + self._logger.debug('--> searches: {}; xy: {}'.format(searches, full_results)) + for s, r in zip(searches, full_results): + results.append((s[0], r[0], r[1])) self._logger.debug('--> results: {}'.format(results)) return results diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py index a3d6f93..5d644d9 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py @@ -22,6 +22,8 @@ ENTRY_COORDINATES = 'coordinates' ENTRY_TYPE = 'type' TYPE_POINT = 'Point' +EMPTY_RESPONSE = [[], {}] + class MapboxGeocoder(Traceable): ''' @@ -49,12 +51,16 @@ class MapboxGeocoder(Traceable): for a_json_response in json_response: if a_json_response[ENTRY_FEATURES]: feature = a_json_response[ENTRY_FEATURES][0] - result.append(self._extract_lng_lat_from_feature(feature)) + result.append([ + self._extract_lng_lat_from_feature(feature), + self._extract_metadata_from_result(feature) + ] + ) else: - result.append([]) + result.append(EMPTY_RESPONSE) return result else: - return [] + return EMPTY_RESPONSE def _extract_lng_lat_from_feature(self, feature): geometry = feature[ENTRY_GEOMETRY] @@ -67,6 +73,14 @@ class MapboxGeocoder(Traceable): latitude = location[1] return [longitude, latitude] + def _extract_metadata_from_result(self, result): + return { + 'relevance': self._normalize_relevance(float(result['relevance'])) + } + + def _normalize_relevance(self, relevance): + return 1 if relevance == 0.99 else relevance + def _validate_input(self, searchtext, city=None, state_province=None, country=None): if searchtext and searchtext.strip(): @@ -88,8 +102,13 @@ class MapboxGeocoder(Traceable): :param country: Country ISO 3166 code :return: [x, y] on success, [] on error """ + return self.geocode_meta(searchtext, city, state_province, country)[0] + + @qps_retry(qps=10) + def geocode_meta(self, searchtext, city=None, state_province=None, + country=None): if not self._validate_input(searchtext, city, state_province, country): - return [] + return EMPTY_RESPONSE address = [] if searchtext and searchtext.strip(): @@ -99,32 +118,31 @@ class MapboxGeocoder(Traceable): if state_province: address.append(normalize(state_province)) - country = [country] if country else None - free_search = ', '.join(address) - return self.geocode_free_text([free_search], country)[0] + return self.geocode_free_text_meta([free_search], country)[0] @qps_retry(qps=10) - def geocode_free_text(self, free_searches, country=None): + def geocode_free_text_meta(self, free_searches, country=None): """ :param free_searches: Free text searches :param country: Country ISO 3166 code :return: list of [x, y] on success, [] on error """ + country = [country] if country else None + try: free_search = ';'.join([self._escape(fs) for fs in free_searches]) - self._logger.debug('--> free search: {}'.format(free_search)) + self._logger.debug('--> free search: {}, country: {}'.format(free_search, country)) response = self._geocoder.forward(address=free_search.decode('utf-8'), - country=country, - limit=1) + country=country) if response.status_code == requests.codes.ok: return self._parse_geocoder_response(response.text) elif response.status_code == requests.codes.bad_request: - return [] + return EMPTY_RESPONSE elif response.status_code == requests.codes.unprocessable_entity: - return [] + return EMPTY_RESPONSE else: raise ServiceException(response.status_code, response) except requests.Timeout as te: @@ -138,7 +156,7 @@ class MapboxGeocoder(Traceable): # Don't raise the exception to continue with the geocoding job self._logger.error('Error connecting to Mapbox geocoding server', exception=ce) - return [] + return EMPTY_RESPONSE def _escape(self, free_search): # Semicolon is used to separate batch geocoding; there's no documented diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index e842396..b0c6deb 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -22,7 +22,7 @@ class TestStreetFunctionsSetUp(TestCase): 'Madrid': [-3.7037902, 40.4167754], 'Logroño, Spain': [-2.4449852, 42.4627195], 'Logroño, Argentina': [-61.6961807, -29.5031057], - 'Plaza España 1, Barcelona': [2.1482563, 41.375485] + 'Plaza España, Barcelona': [2.1482563, 41.375485] } HERE_POINTS = { @@ -37,7 +37,7 @@ class TestStreetFunctionsSetUp(TestCase): 'Madrid': [-3.70578, 40.42028], 'Logroño, Spain': [-2.45194, 42.46592], 'Logroño, Argentina': [-61.69604, -29.50425], - 'Plaza España 1, Barcelona': [2.1735699, 41.3823] # TODO: not ideal + 'Plaza España, Barcelona': [2.1735699, 41.3823] # TODO: not ideal } TOMTOM_POINTS = HERE_POINTS.copy() @@ -48,7 +48,7 @@ class TestStreetFunctionsSetUp(TestCase): 'Valladolid, Spain': [-4.72838, 41.6542], 'Madrid': [-3.70035, 40.42028], 'Logroño, Spain': [-2.44998, 42.46592], - 'Plaza España 1, Barcelona': [2.07479, 41.36818] # TODO: not ideal + 'Plaza España, Barcelona': [2.07479, 41.36818] # TODO: not ideal }) MAPBOX_POINTS = GOOGLE_POINTS.copy() @@ -59,7 +59,7 @@ class TestStreetFunctionsSetUp(TestCase): 'Valladolid, Spain': [-4.72856, 41.652251], '1902 amphitheatre parkway': [-118.03, 34.06], # TODO: huge mismatch 'Madrid': [-3.69194, 40.4167754], - 'Plaza España 1, Barcelona': [2.245969, 41.452483] # TODO: not ideal + 'Plaza España, Barcelona': [2.245969, 41.452483] # TODO: not ideal }) FIXTURE_POINTS = { @@ -69,6 +69,21 @@ class TestStreetFunctionsSetUp(TestCase): 'mapbox': MAPBOX_POINTS } + HERE_RELEVANCES = { + 'Plaza España, Barcelona': 1 + } + + MAPBOX_RELEVANCES = { + 'Plaza España, Barcelona': 0.75 + } + + RELEVANCES = { + 'here': HERE_RELEVANCES, + 'tomtom': HERE_RELEVANCES, + 'mapbox': MAPBOX_RELEVANCES, + 'google': HERE_RELEVANCES + } + def setUp(self): self.env_variables = IntegrationTestHelper.get_environment_variables() self.sql_api_url = "{0}://{1}.{2}/api/v1/sql".format( @@ -86,6 +101,8 @@ class TestStreetFunctionsSetUp(TestCase): provider = response['rows'][0]['provider'] self.fixture_points = self.FIXTURE_POINTS[provider] + self.relevances = self.RELEVANCES[provider] + def _run_authenticated(self, query): authenticated_query = "{}&api_key={}".format(query, @@ -118,12 +135,12 @@ class TestStreetFunctions(TestStreetFunctionsSetUp): def test_component_aggregation(self): query = "select st_x(the_geom), st_y(the_geom) from (" \ "select cdb_dataservices_client.cdb_geocode_street_point( " \ - "'Plaza España 1', 'Barcelona', null, 'Spain') as the_geom) _x" + "'Plaza España', 'Barcelona', null, 'Spain') as the_geom) _x" response = self._run_authenticated(query) row = response['rows'][0] x_y = [row['st_x'], row['st_y']] # Wrong coordinates (Plaza España, Madrid): [-3.7138975, 40.4256762] - assert_close_enough(x_y, self.fixture_points['Plaza España 1, Barcelona']) + assert_close_enough(x_y, self.fixture_points['Plaza España, Barcelona']) class TestBulkStreetFunctions(TestStreetFunctionsSetUp): @@ -294,18 +311,18 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): x_y_by_cartodb_id = self._x_y_by_cartodb_id(response) assert_equal(x_y_by_cartodb_id[1], x_y_by_cartodb_id[2]) - # "'Plaza España 1', 'Barcelona', null, 'Spain') as the_geom) _x" + # "'Plaza España', 'Barcelona', null, 'Spain') as the_geom) _x" def test_component_aggregation(self): query = "select cartodb_id, st_x(the_geom), st_y(the_geom) " \ "FROM cdb_dataservices_client.cdb_bulk_geocode_street_point(" \ "'select 1 as cartodb_id, ''Spain'' as country, " \ "''Barcelona'' as city, " \ - "''Plaza España 1'' as street' " \ + "''Plaza España'' as street' " \ ", 'street', 'city', NULL, 'country')" response = self._run_authenticated(query) assert_close_enough(self._x_y_by_cartodb_id(response)[1], - self.fixture_points['Plaza España 1, Barcelona']) + self.fixture_points['Plaza España, Barcelona']) def _test_known_table(self): subquery = 'select * from known_table where cartodb_id < 1100' @@ -325,11 +342,12 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): "FROM cdb_dataservices_client.cdb_bulk_geocode_street_point(" \ "'select 1 as cartodb_id, ''Spain'' as country, " \ "''Barcelona'' as city, " \ - "''Plaza España 1'' as street' " \ + "''Plaza España'' as street' " \ ", 'street', 'city', NULL, 'country')" response = self._run_authenticated(query) - assert_true(isclose(response['rows'][0]['metadata']['relevance'], 1)) + assert_true(isclose(response['rows'][0]['metadata']['relevance'], + self.relevances['Plaza España, Barcelona'])) def _run_authenticated(self, query): authenticated_query = "{}&api_key={}".format(query, From 0b2ee85c1167793cfbc4216e71f3da2e2b77a1bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Wed, 11 Jul 2018 09:30:28 +0200 Subject: [PATCH 11/31] TomTom normalization --- .../cartodb_services/tomtom/geocoder.py | 7 +++++- test/integration/test_street_functions.py | 22 +++++++++++++------ 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py index b064ea5..4b3843b 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py @@ -4,6 +4,7 @@ import json import requests from uritemplate import URITemplate +from math import tanh from cartodb_services.metrics import Traceable from cartodb_services.tools.exceptions import ServiceException from cartodb_services.tools.qps import qps_retry @@ -20,6 +21,7 @@ ENTRY_LON = 'lon' ENTRY_LAT = 'lat' EMPTY_RESPONSE = [[], {}] +SCORE_NORMALIZATION_FACTOR = 0.15 class TomTomGeocoder(Traceable): ''' @@ -132,5 +134,8 @@ class TomTomGeocoder(Traceable): def _extract_metadata_from_result(self, result): return { - 'relevance': result['score'] # TODO: normalize + 'relevance': self._normalize_score(result['score']) } + + def _normalize_score(self, score): + return tanh(score * SCORE_NORMALIZATION_FACTOR) diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index b0c6deb..79af788 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -73,15 +73,21 @@ class TestStreetFunctionsSetUp(TestCase): 'Plaza España, Barcelona': 1 } - MAPBOX_RELEVANCES = { + MAPBOX_RELEVANCES = HERE_RELEVANCES.copy() + MAPBOX_RELEVANCES.update({ 'Plaza España, Barcelona': 0.75 - } + }) + + TOMTOM_RELEVANCES = MAPBOX_RELEVANCES.copy() + TOMTOM_RELEVANCES.update({ + 'Plaza España, Barcelona': 0.85 + }) RELEVANCES = { + 'google': HERE_RELEVANCES, 'here': HERE_RELEVANCES, - 'tomtom': HERE_RELEVANCES, - 'mapbox': MAPBOX_RELEVANCES, - 'google': HERE_RELEVANCES + 'tomtom': TOMTOM_RELEVANCES, + 'mapbox': MAPBOX_RELEVANCES } def setUp(self): @@ -346,8 +352,10 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): ", 'street', 'city', NULL, 'country')" response = self._run_authenticated(query) - assert_true(isclose(response['rows'][0]['metadata']['relevance'], - self.relevances['Plaza España, Barcelona'])) + relevance = response['rows'][0]['metadata']['relevance'] + expected_relevance = self.relevances['Plaza España, Barcelona'] + assert_true(isclose(relevance, expected_relevance, 0.05), + '{} not close to {}'.format(relevance, expected_relevance)) def _run_authenticated(self, query): authenticated_query = "{}&api_key={}".format(query, From d46d51c3bbf49608b038499015f4ae66ee1ca7c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Wed, 11 Jul 2018 11:43:54 +0200 Subject: [PATCH 12/31] Relevance metadata for Google --- .../cartodb_services/google/bulk_geocoder.py | 29 ++++-------- .../cartodb_services/google/geocoder.py | 46 ++++++++++++++++--- test/integration/test_street_functions.py | 9 +++- 3 files changed, 56 insertions(+), 28 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py index e59e408..42a60d9 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py @@ -6,8 +6,7 @@ from cartodb_services.google import GoogleMapsGeocoder def async_geocoder(geocoder, address, components): - results = geocoder.geocode(address=address, components=components) - return results if results else [] + return geocoder.geocode(address=address, components=components) class GoogleMapsBulkGeocoder(GoogleMapsGeocoder, StreetPointBulkGeocoder): @@ -26,12 +25,9 @@ class GoogleMapsBulkGeocoder(GoogleMapsGeocoder, StreetPointBulkGeocoder): results = [] for search in searches: (cartodb_id, street, city, state, country) = search - address = compose_address(street, city, state, country) - components = self._build_optional_parameters(city, state, country) - result = self.geocoder.geocode(address=address, components=components) - lng_lat = self._extract_lng_lat_from_result(result[0]) if result else [] - self._logger.debug('--> lng_lat: {}'.format(lng_lat)) - results.append((cartodb_id, lng_lat, [])) + lng_lat, metadata = self.geocode_meta(street, city, state, country) + self._logger.debug('--> lng_lat: {}. metadata: {}'.format(lng_lat, metadata)) + results.append((cartodb_id, lng_lat, metadata)) return results def _batch_geocode(self, searches): @@ -39,16 +35,13 @@ class GoogleMapsBulkGeocoder(GoogleMapsGeocoder, StreetPointBulkGeocoder): pool = Pool(processes=self.PARALLEL_PROCESSES) for search in searches: (cartodb_id, street, city, state, country) = search - components = self._build_optional_parameters(city, state, country) - # Geocoding works better if components are also inside the address + self._logger.debug('async geocoding --> {}'.format(search)) address = compose_address(street, city, state, country) if address: - self._logger.debug('async geocoding --> {} {}'.format(address.encode('utf-8'), components)) + components = self._build_optional_parameters(city, state, country) result = pool.apply_async(async_geocoder, (self.geocoder, address, components)) - else: - result = [] - bulk_results[cartodb_id] = result + bulk_results[cartodb_id] = result pool.close() pool.join() @@ -56,13 +49,12 @@ class GoogleMapsBulkGeocoder(GoogleMapsGeocoder, StreetPointBulkGeocoder): results = [] for cartodb_id, bulk_result in bulk_results.items(): try: - result = bulk_result.get() + lng_lat, metadata = self._process_results(bulk_result.get()) except Exception as e: self._logger.error('Error at Google async_geocoder', e) - result = [] + lng_lat, metadata = [[], {}] - lng_lat = self._extract_lng_lat_from_result(result[0]) if result else [] - results.append((cartodb_id, lng_lat, [])) + results.append((cartodb_id, lng_lat, metadata)) return results except KeyError as e: self._logger.error('KeyError error', exception=e) @@ -70,4 +62,3 @@ class GoogleMapsBulkGeocoder(GoogleMapsGeocoder, StreetPointBulkGeocoder): except Exception as e: self._logger.error('General error', exception=e) raise e - diff --git a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py index aa8877c..b07dc6b 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py @@ -8,6 +8,15 @@ from cartodb_services.geocoder import compose_address from cartodb_services.google.exceptions import InvalidGoogleCredentials from client_factory import GoogleMapsClientFactory +EMPTY_RESPONSE = [[], {}] +PARTIAL_FACTOR = 0.8 +RELEVANCE_BY_LOCATION_TYPE = { + 'ROOFTOP': 1, + 'GEOMETRIC_CENTER': 0.9, + 'RANGE_INTERPOLATED': 0.8, + 'APPROXIMATE': 0.7 +} + class GoogleMapsGeocoder(): @@ -19,26 +28,49 @@ class GoogleMapsGeocoder(): self.geocoder = GoogleMapsClientFactory.get(self.client_id, self.client_secret, self.channel) self._logger = logger - def geocode(self, searchtext, city=None, state=None, - country=None): + def geocode(self, searchtext, city=None, state=None, country=None): + return self.geocode_meta(searchtext, city, state, country)[0] + + def geocode_meta(self, searchtext, city=None, state=None, country=None): try: address = compose_address(searchtext, city, state, country) opt_params = self._build_optional_parameters(city, state, country) results = self.geocoder.geocode(address=address, components=opt_params) - if results: - return self._extract_lng_lat_from_result(results[0]) - else: - return [] - except KeyError: + return self._process_results(results) + except KeyError as e: + self._logger.error('params: {}, {}, {}, {}'.format( + searchtext.encode('utf-8'), city.encode('utf-8'), + state.encode('utf-8'), country.encode('utf-8') + ), e) raise MalformedResult() + def _process_results(self, results): + if results: + self._logger.debug('--> results: {}'.format(results[0])) + return [ + self._extract_lng_lat_from_result(results[0]), + self._extract_metadata_from_result(results[0]) + ] + else: + return EMPTY_RESPONSE + def _extract_lng_lat_from_result(self, result): location = result['geometry']['location'] longitude = location['lng'] latitude = location['lat'] return [longitude, latitude] + def _extract_metadata_from_result(self, result): + location_type = result['geometry']['location_type'] + base_relevance = RELEVANCE_BY_LOCATION_TYPE[location_type] + partial_match = result.get('partial_match', False) + partial_factor = PARTIAL_FACTOR if partial_match else 1 + return { + 'relevance': base_relevance * partial_factor + } + + def _build_optional_parameters(self, city=None, state=None, country=None): optional_params = {} diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index 79af788..cca42ca 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -83,8 +83,13 @@ class TestStreetFunctionsSetUp(TestCase): 'Plaza España, Barcelona': 0.85 }) + GOOGLE_RELEVANCES = HERE_RELEVANCES.copy() + GOOGLE_RELEVANCES.update({ + 'Plaza España, Barcelona': 0.9 + }) + RELEVANCES = { - 'google': HERE_RELEVANCES, + 'google': GOOGLE_RELEVANCES, 'here': HERE_RELEVANCES, 'tomtom': TOMTOM_RELEVANCES, 'mapbox': MAPBOX_RELEVANCES @@ -331,7 +336,7 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): self.fixture_points['Plaza España, Barcelona']) def _test_known_table(self): - subquery = 'select * from known_table where cartodb_id < 1100' + subquery = 'select * from unknown_table where cartodb_id < 1100' subquery_count = 'select count(1) from ({}) _x'.format(subquery) count = self._run_authenticated(subquery_count)['rows'][0]['count'] From da78b0bc654986679a9599577a2740b6822cbdc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Wed, 11 Jul 2018 12:28:39 +0200 Subject: [PATCH 13/31] Fix batching with negatives cartodb_id --- client/cdb_dataservices_client--0.25.0.sql | 13 +++++++------ client/sql/21_bulk_geocoding_functions.sql | 13 +++++++------ test/integration/test_street_functions.py | 9 +++++++-- 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/client/cdb_dataservices_client--0.25.0.sql b/client/cdb_dataservices_client--0.25.0.sql index 2bed351..af58ce2 100644 --- a/client/cdb_dataservices_client--0.25.0.sql +++ b/client/cdb_dataservices_client--0.25.0.sql @@ -1995,6 +1995,8 @@ DECLARE enough_quota boolean; remaining_quota integer; max_batch_size integer; + max_cartodb_id integer; + min_cartodb_id integer; cartodb_id_batch integer; batches_n integer; @@ -2020,7 +2022,8 @@ BEGIN batch_size := MAX_SAFE_BATCH_SIZE; END IF; - EXECUTE format('SELECT COUNT(1) from (%s) _x', query) INTO query_row_count; + EXECUTE format('SELECT count(1), max(cartodb_id), min(cartodb_id), ceil((max(cartodb_id) + 1 - min(cartodb_id))::float/%s) FROM (%s) _x', batch_size, query) + INTO query_row_count, max_cartodb_id, min_cartodb_id, batches_n; RAISE DEBUG 'cdb_bulk_geocode_street_point --> query_row_count: %; query: %; country: %; state: %; city: %; street: %', query_row_count, query, country_column, state_column, city_column, street_column; @@ -2029,8 +2032,6 @@ BEGIN RAISE EXCEPTION 'Remaining quota: %. Estimated cost: %', remaining_quota, query_row_count; END IF; - EXECUTE format('SELECT ceil(max(cartodb_id)::float/%s) FROM (%s) _x', batch_size, query) INTO batches_n; - RAISE DEBUG 'batches_n: %', batches_n; temp_table_name := 'bulk_geocode_street_' || md5(random()::text); @@ -2052,13 +2053,13 @@ BEGIN 'WITH geocoding_data as (' || ' SELECT ' || ' json_build_object(''id'', cartodb_id, ''address'', %s, ''city'', %s, ''state'', %s, ''country'', %s) as data , ' || - ' floor((cartodb_id-1)::float/$1) as batch' || + ' floor((cartodb_id - $1)::float/$2) as batch' || ' FROM (%s) _x' || ') ' || 'INSERT INTO %s SELECT (cdb_dataservices_client._cdb_bulk_geocode_street_point(jsonb_agg(data))).* ' || 'FROM geocoding_data ' || - 'WHERE batch = $2', street_column, city_column, state_column, country_column, query, temp_table_name) - USING batch_size, cartodb_id_batch; + 'WHERE batch = $3', street_column, city_column, state_column, country_column, query, temp_table_name) + USING min_cartodb_id, batch_size, cartodb_id_batch; GET DIAGNOSTICS current_row_count = ROW_COUNT; RAISE DEBUG 'Batch % --> %', cartodb_id_batch, current_row_count; diff --git a/client/sql/21_bulk_geocoding_functions.sql b/client/sql/21_bulk_geocoding_functions.sql index f9ba824..7ab3407 100644 --- a/client/sql/21_bulk_geocoding_functions.sql +++ b/client/sql/21_bulk_geocoding_functions.sql @@ -6,6 +6,8 @@ DECLARE enough_quota boolean; remaining_quota integer; max_batch_size integer; + max_cartodb_id integer; + min_cartodb_id integer; cartodb_id_batch integer; batches_n integer; @@ -31,7 +33,8 @@ BEGIN batch_size := MAX_SAFE_BATCH_SIZE; END IF; - EXECUTE format('SELECT COUNT(1) from (%s) _x', query) INTO query_row_count; + EXECUTE format('SELECT count(1), max(cartodb_id), min(cartodb_id), ceil((max(cartodb_id) + 1 - min(cartodb_id))::float/%s) FROM (%s) _x', batch_size, query) + INTO query_row_count, max_cartodb_id, min_cartodb_id, batches_n; RAISE DEBUG 'cdb_bulk_geocode_street_point --> query_row_count: %; query: %; country: %; state: %; city: %; street: %', query_row_count, query, country_column, state_column, city_column, street_column; @@ -40,8 +43,6 @@ BEGIN RAISE EXCEPTION 'Remaining quota: %. Estimated cost: %', remaining_quota, query_row_count; END IF; - EXECUTE format('SELECT ceil(max(cartodb_id)::float/%s) FROM (%s) _x', batch_size, query) INTO batches_n; - RAISE DEBUG 'batches_n: %', batches_n; temp_table_name := 'bulk_geocode_street_' || md5(random()::text); @@ -63,13 +64,13 @@ BEGIN 'WITH geocoding_data as (' || ' SELECT ' || ' json_build_object(''id'', cartodb_id, ''address'', %s, ''city'', %s, ''state'', %s, ''country'', %s) as data , ' || - ' floor((cartodb_id-1)::float/$1) as batch' || + ' floor((cartodb_id - $1)::float/$2) as batch' || ' FROM (%s) _x' || ') ' || 'INSERT INTO %s SELECT (cdb_dataservices_client._cdb_bulk_geocode_street_point(jsonb_agg(data))).* ' || 'FROM geocoding_data ' || - 'WHERE batch = $2', street_column, city_column, state_column, country_column, query, temp_table_name) - USING batch_size, cartodb_id_batch; + 'WHERE batch = $3', street_column, city_column, state_column, country_column, query, temp_table_name) + USING min_cartodb_id, batch_size, cartodb_id_batch; GET DIAGNOSTICS current_row_count = ROW_COUNT; RAISE DEBUG 'Batch % --> %', cartodb_id_batch, current_row_count; diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index cca42ca..43186b9 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -287,11 +287,14 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): Useful just to test a good batch size """ n = 110 + first_cartodb_id = -1 + first_street_number = 1 batch_size = 'NULL' # NULL for optimal streets = [] for i in range(0, n): streets.append('{{"cartodb_id": {}, "address": "{} Yonge Street, ' \ - 'Toronto, Canada"}}'.format(i, i)) + 'Toronto, Canada"}}'.format(first_cartodb_id + i, + first_street_number + i)) query = "select *, st_x(the_geom), st_y(the_geom) " \ "FROM cdb_dataservices_client.cdb_bulk_geocode_street_point( " \ @@ -300,7 +303,9 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): "]''::jsonb) as (cartodb_id integer, address text)', " \ "'address', null, null, null, {})".format(','.join(streets), batch_size) response = self._run_authenticated(query) - assert_equal(n - 1, len(response['rows'])) + assert_equal(n, len(response['rows'])) + for row in response['rows']: + assert_not_equal(row['st_x'], None) def test_missing_components_on_private_function(self): query = "SELECT _cdb_bulk_geocode_street_point(" \ From b77974258564f2ff2b053438a442283dc24a5a1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Wed, 11 Jul 2018 12:51:56 +0200 Subject: [PATCH 14/31] Fix logging on error --- .../cartodb_services/cartodb_services/google/geocoder.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py index b07dc6b..491ce0f 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py @@ -32,17 +32,14 @@ class GoogleMapsGeocoder(): return self.geocode_meta(searchtext, city, state, country)[0] def geocode_meta(self, searchtext, city=None, state=None, country=None): + address = compose_address(searchtext, city, state, country) try: - address = compose_address(searchtext, city, state, country) opt_params = self._build_optional_parameters(city, state, country) results = self.geocoder.geocode(address=address, components=opt_params) return self._process_results(results) except KeyError as e: - self._logger.error('params: {}, {}, {}, {}'.format( - searchtext.encode('utf-8'), city.encode('utf-8'), - state.encode('utf-8'), country.encode('utf-8') - ), e) + self._logger.error('address: {}'.format(address), e) raise MalformedResult() def _process_results(self, results): From 67fee1cce897bafe6a60ea5f6a18790055208212 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Wed, 11 Jul 2018 13:59:48 +0200 Subject: [PATCH 15/31] Precision metadata for Google --- .../cartodb_services/__init__.py | 3 + .../cartodb_services/google/geocoder.py | 10 ++- test/integration/test_street_functions.py | 75 ++++++++++++------- 3 files changed, 61 insertions(+), 27 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/__init__.py b/server/lib/python/cartodb_services/cartodb_services/__init__.py index ef5caa4..dd364b4 100644 --- a/server/lib/python/cartodb_services/cartodb_services/__init__.py +++ b/server/lib/python/cartodb_services/cartodb_services/__init__.py @@ -35,3 +35,6 @@ def _reset(): GD = None from geocoder import run_street_point_geocoder, StreetPointBulkGeocoder + +PRECISION_PRECISE = 'precise' +PRECISION_INTERPOLATED = 'interpolated' diff --git a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py index 491ce0f..f4cd0a5 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py @@ -4,6 +4,7 @@ from urlparse import parse_qs from exceptions import MalformedResult +from cartodb_services import PRECISION_PRECISE, PRECISION_INTERPOLATED from cartodb_services.geocoder import compose_address from cartodb_services.google.exceptions import InvalidGoogleCredentials from client_factory import GoogleMapsClientFactory @@ -16,6 +17,12 @@ RELEVANCE_BY_LOCATION_TYPE = { 'RANGE_INTERPOLATED': 0.8, 'APPROXIMATE': 0.7 } +PRECISION_BY_LOCATION_TYPE = { + 'ROOFTOP': PRECISION_PRECISE, + 'GEOMETRIC_CENTER': PRECISION_PRECISE, + 'RANGE_INTERPOLATED': PRECISION_INTERPOLATED, + 'APPROXIMATE': PRECISION_INTERPOLATED +} class GoogleMapsGeocoder(): @@ -64,7 +71,8 @@ class GoogleMapsGeocoder(): partial_match = result.get('partial_match', False) partial_factor = PARTIAL_FACTOR if partial_match else 1 return { - 'relevance': base_relevance * partial_factor + 'relevance': base_relevance * partial_factor, + 'precision': PRECISION_BY_LOCATION_TYPE[location_type] } diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index 43186b9..1b8430d 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -69,30 +69,38 @@ class TestStreetFunctionsSetUp(TestCase): 'mapbox': MAPBOX_POINTS } - HERE_RELEVANCES = { - 'Plaza España, Barcelona': 1 + GOOGLE_METADATAS = { + 'Plaza España, Barcelona': { + 'relevance': 0.9, 'precision': 'precise' + }, + 'Santiago Rusiñol 123, Valladolid': { + 'relevance': 0.8, 'precision': 'interpolated' + } } - MAPBOX_RELEVANCES = HERE_RELEVANCES.copy() - MAPBOX_RELEVANCES.update({ - 'Plaza España, Barcelona': 0.75 - }) + HERE_METADATAS = { + 'Plaza España, Barcelona': { + 'relevance': 1 + } + } - TOMTOM_RELEVANCES = MAPBOX_RELEVANCES.copy() - TOMTOM_RELEVANCES.update({ - 'Plaza España, Barcelona': 0.85 - }) + TOMTOM_METADATAS = { + 'Plaza España, Barcelona': { + 'relevance': 0.85 + } + } - GOOGLE_RELEVANCES = HERE_RELEVANCES.copy() - GOOGLE_RELEVANCES.update({ - 'Plaza España, Barcelona': 0.9 - }) + MAPBOX_METADATAS = { + 'Plaza España, Barcelona': { + 'relevance': 0.75 + } + } - RELEVANCES = { - 'google': GOOGLE_RELEVANCES, - 'here': HERE_RELEVANCES, - 'tomtom': TOMTOM_RELEVANCES, - 'mapbox': MAPBOX_RELEVANCES + METADATAS = { + 'google': GOOGLE_METADATAS, + 'here': HERE_METADATAS, + 'tomtom': TOMTOM_METADATAS, + 'mapbox': MAPBOX_METADATAS } def setUp(self): @@ -112,7 +120,7 @@ class TestStreetFunctionsSetUp(TestCase): provider = response['rows'][0]['provider'] self.fixture_points = self.FIXTURE_POINTS[provider] - self.relevances = self.RELEVANCES[provider] + self.metadata = self.METADATAS[provider] def _run_authenticated(self, query): @@ -353,19 +361,25 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): assert_equal(len(response['rows']), count) assert_not_equal(response['rows'][0]['st_x'], None) - def test_relevance(self): + def test_metadata(self): query = "select metadata " \ "FROM cdb_dataservices_client.cdb_bulk_geocode_street_point(" \ "'select 1 as cartodb_id, ''Spain'' as country, " \ "''Barcelona'' as city, " \ - "''Plaza España'' as street' " \ + "''Plaza España'' as street " \ + "UNION " \ + "select 2 as cartodb_id, ''Spain'' as country, " \ + "''Valladolid'' as city, " \ + "''Calle Santiago Rusiñol 123'' as street' " \ ", 'street', 'city', NULL, 'country')" response = self._run_authenticated(query) - relevance = response['rows'][0]['metadata']['relevance'] - expected_relevance = self.relevances['Plaza España, Barcelona'] - assert_true(isclose(relevance, expected_relevance, 0.05), - '{} not close to {}'.format(relevance, expected_relevance)) + expected = [ + self.metadata['Plaza España, Barcelona'], + self.metadata['Santiago Rusiñol 123, Valladolid'] + ] + for r, e in zip(response['rows'], expected): + self.assert_metadata(r['metadata'], e) def _run_authenticated(self, query): authenticated_query = "{}&api_key={}".format(query, @@ -384,3 +398,12 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): assert_equal(len(points_a_by_cartodb_id), len(points_b_by_cartodb_id)) for cartodb_id, point in points_a_by_cartodb_id.iteritems(): assert_close_enough(point, points_b_by_cartodb_id[cartodb_id]) + + @staticmethod + def assert_metadata(metadata, expected): + relevance = metadata['relevance'] + expected_relevance = expected['relevance'] + assert_true(isclose(relevance, expected_relevance, 0.05), + '{} not close to {}'.format(relevance, expected_relevance)) + + assert_equal(metadata['precision'], expected['precision']) From dbb4f9204aabd2ea8a9cd78ab753f12607006d26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Wed, 11 Jul 2018 15:30:51 +0200 Subject: [PATCH 16/31] Precision metadata for HERE --- .../cartodb_services/here/geocoder.py | 16 ++++++++++++++-- test/integration/test_street_functions.py | 19 +++++++++---------- 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py index 8139e1a..44d695d 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py @@ -6,8 +6,14 @@ import requests from requests.adapters import HTTPAdapter from exceptions import * +from cartodb_services import PRECISION_PRECISE, PRECISION_INTERPOLATED from cartodb_services.metrics import Traceable +PRECISION_BY_MATCH_TYPE = { + 'pointAddress': PRECISION_PRECISE, + 'interpolated': PRECISION_INTERPOLATED +} + class HereMapsGeocoder(Traceable): 'A Here Maps Geocoder wrapper for python' @@ -82,11 +88,13 @@ class HereMapsGeocoder(Traceable): try: response = self._perform_request(params) result = response['Response']['View'][0]['Result'][0] + self._logger.debug('--> Result: {}'.format(result)) return [self._extract_lng_lat_from_result(result), self._extract_metadata_from_result(result)] except IndexError: return [[], {}] - except KeyError: + except KeyError as e: + self._logger.error('params: {}'.format(params), e) raise MalformedResult() def _perform_request(self, params): @@ -124,6 +132,10 @@ class HereMapsGeocoder(Traceable): return [longitude, latitude] def _extract_metadata_from_result(self, result): + # See https://stackoverflow.com/questions/51285622/missing-matchtype-at-here-geocoding-responses + precision = PRECISION_BY_MATCH_TYPE[ + result.get('MatchType', 'pointAddress')] return { - 'relevance': result['Relevance'] + 'relevance': result['Relevance'], + 'precision': precision } diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index 1b8430d..cfc4547 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -70,18 +70,17 @@ class TestStreetFunctionsSetUp(TestCase): } GOOGLE_METADATAS = { - 'Plaza España, Barcelona': { - 'relevance': 0.9, 'precision': 'precise' - }, - 'Santiago Rusiñol 123, Valladolid': { - 'relevance': 0.8, 'precision': 'interpolated' - } + 'Plaza España, Barcelona': + {'relevance': 0.9, 'precision': 'precise'}, + 'Santiago Rusiñol 123, Valladolid': + {'relevance': 0.8, 'precision': 'interpolated'} } HERE_METADATAS = { - 'Plaza España, Barcelona': { - 'relevance': 1 - } + 'Plaza España, Barcelona': + {'relevance': 1, 'precision': 'precise'}, + 'Santiago Rusiñol 123, Valladolid': + {'relevance': 0.89, 'precision': 'precise'} # Wrong. See https://stackoverflow.com/questions/51285622/missing-matchtype-at-here-geocoding-responses } TOMTOM_METADATAS = { @@ -98,7 +97,7 @@ class TestStreetFunctionsSetUp(TestCase): METADATAS = { 'google': GOOGLE_METADATAS, - 'here': HERE_METADATAS, + 'heremaps': HERE_METADATAS, 'tomtom': TOMTOM_METADATAS, 'mapbox': MAPBOX_METADATAS } From 4123a4c4425e96996c1c30da7446aaefaff9e850 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Wed, 11 Jul 2018 19:09:02 +0200 Subject: [PATCH 17/31] Precision metadata for TomTom --- .../cartodb_services/tomtom/geocoder.py | 10 +++++++++- test/integration/test_street_functions.py | 11 ++++++----- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py index 4b3843b..42970f8 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py @@ -5,6 +5,7 @@ import json import requests from uritemplate import URITemplate from math import tanh +from cartodb_services import PRECISION_PRECISE, PRECISION_INTERPOLATED from cartodb_services.metrics import Traceable from cartodb_services.tools.exceptions import ServiceException from cartodb_services.tools.qps import qps_retry @@ -22,6 +23,7 @@ ENTRY_LAT = 'lat' EMPTY_RESPONSE = [[], {}] SCORE_NORMALIZATION_FACTOR = 0.15 +PRECISION_SCORE_THRESHOLD = 0.5 class TomTomGeocoder(Traceable): ''' @@ -133,9 +135,15 @@ class TomTomGeocoder(Traceable): return EMPTY_RESPONSE def _extract_metadata_from_result(self, result): + score = self._normalize_score(result['score']) return { - 'relevance': self._normalize_score(result['score']) + 'relevance': score, + 'precision': self._precision_from_score(score) } def _normalize_score(self, score): return tanh(score * SCORE_NORMALIZATION_FACTOR) + + def _precision_from_score(self, score): + return PRECISION_PRECISE \ + if score > PRECISION_SCORE_THRESHOLD else PRECISION_INTERPOLATED diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index cfc4547..970e1f7 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -73,7 +73,7 @@ class TestStreetFunctionsSetUp(TestCase): 'Plaza España, Barcelona': {'relevance': 0.9, 'precision': 'precise'}, 'Santiago Rusiñol 123, Valladolid': - {'relevance': 0.8, 'precision': 'interpolated'} + {'relevance': 0.56, 'precision': 'interpolated'} } HERE_METADATAS = { @@ -84,9 +84,10 @@ class TestStreetFunctionsSetUp(TestCase): } TOMTOM_METADATAS = { - 'Plaza España, Barcelona': { - 'relevance': 0.85 - } + 'Plaza España, Barcelona': + {'relevance': 0.85, 'precision': 'precise'}, + 'Santiago Rusiñol 123, Valladolid': + {'relevance': 0.448283494533, 'precision': 'interpolated'} } MAPBOX_METADATAS = { @@ -369,7 +370,7 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): "UNION " \ "select 2 as cartodb_id, ''Spain'' as country, " \ "''Valladolid'' as city, " \ - "''Calle Santiago Rusiñol 123'' as street' " \ + "''Santiago Rusiñol 123'' as street' " \ ", 'street', 'city', NULL, 'country')" response = self._run_authenticated(query) From 6e78da55b27d2f814e23a6645cb6b956998f5949 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Wed, 11 Jul 2018 19:28:16 +0200 Subject: [PATCH 18/31] Precision metadata for Mapbox --- .../cartodb_services/cartodb_services/mapbox/geocoder.py | 9 ++++++++- test/integration/test_street_functions.py | 7 ++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py index 5d644d9..9eb5e4e 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py @@ -5,6 +5,7 @@ Python client for the Mapbox Geocoder service. import json import requests from mapbox import Geocoder +from cartodb_services import PRECISION_PRECISE, PRECISION_INTERPOLATED from cartodb_services.metrics import Traceable from cartodb_services.tools.exceptions import ServiceException from cartodb_services.tools.qps import qps_retry @@ -74,8 +75,14 @@ class MapboxGeocoder(Traceable): return [longitude, latitude] def _extract_metadata_from_result(self, result): + if result[ENTRY_GEOMETRY].get('interpolated', False): + precision = PRECISION_INTERPOLATED + else: + precision = PRECISION_PRECISE + return { - 'relevance': self._normalize_relevance(float(result['relevance'])) + 'relevance': self._normalize_relevance(float(result['relevance'])), + 'precision': precision } def _normalize_relevance(self, relevance): diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index 970e1f7..c8abeae 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -91,9 +91,10 @@ class TestStreetFunctionsSetUp(TestCase): } MAPBOX_METADATAS = { - 'Plaza España, Barcelona': { - 'relevance': 0.75 - } + 'Plaza España, Barcelona': + {'relevance': 0.666, 'precision': 'precise'}, + 'Santiago Rusiñol 123, Valladolid': + {'relevance': 0.666, 'precision': 'precise'} # TODO: wrong } METADATAS = { From f2197d4b2ae3e35b4ac12ec1fea31fd1082f4fa2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 16 Jul 2018 11:34:33 +0200 Subject: [PATCH 19/31] match_types for Google metadata --- .../cartodb_services/google/geocoder.py | 17 ++++++++++++++++- test/integration/test_street_functions.py | 6 ++++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py index f4cd0a5..74d8f53 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py @@ -23,6 +23,18 @@ PRECISION_BY_LOCATION_TYPE = { 'RANGE_INTERPOLATED': PRECISION_INTERPOLATED, 'APPROXIMATE': PRECISION_INTERPOLATED } +MATCH_TYPE_BY_MATCH_LEVEL = { + 'point_of_interest': 'point_of_interest', + 'country': 'country', + 'administrative_area_level_1': 'state', + 'administrative_area_level_2': 'county', + 'locality': 'locality', + 'sublocality': 'district', + 'street_address': 'street', + 'intersection': 'intersection', + 'street_number': 'street_number', + 'postal_code': 'postal_code' +} class GoogleMapsGeocoder(): @@ -70,9 +82,12 @@ class GoogleMapsGeocoder(): base_relevance = RELEVANCE_BY_LOCATION_TYPE[location_type] partial_match = result.get('partial_match', False) partial_factor = PARTIAL_FACTOR if partial_match else 1 + match_types = [MATCH_TYPE_BY_MATCH_LEVEL.get(match_level, None) + for match_level in result['types']] return { 'relevance': base_relevance * partial_factor, - 'precision': PRECISION_BY_LOCATION_TYPE[location_type] + 'precision': PRECISION_BY_LOCATION_TYPE[location_type], + 'match_types': filter(None, match_types) } diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index c8abeae..7575b7c 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -71,9 +71,9 @@ class TestStreetFunctionsSetUp(TestCase): GOOGLE_METADATAS = { 'Plaza España, Barcelona': - {'relevance': 0.9, 'precision': 'precise'}, + {'relevance': 0.9, 'precision': 'precise', 'match_types': ['point_of_interest']}, 'Santiago Rusiñol 123, Valladolid': - {'relevance': 0.56, 'precision': 'interpolated'} + {'relevance': 0.56, 'precision': 'interpolated', 'match_types': ['locality']} } HERE_METADATAS = { @@ -408,3 +408,5 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): '{} not close to {}'.format(relevance, expected_relevance)) assert_equal(metadata['precision'], expected['precision']) + + assert_equal(metadata['match_types'], expected['match_types']) From 0b635377ef542b86d40c52a081829b224b4f38e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 16 Jul 2018 11:52:25 +0200 Subject: [PATCH 20/31] match_types for HERE metadata --- .../cartodb_services/here/geocoder.py | 16 +++++++++++++++- test/integration/test_street_functions.py | 4 ++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py index 44d695d..d8f7f41 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py @@ -13,6 +13,18 @@ PRECISION_BY_MATCH_TYPE = { 'pointAddress': PRECISION_PRECISE, 'interpolated': PRECISION_INTERPOLATED } +MATCH_TYPE_BY_MATCH_LEVEL = { + 'landmark': 'point_of_interest', + 'country': 'country', + 'state': 'state', + 'county': 'county', + 'city': 'locality', + 'district': 'district', + 'street': 'street', + 'intersection': 'intersection', + 'houseNumber': 'street_number', + 'postalCode': 'postal_code' +} class HereMapsGeocoder(Traceable): @@ -135,7 +147,9 @@ class HereMapsGeocoder(Traceable): # See https://stackoverflow.com/questions/51285622/missing-matchtype-at-here-geocoding-responses precision = PRECISION_BY_MATCH_TYPE[ result.get('MatchType', 'pointAddress')] + match_type = MATCH_TYPE_BY_MATCH_LEVEL.get(result['MatchLevel'], None) return { 'relevance': result['Relevance'], - 'precision': precision + 'precision': precision, + 'match_types': [match_type] if match_type else [] } diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index 7575b7c..bf7672e 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -78,9 +78,9 @@ class TestStreetFunctionsSetUp(TestCase): HERE_METADATAS = { 'Plaza España, Barcelona': - {'relevance': 1, 'precision': 'precise'}, + {'relevance': 1, 'precision': 'precise', 'match_types': ['street']}, 'Santiago Rusiñol 123, Valladolid': - {'relevance': 0.89, 'precision': 'precise'} # Wrong. See https://stackoverflow.com/questions/51285622/missing-matchtype-at-here-geocoding-responses + {'relevance': 0.89, 'precision': 'precise', 'match_types': ['street']} # Wrong. See https://stackoverflow.com/questions/51285622/missing-matchtype-at-here-geocoding-responses } TOMTOM_METADATAS = { From 0a92ae14452331fc0a3fee960e06a2daecc8aee4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 16 Jul 2018 12:01:55 +0200 Subject: [PATCH 21/31] match_types for TomTom metadata --- .../cartodb_services/tomtom/geocoder.py | 11 ++++++++++- test/integration/test_street_functions.py | 4 ++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py index 42970f8..09ed69d 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py @@ -24,6 +24,13 @@ EMPTY_RESPONSE = [[], {}] SCORE_NORMALIZATION_FACTOR = 0.15 PRECISION_SCORE_THRESHOLD = 0.5 +MATCH_TYPE_BY_MATCH_LEVEL = { + 'POI': 'point_of_interest', + 'Street': 'street', + 'Address Range': 'street', + 'Cross Street': 'intersection', + 'Point Address': 'street_number' +} class TomTomGeocoder(Traceable): ''' @@ -136,9 +143,11 @@ class TomTomGeocoder(Traceable): def _extract_metadata_from_result(self, result): score = self._normalize_score(result['score']) + match_type = MATCH_TYPE_BY_MATCH_LEVEL.get(result['type'], None) return { 'relevance': score, - 'precision': self._precision_from_score(score) + 'precision': self._precision_from_score(score), + 'match_types': [match_type] if match_type else [] } def _normalize_score(self, score): diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index bf7672e..1c05058 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -85,9 +85,9 @@ class TestStreetFunctionsSetUp(TestCase): TOMTOM_METADATAS = { 'Plaza España, Barcelona': - {'relevance': 0.85, 'precision': 'precise'}, + {'relevance': 0.85, 'precision': 'precise', 'match_types': ['street']}, 'Santiago Rusiñol 123, Valladolid': - {'relevance': 0.448283494533, 'precision': 'interpolated'} + {'relevance': 0.448283494533, 'precision': 'interpolated', 'match_types': ['street']} } MAPBOX_METADATAS = { From 080de34163c3fd9b3793c357131bec8f19dedb7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 16 Jul 2018 12:11:40 +0200 Subject: [PATCH 22/31] match_types for Mapbox metadata --- .../cartodb_services/mapbox/geocoder.py | 16 +++++++++++++++- test/integration/test_street_functions.py | 4 ++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py index 9eb5e4e..1fc8b4e 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py @@ -25,6 +25,17 @@ TYPE_POINT = 'Point' EMPTY_RESPONSE = [[], {}] +MATCH_TYPE_BY_MATCH_LEVEL = { + 'poi': 'point_of_interest', + 'poi.landmark': 'point_of_interest', + 'place': 'point_of_interest', + 'country': 'country', + 'region': 'state', + 'locality': 'locality', + 'district': 'district', + 'address': 'street' +} + class MapboxGeocoder(Traceable): ''' @@ -80,9 +91,12 @@ class MapboxGeocoder(Traceable): else: precision = PRECISION_PRECISE + match_types = [MATCH_TYPE_BY_MATCH_LEVEL.get(match_level, None) + for match_level in result['place_type']] return { 'relevance': self._normalize_relevance(float(result['relevance'])), - 'precision': precision + 'precision': precision, + 'match_types': filter(None, match_types) } def _normalize_relevance(self, relevance): diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index 1c05058..86d03c6 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -92,9 +92,9 @@ class TestStreetFunctionsSetUp(TestCase): MAPBOX_METADATAS = { 'Plaza España, Barcelona': - {'relevance': 0.666, 'precision': 'precise'}, + {'relevance': 0.666, 'precision': 'precise', 'match_types': ['point_of_interest']}, 'Santiago Rusiñol 123, Valladolid': - {'relevance': 0.666, 'precision': 'precise'} # TODO: wrong + {'relevance': 0.666, 'precision': 'precise', 'match_types': ['point_of_interest']} # TODO: wrong } METADATAS = { From e82346e7f68c286225b19f828fc580e38429140b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 16 Jul 2018 12:43:40 +0200 Subject: [PATCH 23/31] match_types for batched HERE metadata --- .../cartodb_services/here/bulk_geocoder.py | 7 +++- .../cartodb_services/here/geocoder.py | 39 +++++++++---------- test/integration/test_street_functions.py | 5 +++ 3 files changed, 30 insertions(+), 21 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py index dd47fda..68c4766 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py @@ -140,10 +140,15 @@ class HereMapsBulkGeocoder(HereMapsGeocoder, StreetPointBulkGeocoder): reader = csv.DictReader(root_zip.open(name), delimiter='|') for row in reader: if row['SeqNumber'] == '1': # First per requested data + precision = self.PRECISION_BY_MATCH_TYPE[ + row.get('matchType', 'pointAddress')] + match_type = self.MATCH_TYPE_BY_MATCH_LEVEL.get(row['matchLevel'], None) results.append((row['recId'], [row['displayLongitude'], row['displayLatitude']], { - 'relevance': float(row['relevance']) + 'relevance': float(row['relevance']), + 'precision': precision, + 'match_types': [match_type] if match_type else [] })) return results diff --git a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py index d8f7f41..6031c97 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py @@ -9,24 +9,6 @@ from exceptions import * from cartodb_services import PRECISION_PRECISE, PRECISION_INTERPOLATED from cartodb_services.metrics import Traceable -PRECISION_BY_MATCH_TYPE = { - 'pointAddress': PRECISION_PRECISE, - 'interpolated': PRECISION_INTERPOLATED -} -MATCH_TYPE_BY_MATCH_LEVEL = { - 'landmark': 'point_of_interest', - 'country': 'country', - 'state': 'state', - 'county': 'county', - 'city': 'locality', - 'district': 'district', - 'street': 'street', - 'intersection': 'intersection', - 'houseNumber': 'street_number', - 'postalCode': 'postal_code' -} - - class HereMapsGeocoder(Traceable): 'A Here Maps Geocoder wrapper for python' @@ -70,6 +52,23 @@ class HereMapsGeocoder(Traceable): 'strictlanguagemode' ] + ADDRESS_PARAMS + PRECISION_BY_MATCH_TYPE = { + 'pointAddress': PRECISION_PRECISE, + 'interpolated': PRECISION_INTERPOLATED + } + MATCH_TYPE_BY_MATCH_LEVEL = { + 'landmark': 'point_of_interest', + 'country': 'country', + 'state': 'state', + 'county': 'county', + 'city': 'locality', + 'district': 'district', + 'street': 'street', + 'intersection': 'intersection', + 'houseNumber': 'street_number', + 'postalCode': 'postal_code' + } + def __init__(self, app_id, app_code, logger, service_params=None, maxresults=DEFAULT_MAXRESULTS): service_params = service_params or {} self.app_id = app_id @@ -145,9 +144,9 @@ class HereMapsGeocoder(Traceable): def _extract_metadata_from_result(self, result): # See https://stackoverflow.com/questions/51285622/missing-matchtype-at-here-geocoding-responses - precision = PRECISION_BY_MATCH_TYPE[ + precision = self.PRECISION_BY_MATCH_TYPE[ result.get('MatchType', 'pointAddress')] - match_type = MATCH_TYPE_BY_MATCH_LEVEL.get(result['MatchLevel'], None) + match_type = self.MATCH_TYPE_BY_MATCH_LEVEL.get(result['MatchLevel'], None) return { 'relevance': result['Relevance'], 'precision': precision, diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index 86d03c6..9b134e7 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -315,6 +315,11 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): assert_equal(n, len(response['rows'])) for row in response['rows']: assert_not_equal(row['st_x'], None) + assert_not_equal(row['metadata'], {}) + metadata = row['metadata'] + assert_not_equal(metadata['relevance'], None) + assert_not_equal(metadata['precision'], None) + assert_not_equal(metadata['match_types'], None) def test_missing_components_on_private_function(self): query = "SELECT _cdb_bulk_geocode_street_point(" \ From 8cb9e123b1a625b8bd3e84d8ec9c322a6ee62949 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 16 Jul 2018 19:55:04 +0200 Subject: [PATCH 24/31] Helper function to convert json arrays to PG arrays --- client/cdb_dataservices_client--0.25.0.sql | 7 ++++++- client/sql/05_utils.sql | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/client/cdb_dataservices_client--0.25.0.sql b/client/cdb_dataservices_client--0.25.0.sql index af58ce2..b69a536 100644 --- a/client/cdb_dataservices_client--0.25.0.sql +++ b/client/cdb_dataservices_client--0.25.0.sql @@ -18,7 +18,12 @@ BEGIN RETURN ROWS; END -$func$ LANGUAGE plpgsql;-- +$func$ LANGUAGE plpgsql; + +-- Taken from https://stackoverflow.com/a/48013356/351721 +CREATE OR REPLACE FUNCTION jsonb_array_casttext(jsonb) RETURNS text[] AS $f$ + SELECT array_agg(x) || ARRAY[]::text[] FROM jsonb_array_elements_text($1) t(x); +$f$ LANGUAGE sql IMMUTABLE;-- -- Geocoder server connection config -- -- The purpose of this function is provide to the PL/Proxy functions diff --git a/client/sql/05_utils.sql b/client/sql/05_utils.sql index d161c6a..31c8445 100644 --- a/client/sql/05_utils.sql +++ b/client/sql/05_utils.sql @@ -12,4 +12,9 @@ BEGIN RETURN ROWS; END -$func$ LANGUAGE plpgsql; \ No newline at end of file +$func$ LANGUAGE plpgsql; + +-- Taken from https://stackoverflow.com/a/48013356/351721 +CREATE OR REPLACE FUNCTION jsonb_array_casttext(jsonb) RETURNS text[] AS $f$ + SELECT array_agg(x) || ARRAY[]::text[] FROM jsonb_array_elements_text($1) t(x); +$f$ LANGUAGE sql IMMUTABLE; \ No newline at end of file From e2762a6e0352a4bde5187603110498cc8b78054e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 17 Jul 2018 10:06:43 +0200 Subject: [PATCH 25/31] Removed debug traces --- .../python/cartodb_services/cartodb_services/geocoder.py | 2 -- .../cartodb_services/google/bulk_geocoder.py | 2 -- .../cartodb_services/cartodb_services/google/geocoder.py | 1 - .../cartodb_services/here/bulk_geocoder.py | 8 -------- .../cartodb_services/cartodb_services/here/geocoder.py | 1 - .../cartodb_services/mapbox/bulk_geocoder.py | 4 ---- .../cartodb_services/cartodb_services/mapbox/geocoder.py | 2 -- .../cartodb_services/tomtom/bulk_geocoder.py | 8 -------- 8 files changed, 28 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index cea8ef4..c6043e4 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -91,10 +91,8 @@ class StreetPointBulkGeocoder: if len(street_geocoder_searches) > self.MAX_BATCH_SIZE: raise Exception("Batch size can't be larger than {}".format(self.MAX_BATCH_SIZE)) if self._should_use_batch(street_geocoder_searches): - self._logger.debug('--> Batch geocode') return self._batch_geocode(street_geocoder_searches) else: - self._logger.debug('--> Serial geocode') return self._serial_geocode(street_geocoder_searches) def _batch_geocode(self, street_geocoder_searches): diff --git a/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py index 42a60d9..0e1ab1f 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/bulk_geocoder.py @@ -26,7 +26,6 @@ class GoogleMapsBulkGeocoder(GoogleMapsGeocoder, StreetPointBulkGeocoder): for search in searches: (cartodb_id, street, city, state, country) = search lng_lat, metadata = self.geocode_meta(street, city, state, country) - self._logger.debug('--> lng_lat: {}. metadata: {}'.format(lng_lat, metadata)) results.append((cartodb_id, lng_lat, metadata)) return results @@ -35,7 +34,6 @@ class GoogleMapsBulkGeocoder(GoogleMapsGeocoder, StreetPointBulkGeocoder): pool = Pool(processes=self.PARALLEL_PROCESSES) for search in searches: (cartodb_id, street, city, state, country) = search - self._logger.debug('async geocoding --> {}'.format(search)) address = compose_address(street, city, state, country) if address: components = self._build_optional_parameters(city, state, country) diff --git a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py index 74d8f53..44b678c 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py @@ -63,7 +63,6 @@ class GoogleMapsGeocoder(): def _process_results(self, results): if results: - self._logger.debug('--> results: {}'.format(results[0])) return [ self._extract_lng_lat_from_result(results[0]), self._extract_metadata_from_result(results[0]) diff --git a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py index 68c4766..51b9ec9 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py @@ -47,7 +47,6 @@ class HereMapsBulkGeocoder(HereMapsGeocoder, StreetPointBulkGeocoder): def _batch_geocode(self, searches): request_id = self._send_batch(self._searches_to_csv(searches)) - self._logger.debug('--> Sent batch {}'.format(request_id)) last_processed = 0 stalled_retries = 0 @@ -55,26 +54,19 @@ class HereMapsBulkGeocoder(HereMapsGeocoder, StreetPointBulkGeocoder): while True: job_info = self._job_status(request_id) if job_info.processed_count == last_processed: - self._logger.debug('--> no progress ({})'.format(last_processed)) stalled_retries += 1 if stalled_retries > self.MAX_STALLED_RETRIES: raise Exception('Too many retries for job {}'.format(request_id)) else: - self._logger.debug('--> progress ({} != {})'.format(job_info.processed_count, last_processed)) stalled_retries = 0 last_processed = job_info.processed_count - self._logger.debug('--> Job poll check ({}): {}'.format( - stalled_retries, job_info)) if job_info.status in self.JOB_FINAL_STATES: break else: time.sleep(self.BATCH_RETRY_SLEEP_S) - self._logger.debug('--> Job complete: {}'.format(job_info)) - results = self._download_results(request_id) - self._logger.debug('--> Results: {} rows; {}'.format(len(results), results)) return results diff --git a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py index 6031c97..f52d2d7 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py @@ -99,7 +99,6 @@ class HereMapsGeocoder(Traceable): try: response = self._perform_request(params) result = response['Response']['View'][0]['Result'][0] - self._logger.debug('--> Result: {}'.format(result)) return [self._extract_lng_lat_from_result(result), self._extract_metadata_from_result(result)] except IndexError: diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py index 3ddd457..2563d43 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py @@ -28,7 +28,6 @@ class MapboxBulkGeocoder(MapboxGeocoder, StreetPointBulkGeocoder): results = [] for search in searches: elements = self._encoded_elements(search) - self._logger.debug('--> Sending serial search: {}'.format(search)) result = self.geocode_meta(*elements) results.append((search[0], result[0], result[1])) @@ -52,13 +51,10 @@ class MapboxBulkGeocoder(MapboxGeocoder, StreetPointBulkGeocoder): free = ', '.join([elem for elem in elements if elem]) frees.append(free) - self._logger.debug('--> sending free search: {}'.format(frees)) full_results = self.geocode_free_text_meta(frees) results = [] - self._logger.debug('--> searches: {}; xy: {}'.format(searches, full_results)) for s, r in zip(searches, full_results): results.append((s[0], r[0], r[1])) - self._logger.debug('--> results: {}'.format(results)) return results def _country_code(self, country): diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py index 1fc8b4e..0e3cd11 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py @@ -53,7 +53,6 @@ class MapboxGeocoder(Traceable): def _parse_geocoder_response(self, response): json_response = json.loads(response) - self._logger.debug('--> json response: {}'.format(json_response)) if json_response: if type(json_response) != list: @@ -154,7 +153,6 @@ class MapboxGeocoder(Traceable): try: free_search = ';'.join([self._escape(fs) for fs in free_searches]) - self._logger.debug('--> free search: {}, country: {}'.format(free_search, country)) response = self._geocoder.forward(address=free_search.decode('utf-8'), country=country) diff --git a/server/lib/python/cartodb_services/cartodb_services/tomtom/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/tomtom/bulk_geocoder.py index c8ad803..8fc2e92 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tomtom/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/tomtom/bulk_geocoder.py @@ -37,10 +37,8 @@ class TomTomBulkGeocoder(TomTomGeocoder, StreetPointBulkGeocoder): city = city.encode('utf-8') if city else None state = state.encode('utf-8') if state else None country = country.encode('utf-8') if country else None - self._logger.debug('--> Sending serial search: {}'.format(search)) result = self.geocode_meta(searchtext=address, city=city, state_province=state, country=country) - self._logger.debug('--> result sent') results.append((search_id, result[0], result[1])) return results @@ -50,12 +48,10 @@ class TomTomBulkGeocoder(TomTomGeocoder, StreetPointBulkGeocoder): results = [] for s, r in zip(searches, full_results): results.append((s[0], r[0], r[1])) - self._logger.debug('--> results: {}'.format(results)) return results def _send_batch(self, searches): body = {'batchItems': [{'query': self._query(s)} for s in searches]} - self._logger.debug('--> {}; Body: {}'.format(self.BATCH_URL, body)) request_params = { 'key': self._apikey } @@ -63,9 +59,7 @@ class TomTomBulkGeocoder(TomTomGeocoder, StreetPointBulkGeocoder): allow_redirects=False, params=request_params, timeout=(self.connect_timeout, self.read_timeout)) - self._logger.debug('--> response: {}'.format(response.status_code)) if response.status_code == 303: - self._logger.debug(response.headers) return response.headers['Location'] else: msg = "Error sending batch: {}; Headers: {}".format( @@ -78,14 +72,12 @@ class TomTomBulkGeocoder(TomTomGeocoder, StreetPointBulkGeocoder): while True: response = self.session.get(self.BASE_URL + location) if response.status_code == 200: - self._logger.debug('--> Results ready {}'.format(location)) return self._parse_results(response.json()) elif response.status_code == 202: stalled_retries += 1 if stalled_retries > self.MAX_STALLED_RETRIES: raise Exception('Too many retries for job {}'.format(location)) location = response.headers['Location'] - self._logger.debug('--> Waiting for {}'.format(location)) time.sleep(self.BATCH_RETRY_SLEEP_S) else: msg = "Error downloading batch: {}; Headers: {}".format( From e9ed3bca18c2c9f37f6c54511e7b1bce1cf413b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 17 Jul 2018 12:27:42 +0200 Subject: [PATCH 26/31] Safer comparison --- .../python/cartodb_services/cartodb_services/mapbox/geocoder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py index 0e3cd11..a54d06a 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py @@ -99,7 +99,7 @@ class MapboxGeocoder(Traceable): } def _normalize_relevance(self, relevance): - return 1 if relevance == 0.99 else relevance + return 1 if relevance >= 0.99 else relevance def _validate_input(self, searchtext, city=None, state_province=None, country=None): From c104f6f34bd6e714aa7bafd3eea350a173eb04d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 17 Jul 2018 12:46:16 +0200 Subject: [PATCH 27/31] Metadata attributes constant extraction --- .../python/cartodb_services/cartodb_services/__init__.py | 3 --- .../python/cartodb_services/cartodb_services/geocoder.py | 7 +++++++ .../cartodb_services/cartodb_services/google/geocoder.py | 9 ++++----- .../cartodb_services/here/bulk_geocoder.py | 7 ++++--- .../cartodb_services/cartodb_services/here/geocoder.py | 8 ++++---- .../cartodb_services/cartodb_services/mapbox/geocoder.py | 8 ++++---- .../cartodb_services/cartodb_services/tomtom/geocoder.py | 8 ++++---- test/integration/test_street_functions.py | 1 + 8 files changed, 28 insertions(+), 23 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/__init__.py b/server/lib/python/cartodb_services/cartodb_services/__init__.py index dd364b4..ef5caa4 100644 --- a/server/lib/python/cartodb_services/cartodb_services/__init__.py +++ b/server/lib/python/cartodb_services/cartodb_services/__init__.py @@ -35,6 +35,3 @@ def _reset(): GD = None from geocoder import run_street_point_geocoder, StreetPointBulkGeocoder - -PRECISION_PRECISE = 'precise' -PRECISION_INTERPOLATED = 'interpolated' diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index c6043e4..47e6d3a 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -6,6 +6,13 @@ from collections import namedtuple import json +METADATA_RELEVANCE = 'relevance' +METADATA_PRECISION = 'precision' +METADATA_MATCH_TYPES = 'match_types' + +PRECISION_PRECISE = 'precise' +PRECISION_INTERPOLATED = 'interpolated' + def compose_address(street, city=None, state=None, country=None): return ', '.join(filter(None, [street, city, state, country])) diff --git a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py index 44b678c..2f82549 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py @@ -4,8 +4,7 @@ from urlparse import parse_qs from exceptions import MalformedResult -from cartodb_services import PRECISION_PRECISE, PRECISION_INTERPOLATED -from cartodb_services.geocoder import compose_address +from cartodb_services.geocoder import compose_address, METADATA_RELEVANCE, METADATA_PRECISION, METADATA_MATCH_TYPES, PRECISION_PRECISE, PRECISION_INTERPOLATED from cartodb_services.google.exceptions import InvalidGoogleCredentials from client_factory import GoogleMapsClientFactory @@ -84,9 +83,9 @@ class GoogleMapsGeocoder(): match_types = [MATCH_TYPE_BY_MATCH_LEVEL.get(match_level, None) for match_level in result['types']] return { - 'relevance': base_relevance * partial_factor, - 'precision': PRECISION_BY_LOCATION_TYPE[location_type], - 'match_types': filter(None, match_types) + METADATA_RELEVANCE: base_relevance * partial_factor, + METADATA_PRECISION: PRECISION_BY_LOCATION_TYPE[location_type], + METADATA_MATCH_TYPES: filter(None, match_types) } diff --git a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py index 51b9ec9..b501c6e 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py @@ -8,6 +8,7 @@ from collections import namedtuple from requests.adapters import HTTPAdapter from cartodb_services import StreetPointBulkGeocoder from cartodb_services.here import HereMapsGeocoder +from cartodb_services.geocoder import METADATA_RELEVANCE, METADATA_PRECISION, METADATA_MATCH_TYPES from cartodb_services.metrics import Traceable from cartodb_services.tools.exceptions import ServiceException @@ -138,9 +139,9 @@ class HereMapsBulkGeocoder(HereMapsGeocoder, StreetPointBulkGeocoder): results.append((row['recId'], [row['displayLongitude'], row['displayLatitude']], { - 'relevance': float(row['relevance']), - 'precision': precision, - 'match_types': [match_type] if match_type else [] + METADATA_RELEVANCE: float(row['relevance']), + METADATA_PRECISION: precision, + METADATA_MATCH_TYPES: [match_type] if match_type else [] })) return results diff --git a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py index f52d2d7..52014b9 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py @@ -6,7 +6,7 @@ import requests from requests.adapters import HTTPAdapter from exceptions import * -from cartodb_services import PRECISION_PRECISE, PRECISION_INTERPOLATED +from cartodb_services.geocoder import METADATA_RELEVANCE, METADATA_PRECISION, METADATA_MATCH_TYPES, PRECISION_PRECISE, PRECISION_INTERPOLATED from cartodb_services.metrics import Traceable class HereMapsGeocoder(Traceable): @@ -147,7 +147,7 @@ class HereMapsGeocoder(Traceable): result.get('MatchType', 'pointAddress')] match_type = self.MATCH_TYPE_BY_MATCH_LEVEL.get(result['MatchLevel'], None) return { - 'relevance': result['Relevance'], - 'precision': precision, - 'match_types': [match_type] if match_type else [] + METADATA_RELEVANCE: result['Relevance'], + METADATA_PRECISION: precision, + METADATA_MATCH_TYPES: [match_type] if match_type else [] } diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py index a54d06a..59c707b 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py @@ -5,7 +5,7 @@ Python client for the Mapbox Geocoder service. import json import requests from mapbox import Geocoder -from cartodb_services import PRECISION_PRECISE, PRECISION_INTERPOLATED +from cartodb_services.geocoder import METADATA_RELEVANCE, METADATA_PRECISION, METADATA_MATCH_TYPES, PRECISION_PRECISE, PRECISION_INTERPOLATED from cartodb_services.metrics import Traceable from cartodb_services.tools.exceptions import ServiceException from cartodb_services.tools.qps import qps_retry @@ -93,9 +93,9 @@ class MapboxGeocoder(Traceable): match_types = [MATCH_TYPE_BY_MATCH_LEVEL.get(match_level, None) for match_level in result['place_type']] return { - 'relevance': self._normalize_relevance(float(result['relevance'])), - 'precision': precision, - 'match_types': filter(None, match_types) + METADATA_RELEVANCE: self._normalize_relevance(float(result['relevance'])), + METADATA_PRECISION: precision, + METADATA_MATCH_TYPES: filter(None, match_types) } def _normalize_relevance(self, relevance): diff --git a/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py index 09ed69d..d1c5bed 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py @@ -5,7 +5,7 @@ import json import requests from uritemplate import URITemplate from math import tanh -from cartodb_services import PRECISION_PRECISE, PRECISION_INTERPOLATED +from cartodb_services.geocoder import METADATA_RELEVANCE, METADATA_PRECISION, METADATA_MATCH_TYPES, PRECISION_PRECISE, PRECISION_INTERPOLATED from cartodb_services.metrics import Traceable from cartodb_services.tools.exceptions import ServiceException from cartodb_services.tools.qps import qps_retry @@ -145,9 +145,9 @@ class TomTomGeocoder(Traceable): score = self._normalize_score(result['score']) match_type = MATCH_TYPE_BY_MATCH_LEVEL.get(result['type'], None) return { - 'relevance': score, - 'precision': self._precision_from_score(score), - 'match_types': [match_type] if match_type else [] + METADATA_RELEVANCE: score, + METADATA_PRECISION: self._precision_from_score(score), + METADATA_MATCH_TYPES: [match_type] if match_type else [] } def _normalize_score(self, score): diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index 9b134e7..8535139 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -384,6 +384,7 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): self.metadata['Plaza España, Barcelona'], self.metadata['Santiago Rusiñol 123, Valladolid'] ] + assert_equal(len(response['rows']), len(expected)) for r, e in zip(response['rows'], expected): self.assert_metadata(r['metadata'], e) From d060bd8229bf306c81ed906f341da35039ab826e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 17 Jul 2018 13:24:39 +0200 Subject: [PATCH 28/31] Simplification of batching --- client/cdb_dataservices_client--0.25.0.sql | 12 +++++------- client/sql/21_bulk_geocoding_functions.sql | 12 +++++------- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/client/cdb_dataservices_client--0.25.0.sql b/client/cdb_dataservices_client--0.25.0.sql index b69a536..b67c219 100644 --- a/client/cdb_dataservices_client--0.25.0.sql +++ b/client/cdb_dataservices_client--0.25.0.sql @@ -2000,8 +2000,6 @@ DECLARE enough_quota boolean; remaining_quota integer; max_batch_size integer; - max_cartodb_id integer; - min_cartodb_id integer; cartodb_id_batch integer; batches_n integer; @@ -2027,8 +2025,8 @@ BEGIN batch_size := MAX_SAFE_BATCH_SIZE; END IF; - EXECUTE format('SELECT count(1), max(cartodb_id), min(cartodb_id), ceil((max(cartodb_id) + 1 - min(cartodb_id))::float/%s) FROM (%s) _x', batch_size, query) - INTO query_row_count, max_cartodb_id, min_cartodb_id, batches_n; + EXECUTE format('SELECT count(1), ceil(count(1)::float/%s) FROM (%s) _x', batch_size, query) + INTO query_row_count, batches_n; RAISE DEBUG 'cdb_bulk_geocode_street_point --> query_row_count: %; query: %; country: %; state: %; city: %; street: %', query_row_count, query, country_column, state_column, city_column, street_column; @@ -2058,13 +2056,13 @@ BEGIN 'WITH geocoding_data as (' || ' SELECT ' || ' json_build_object(''id'', cartodb_id, ''address'', %s, ''city'', %s, ''state'', %s, ''country'', %s) as data , ' || - ' floor((cartodb_id - $1)::float/$2) as batch' || + ' floor((row_number() over ())::float/$1) as batch' || ' FROM (%s) _x' || ') ' || 'INSERT INTO %s SELECT (cdb_dataservices_client._cdb_bulk_geocode_street_point(jsonb_agg(data))).* ' || 'FROM geocoding_data ' || - 'WHERE batch = $3', street_column, city_column, state_column, country_column, query, temp_table_name) - USING min_cartodb_id, batch_size, cartodb_id_batch; + 'WHERE batch = $2', street_column, city_column, state_column, country_column, query, temp_table_name) + USING batch_size, cartodb_id_batch; GET DIAGNOSTICS current_row_count = ROW_COUNT; RAISE DEBUG 'Batch % --> %', cartodb_id_batch, current_row_count; diff --git a/client/sql/21_bulk_geocoding_functions.sql b/client/sql/21_bulk_geocoding_functions.sql index 7ab3407..dfbe515 100644 --- a/client/sql/21_bulk_geocoding_functions.sql +++ b/client/sql/21_bulk_geocoding_functions.sql @@ -6,8 +6,6 @@ DECLARE enough_quota boolean; remaining_quota integer; max_batch_size integer; - max_cartodb_id integer; - min_cartodb_id integer; cartodb_id_batch integer; batches_n integer; @@ -33,8 +31,8 @@ BEGIN batch_size := MAX_SAFE_BATCH_SIZE; END IF; - EXECUTE format('SELECT count(1), max(cartodb_id), min(cartodb_id), ceil((max(cartodb_id) + 1 - min(cartodb_id))::float/%s) FROM (%s) _x', batch_size, query) - INTO query_row_count, max_cartodb_id, min_cartodb_id, batches_n; + EXECUTE format('SELECT count(1), ceil(count(1)::float/%s) FROM (%s) _x', batch_size, query) + INTO query_row_count, batches_n; RAISE DEBUG 'cdb_bulk_geocode_street_point --> query_row_count: %; query: %; country: %; state: %; city: %; street: %', query_row_count, query, country_column, state_column, city_column, street_column; @@ -64,13 +62,13 @@ BEGIN 'WITH geocoding_data as (' || ' SELECT ' || ' json_build_object(''id'', cartodb_id, ''address'', %s, ''city'', %s, ''state'', %s, ''country'', %s) as data , ' || - ' floor((cartodb_id - $1)::float/$2) as batch' || + ' floor((row_number() over ())::float/$1) as batch' || ' FROM (%s) _x' || ') ' || 'INSERT INTO %s SELECT (cdb_dataservices_client._cdb_bulk_geocode_street_point(jsonb_agg(data))).* ' || 'FROM geocoding_data ' || - 'WHERE batch = $3', street_column, city_column, state_column, country_column, query, temp_table_name) - USING min_cartodb_id, batch_size, cartodb_id_batch; + 'WHERE batch = $2', street_column, city_column, state_column, country_column, query, temp_table_name) + USING batch_size, cartodb_id_batch; GET DIAGNOSTICS current_row_count = ROW_COUNT; RAISE DEBUG 'Batch % --> %', cartodb_id_batch, current_row_count; From b90d402fa9e9822c2a791c36458028d9b8127efc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 17 Jul 2018 13:53:39 +0200 Subject: [PATCH 29/31] Round relevance (plus refactor) --- .../cartodb_services/cartodb_services/geocoder.py | 12 ++++++++---- .../cartodb_services/google/geocoder.py | 12 ++++++------ .../cartodb_services/here/bulk_geocoder.py | 12 ++++++------ .../cartodb_services/here/geocoder.py | 12 ++++++------ .../cartodb_services/mapbox/geocoder.py | 12 ++++++------ .../cartodb_services/tomtom/geocoder.py | 12 ++++++------ test/integration/test_street_functions.py | 8 ++++---- 7 files changed, 42 insertions(+), 38 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index 47e6d3a..465118d 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -6,13 +6,17 @@ from collections import namedtuple import json -METADATA_RELEVANCE = 'relevance' -METADATA_PRECISION = 'precision' -METADATA_MATCH_TYPES = 'match_types' - PRECISION_PRECISE = 'precise' PRECISION_INTERPOLATED = 'interpolated' + +def geocoder_metadata(relevance, precision, match_types): + return { + 'relevance': round(relevance, 2), + 'precision': precision, + 'match_types': match_types + } + def compose_address(street, city=None, state=None, country=None): return ', '.join(filter(None, [street, city, state, country])) diff --git a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py index 2f82549..fb75c76 100644 --- a/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/google/geocoder.py @@ -4,7 +4,7 @@ from urlparse import parse_qs from exceptions import MalformedResult -from cartodb_services.geocoder import compose_address, METADATA_RELEVANCE, METADATA_PRECISION, METADATA_MATCH_TYPES, PRECISION_PRECISE, PRECISION_INTERPOLATED +from cartodb_services.geocoder import compose_address, geocoder_metadata, PRECISION_PRECISE, PRECISION_INTERPOLATED from cartodb_services.google.exceptions import InvalidGoogleCredentials from client_factory import GoogleMapsClientFactory @@ -82,11 +82,11 @@ class GoogleMapsGeocoder(): partial_factor = PARTIAL_FACTOR if partial_match else 1 match_types = [MATCH_TYPE_BY_MATCH_LEVEL.get(match_level, None) for match_level in result['types']] - return { - METADATA_RELEVANCE: base_relevance * partial_factor, - METADATA_PRECISION: PRECISION_BY_LOCATION_TYPE[location_type], - METADATA_MATCH_TYPES: filter(None, match_types) - } + return geocoder_metadata( + base_relevance * partial_factor, + PRECISION_BY_LOCATION_TYPE[location_type], + filter(None, match_types) + ) def _build_optional_parameters(self, city=None, state=None, diff --git a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py index b501c6e..8316c44 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/bulk_geocoder.py @@ -8,7 +8,7 @@ from collections import namedtuple from requests.adapters import HTTPAdapter from cartodb_services import StreetPointBulkGeocoder from cartodb_services.here import HereMapsGeocoder -from cartodb_services.geocoder import METADATA_RELEVANCE, METADATA_PRECISION, METADATA_MATCH_TYPES +from cartodb_services.geocoder import geocoder_metadata from cartodb_services.metrics import Traceable from cartodb_services.tools.exceptions import ServiceException @@ -138,11 +138,11 @@ class HereMapsBulkGeocoder(HereMapsGeocoder, StreetPointBulkGeocoder): match_type = self.MATCH_TYPE_BY_MATCH_LEVEL.get(row['matchLevel'], None) results.append((row['recId'], [row['displayLongitude'], row['displayLatitude']], - { - METADATA_RELEVANCE: float(row['relevance']), - METADATA_PRECISION: precision, - METADATA_MATCH_TYPES: [match_type] if match_type else [] - })) + geocoder_metadata( + float(row['relevance']), + precision, + [match_type] if match_type else [] + ))) return results diff --git a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py index 52014b9..23dd871 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py @@ -6,7 +6,7 @@ import requests from requests.adapters import HTTPAdapter from exceptions import * -from cartodb_services.geocoder import METADATA_RELEVANCE, METADATA_PRECISION, METADATA_MATCH_TYPES, PRECISION_PRECISE, PRECISION_INTERPOLATED +from cartodb_services.geocoder import PRECISION_PRECISE, PRECISION_INTERPOLATED, geocoder_metadata from cartodb_services.metrics import Traceable class HereMapsGeocoder(Traceable): @@ -146,8 +146,8 @@ class HereMapsGeocoder(Traceable): precision = self.PRECISION_BY_MATCH_TYPE[ result.get('MatchType', 'pointAddress')] match_type = self.MATCH_TYPE_BY_MATCH_LEVEL.get(result['MatchLevel'], None) - return { - METADATA_RELEVANCE: result['Relevance'], - METADATA_PRECISION: precision, - METADATA_MATCH_TYPES: [match_type] if match_type else [] - } + return geocoder_metadata( + result['Relevance'], + precision, + [match_type] if match_type else [] + ) diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py index 59c707b..d76937a 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py @@ -5,7 +5,7 @@ Python client for the Mapbox Geocoder service. import json import requests from mapbox import Geocoder -from cartodb_services.geocoder import METADATA_RELEVANCE, METADATA_PRECISION, METADATA_MATCH_TYPES, PRECISION_PRECISE, PRECISION_INTERPOLATED +from cartodb_services.geocoder import PRECISION_PRECISE, PRECISION_INTERPOLATED, geocoder_metadata from cartodb_services.metrics import Traceable from cartodb_services.tools.exceptions import ServiceException from cartodb_services.tools.qps import qps_retry @@ -92,11 +92,11 @@ class MapboxGeocoder(Traceable): match_types = [MATCH_TYPE_BY_MATCH_LEVEL.get(match_level, None) for match_level in result['place_type']] - return { - METADATA_RELEVANCE: self._normalize_relevance(float(result['relevance'])), - METADATA_PRECISION: precision, - METADATA_MATCH_TYPES: filter(None, match_types) - } + return geocoder_metadata( + self._normalize_relevance(float(result['relevance'])), + precision, + filter(None, match_types) + ) def _normalize_relevance(self, relevance): return 1 if relevance >= 0.99 else relevance diff --git a/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py index d1c5bed..46cdbda 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/tomtom/geocoder.py @@ -5,7 +5,7 @@ import json import requests from uritemplate import URITemplate from math import tanh -from cartodb_services.geocoder import METADATA_RELEVANCE, METADATA_PRECISION, METADATA_MATCH_TYPES, PRECISION_PRECISE, PRECISION_INTERPOLATED +from cartodb_services.geocoder import PRECISION_PRECISE, PRECISION_INTERPOLATED, geocoder_metadata from cartodb_services.metrics import Traceable from cartodb_services.tools.exceptions import ServiceException from cartodb_services.tools.qps import qps_retry @@ -144,11 +144,11 @@ class TomTomGeocoder(Traceable): def _extract_metadata_from_result(self, result): score = self._normalize_score(result['score']) match_type = MATCH_TYPE_BY_MATCH_LEVEL.get(result['type'], None) - return { - METADATA_RELEVANCE: score, - METADATA_PRECISION: self._precision_from_score(score), - METADATA_MATCH_TYPES: [match_type] if match_type else [] - } + return geocoder_metadata( + score, + self._precision_from_score(score), + [match_type] if match_type else [] + ) def _normalize_score(self, score): return tanh(score * SCORE_NORMALIZATION_FACTOR) diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index 8535139..df30d66 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -87,14 +87,14 @@ class TestStreetFunctionsSetUp(TestCase): 'Plaza España, Barcelona': {'relevance': 0.85, 'precision': 'precise', 'match_types': ['street']}, 'Santiago Rusiñol 123, Valladolid': - {'relevance': 0.448283494533, 'precision': 'interpolated', 'match_types': ['street']} + {'relevance': 0.45, 'precision': 'interpolated', 'match_types': ['street']} } MAPBOX_METADATAS = { 'Plaza España, Barcelona': - {'relevance': 0.666, 'precision': 'precise', 'match_types': ['point_of_interest']}, + {'relevance': 0.67, 'precision': 'precise', 'match_types': ['point_of_interest']}, 'Santiago Rusiñol 123, Valladolid': - {'relevance': 0.666, 'precision': 'precise', 'match_types': ['point_of_interest']} # TODO: wrong + {'relevance': 0.67, 'precision': 'precise', 'match_types': ['point_of_interest']} # TODO: wrong } METADATAS = { @@ -410,7 +410,7 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): def assert_metadata(metadata, expected): relevance = metadata['relevance'] expected_relevance = expected['relevance'] - assert_true(isclose(relevance, expected_relevance, 0.05), + assert_true(isclose(relevance, expected_relevance, 0.02), '{} not close to {}'.format(relevance, expected_relevance)) assert_equal(metadata['precision'], expected['precision']) From 5e8dbaf2399eff58460edcf8856a757170300f45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 17 Jul 2018 13:56:44 +0200 Subject: [PATCH 30/31] Improvements in fixtures accuracy --- test/integration/test_street_functions.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index df30d66..a847637 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -37,7 +37,7 @@ class TestStreetFunctionsSetUp(TestCase): 'Madrid': [-3.70578, 40.42028], 'Logroño, Spain': [-2.45194, 42.46592], 'Logroño, Argentina': [-61.69604, -29.50425], - 'Plaza España, Barcelona': [2.1735699, 41.3823] # TODO: not ideal + 'Plaza España, Barcelona': [2.14834, 41.37494] } TOMTOM_POINTS = HERE_POINTS.copy() @@ -48,7 +48,7 @@ class TestStreetFunctionsSetUp(TestCase): 'Valladolid, Spain': [-4.72838, 41.6542], 'Madrid': [-3.70035, 40.42028], 'Logroño, Spain': [-2.44998, 42.46592], - 'Plaza España, Barcelona': [2.07479, 41.36818] # TODO: not ideal + 'Plaza España, Barcelona': [2.1497, 41.37516] }) MAPBOX_POINTS = GOOGLE_POINTS.copy() @@ -59,7 +59,7 @@ class TestStreetFunctionsSetUp(TestCase): 'Valladolid, Spain': [-4.72856, 41.652251], '1902 amphitheatre parkway': [-118.03, 34.06], # TODO: huge mismatch 'Madrid': [-3.69194, 40.4167754], - 'Plaza España, Barcelona': [2.245969, 41.452483] # TODO: not ideal + 'Plaza España, Barcelona': [2.342231, 41.50677] # TODO: not ideal }) FIXTURE_POINTS = { @@ -341,7 +341,6 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): x_y_by_cartodb_id = self._x_y_by_cartodb_id(response) assert_equal(x_y_by_cartodb_id[1], x_y_by_cartodb_id[2]) - # "'Plaza España', 'Barcelona', null, 'Spain') as the_geom) _x" def test_component_aggregation(self): query = "select cartodb_id, st_x(the_geom), st_y(the_geom) " \ "FROM cdb_dataservices_client.cdb_bulk_geocode_street_point(" \ From 5e34faefe55ce80d0fe91d9938f92e96f58f7559 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Tue, 17 Jul 2018 14:39:24 +0200 Subject: [PATCH 31/31] Quota test --- .../python/cartodb_services/cartodb_services/geocoder.py | 1 + test/integration/test_street_functions.py | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index 465118d..54e80f1 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -17,6 +17,7 @@ def geocoder_metadata(relevance, precision, match_types): 'match_types': match_types } + def compose_address(street, city=None, state=None, country=None): return ', '.join(filter(None, [street, city, state, country])) diff --git a/test/integration/test_street_functions.py b/test/integration/test_street_functions.py index a847637..ebcc26f 100644 --- a/test/integration/test_street_functions.py +++ b/test/integration/test_street_functions.py @@ -131,6 +131,11 @@ class TestStreetFunctionsSetUp(TestCase): return IntegrationTestHelper.execute_query_raw(self.sql_api_url, authenticated_query) + def _used_quota(self): + query = "select used_quota " \ + "from cdb_dataservices_client.cdb_service_quota_info() " \ + "where service = 'hires_geocoder'" + return self._run_authenticated(query)['rows'][0]['used_quota'] class TestStreetFunctions(TestStreetFunctionsSetUp): @@ -305,6 +310,8 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): 'Toronto, Canada"}}'.format(first_cartodb_id + i, first_street_number + i)) + used_quota = self._used_quota() + query = "select *, st_x(the_geom), st_y(the_geom) " \ "FROM cdb_dataservices_client.cdb_bulk_geocode_street_point( " \ "'select * from jsonb_to_recordset(''[" \ @@ -321,6 +328,8 @@ class TestBulkStreetFunctions(TestStreetFunctionsSetUp): assert_not_equal(metadata['precision'], None) assert_not_equal(metadata['match_types'], None) + assert_equal(self._used_quota(), used_quota + n) + def test_missing_components_on_private_function(self): query = "SELECT _cdb_bulk_geocode_street_point(" \ " '[{\"id\": \"1\", \"address\": \"Amphitheatre Parkway 22\"}]' " \