Merge pull request ClickHouse#78852 from pratimapatel2008/geotoh3_lat_lon_input_order

Change geoToH3 function input order to (lat,lon,res) from (lon,lat,res)

Author: bharatnc

docs/en/sql-reference/functions/geo/h3.md

@@ -191,31 +191,33 @@ Result:
 
 ## geoToH3 {#geotoh3}
 
-Returns [H3](#h3-index) point index `(lon, lat)` with specified resolution.
+Returns [H3](#h3-index) point index `(lat, lon)` with specified resolution.
 
 **Syntax**
 
 ```sql
-geoToH3(lon, lat, resolution)
+geoToH3(lat, lon, resolution)
 ```
 
 **Arguments**
 
-- `lon` — Longitude. [Float64](../../data-types/float.md).
 - `lat` — Latitude. [Float64](../../data-types/float.md).
+- `lon` — Longitude. [Float64](../../data-types/float.md).
 - `resolution` — Index resolution. Range: `[0, 15]`. [UInt8](../../data-types/int-uint.md).
 
 **Returned values**
 
 - Hexagon index number. [UInt64](../../data-types/int-uint.md).
 - 0 in case of error. [UInt64](../../data-types/int-uint.md).
 
+Note: In ClickHouse v25.4 or older, `geoToH3()` takes values in order `(lon, lat)`. As per ClickHouse v25.5, the input values are in order `(lat, lon)`. The previous behaviour can be restored using setting `geotoh3_lon_lat_input_order = true`.
+
 **Example**
 
 Query:
 
 ```sql
-SELECT geoToH3(37.79506683, 55.71290588, 15) AS h3Index;
+SELECT geoToH3(55.71290588, 37.79506683, 15) AS h3Index;
 ```
 
 Result:

src/Core/Settings.cpp

@@ -3614,6 +3614,9 @@ If enabled, functions 'least' and 'greatest' return NULL if one of their argumen
 )", 0) \
     DECLARE(Bool, h3togeo_lon_lat_result_order, false, R"(
 Function 'h3ToGeo' returns (lon, lat) if true, otherwise (lat, lon).
+)", 0) \
+    DECLARE(Bool, geotoh3_lon_lat_input_order, false, R"(
+Function 'geoToH3' accepts (lon, lat) if true, otherwise (lat, lon).
 )", 0) \
     DECLARE(UInt64, max_partitions_per_insert_block, 100, R"(
 Limits the maximum number of partitions in a single inserted block

src/Core/SettingsChangesHistory.cpp

@@ -68,6 +68,7 @@ const VersionToSettingsChangesMap & getSettingsChangesHistory()
         /// Note: please check if the key already exists to prevent duplicate entries.
         addSettingsChanges(settings_changes_history, "25.5",
         {
+            {"geotoh3_lon_lat_input_order", true, false, "A new setting for legacy behaviour to set lon and lat order"},
             {"secondary_indices_enable_bulk_filtering", false, true, "A new algorithm for filtering by data skipping indices"},
             {"implicit_table_at_top_level", "", "", "A new setting, used in clickhouse-local"},
         });

src/Functions/geoToH3.cpp

@@ -8,7 +8,8 @@
 #include <DataTypes/DataTypesNumber.h>
 #include <Functions/FunctionFactory.h>
 #include <Functions/IFunction.h>
-#include <Common/typeid_cast.h>
+#include <Interpreters/Context.h>
+#include <Core/Settings.h>
 #include <base/range.h>
 
 #include <constants.h>
@@ -17,6 +18,10 @@
 
 namespace DB
 {
+namespace Setting
+{
+    extern const SettingsBool geotoh3_lon_lat_input_order;
+}
 namespace ErrorCodes
 {
     extern const int ILLEGAL_TYPE_OF_ARGUMENT;
@@ -32,10 +37,16 @@ namespace
 /// and returns h3 index of this point
 class FunctionGeoToH3 : public IFunction
 {
+    const bool geotoh3_lon_lat_input_order;
 public:
     static constexpr auto name = "geoToH3";
 
-    static FunctionPtr create(ContextPtr) { return std::make_shared<FunctionGeoToH3>(); }
+    static FunctionPtr create(ContextPtr context) { return std::make_shared<FunctionGeoToH3>(context); }
+
+    explicit FunctionGeoToH3(ContextPtr context)
+    : geotoh3_lon_lat_input_order(context->getSettingsRef()[Setting::geotoh3_lon_lat_input_order])
+    {
+    }
 
     std::string getName() const override { return name; }
 
@@ -80,26 +91,39 @@ class FunctionGeoToH3 : public IFunction
         for (auto & argument : non_const_arguments)
             argument.column = argument.column->convertToFullColumnIfConst();
 
-        const auto * col_lon = checkAndGetColumn<ColumnFloat64>(non_const_arguments[0].column.get());
-        if (!col_lon)
-            throw Exception(
-                    ErrorCodes::ILLEGAL_COLUMN,
-                    "Illegal type {} of argument {} of function {}. Must be Float64.",
-                    arguments[0].type->getName(),
-                    1,
-                    getName());
-        const auto & data_lon = col_lon->getData();
+        const ColumnFloat64 * col_lat = nullptr;
+        const ColumnFloat64 * col_lon = nullptr;
+
+        if (geotoh3_lon_lat_input_order)
+        {
+            col_lon = checkAndGetColumn<ColumnFloat64>(non_const_arguments[0].column.get());
+            col_lat = checkAndGetColumn<ColumnFloat64>(non_const_arguments[1].column.get());
+        }
+        else
+        {
+            col_lat = checkAndGetColumn<ColumnFloat64>(non_const_arguments[0].column.get());
+            col_lon = checkAndGetColumn<ColumnFloat64>(non_const_arguments[1].column.get());
+        }
 
-        const auto * col_lat = checkAndGetColumn<ColumnFloat64>(non_const_arguments[1].column.get());
         if (!col_lat)
             throw Exception(
-                    ErrorCodes::ILLEGAL_COLUMN,
-                    "Illegal type {} of argument {} of function {}. Must be Float64.",
-                    arguments[1].type->getName(),
-                    2,
-                    getName());
+                ErrorCodes::ILLEGAL_COLUMN,
+                "Illegal type {} of argument {} of function {}. Must be Float64.",
+                arguments[1].type->getName(),
+                2,
+                getName());
+
         const auto & data_lat = col_lat->getData();
 
+        if (!col_lon)
+            throw Exception(
+                ErrorCodes::ILLEGAL_COLUMN,
+                "Illegal type {} of argument {} of function {}. Must be Float64.",
+                arguments[0].type->getName(),
+                1,
+                getName());
+        const auto & data_lon = col_lon->getData();
+
         const auto * col_res = checkAndGetColumn<ColumnUInt8>(non_const_arguments[2].column.get());
         if (!col_res)
             throw Exception(
@@ -154,3 +178,4 @@ REGISTER_FUNCTION(GeoToH3)
 }
 
 #endif
+

tests/queries/0_stateless/00926_geo_to_h3.sql

@@ -10,11 +10,11 @@ INSERT INTO table1 VALUES(55.72076200, 37.59813500, 15);
 INSERT INTO table1 VALUES(55.72076201, 37.59813500, 15);
 INSERT INTO table1 VALUES(55.72076200, 37.59813500, 14);
 
-select geoToH3(37.63098076, 55.77922738, 15);
-select geoToH3(37.63098076, 55.77922738, 24); -- { serverError ARGUMENT_OUT_OF_BOUND }
-select geoToH3(lon, lat, resolution) from table1 order by lat, lon, resolution;
-select geoToH3(lon, lat, resolution) AS k from table1 order by lat, lon, k;
-select lat, lon, geoToH3(lon, lat, resolution) AS k from table1 order by lat, lon, k;
-select geoToH3(lon, lat, resolution) AS k, count(*) from table1 group by k order by k;
+select geoToH3(55.77922738, 37.63098076, 15);
+select geoToH3(55.77922738, 37.63098076, 24); -- { serverError ARGUMENT_OUT_OF_BOUND }
+select geoToH3(lat, lon, resolution) from table1 order by lat, lon, resolution;
+select geoToH3(lat, lon, resolution) AS k from table1 order by lat, lon, k;
+select lat, lon, geoToH3(lat, lon, resolution) AS k from table1 order by lat, lon, k;
+select geoToH3(lat, lon, resolution) AS k, count(*) from table1 group by k order by k;
 
 DROP TABLE table1

tests/queries/0_stateless/01906_h3_to_geo.sql

@@ -5,7 +5,7 @@ DROP TABLE IF EXISTS h3_indexes;
 CREATE TABLE h3_indexes (h3_index UInt64) ENGINE = Memory;
 
 -- Random geo coordinates were generated using the H3 tool: https://github.com/ClickHouse-Extras/h3/blob/master/src/apps/testapps/mkRandGeo.c at various resolutions from 0 to 15.
--- Corresponding H3 index values were in turn generated with those geo coordinates using `geoToH3(lon, lat, res)` ClickHouse function for the following test.
+-- Corresponding H3 index values were in turn generated with those geo coordinates using `geoToH3(lat, lon, res)` ClickHouse function for the following test.
 
 INSERT INTO h3_indexes VALUES (579205133326352383);
 INSERT INTO h3_indexes VALUES (581263419093549055);
@@ -55,7 +55,7 @@ INSERT INTO h3_geo VALUES (8.19906334981474, 67.69370966550179, 15);
 SELECT result FROM (
     SELECT
         (lon, lat) AS input_geo,
-        h3ToGeo(geoToH3(lat, lon, res)) AS output_geo,
+        h3ToGeo(geoToH3(lon, lat, res)) AS output_geo,
         if(abs(input_geo.1 - output_geo.1) < 0.001 AND abs(input_geo.2 - output_geo.2) < 0.001, 'ok', 'fail') AS result
     FROM h3_geo
 );

tests/queries/0_stateless/02223_h3_test_const_columns.sql

@@ -1,6 +1,6 @@
 -- Tags: no-fasttest
 
-select round(geoToH3(toFloat64(0),toFloat64(1),arrayJoin([1,2])), 2);
+select round(geoToH3(toFloat64(1),toFloat64(0),arrayJoin([1,2])), 2);
 select h3ToParent(641573946153969375, arrayJoin([1,2]));
 SELECT round(h3HexAreaM2(arrayJoin([1,2])), 2);
 SELECT round(h3HexAreaKm2(arrayJoin([1,2])), 2);

tests/queries/0_stateless/03404_geotoh3_input_order.reference

@@ -0,0 +1,2 @@
+644325524701193974
+644325524701193974

tests/queries/0_stateless/03404_geotoh3_input_order.sql

@@ -0,0 +1,7 @@
+-- Tags: no-fasttest
+-- no-fasttest: h3ToGeo needs binary with Uber H3 libary
+
+-- Test for setting 'geotoh3_lon_lat_input_order'
+
+SELECT geoToH3(37.79506683, 55.71290588, 15) AS h3Index SETTINGS geotoh3_lon_lat_input_order = true;
+SELECT geoToH3(55.71290588, 37.79506683, 15) AS h3Index SETTINGS geotoh3_lon_lat_input_order = false;