Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL: Switch JDBC driver to libs/geo objects for geo_shapes #39204

Merged
merged 4 commits into from
Feb 27, 2019
Merged

SQL: Switch JDBC driver to libs/geo objects for geo_shapes #39204

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5f9cca7
SQL: Switch JDBC driver to libs/geo objects for geo_shapes
imotov Feb 20, 2019
a35fdff
Merge branch 'geosql' into issue-35767-integrate-libsgeo
imotov Feb 26, 2019
dc8fb16
Address review comments
imotov Feb 26, 2019
7752de5
Fix documentation
imotov Feb 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions x-pack/plugin/sql/jdbc/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ dependencies {
compile (project(':libs:x-content')) {
transitive = false
}
compile (project(':libs:elasticsearch-geo')) {
transitive = false
}
compile project(':libs:core')
runtime "com.fasterxml.jackson.core:jackson-core:${versions.jackson}"
testCompile "org.elasticsearch.test:framework:${version}"
Expand Down
10 changes: 9 additions & 1 deletion x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/TypeConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,16 @@
*/
package org.elasticsearch.xpack.sql.jdbc;

import org.elasticsearch.geo.utils.WellKnownText;
import org.elasticsearch.xpack.sql.proto.StringUtils;

import java.io.IOException;
import java.sql.Date;
import java.sql.SQLException;
import java.sql.SQLFeatureNotSupportedException;
import java.sql.Time;
import java.sql.Timestamp;
import java.text.ParseException;
import java.time.Duration;
import java.time.LocalDate;
import java.time.LocalDateTime;
Expand Down Expand Up @@ -234,8 +237,13 @@ static Object convert(Object v, EsType columnType, String typeString) throws SQL
case INTERVAL_MINUTE_TO_SECOND:
return Duration.parse(v.toString());
case GEO_POINT:
case GEO_SHAPE:
return v;
case GEO_SHAPE:
try {
return WellKnownText.fromWKT(v.toString());
} catch (IOException | ParseException ex) {
throw new SQLException("Cannot parse geo_shape", ex);
}
default:
throw new SQLException("Unexpected column type [" + typeString + "]");

Expand Down
2 changes: 0 additions & 2 deletions x-pack/plugin/sql/qa/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ dependencies {
compile project(path: xpackModule('sql:sql-cli'), configuration: 'nodeps')
compile "org.jline:jline:3.8.2"
compile "org.orbisgis:h2gis-ext:1.3.2"
// TODO: Temporary solution until core removes JTS as a dependency
compile "com.vividsolutions:jts-core:1.14.0"
}

/* disable unit tests because these are all integration tests used
Expand Down
5 changes: 5 additions & 0 deletions ...ck/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/geo/GeoSqlSpecTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,14 @@ public abstract class GeoSqlSpecTestCase extends SpecBaseIntegrationTestCase {
// Load GIS extensions
H2GISExtension.load(c);
c.createStatement().execute("RUNSCRIPT FROM 'classpath:/ogc/sqltsch.sql'");
c.createStatement().execute("RUNSCRIPT FROM 'classpath:/geo/setup_test_geo.sql'");
});

@ParametersFactory(argumentFormatting = PARAM_FORMATTING)
public static List<Object[]> readScriptSpec() throws Exception {
Parser parser = new SqlSpecParser();
List<Object[]> tests = new ArrayList<>();
tests.addAll(readScriptSpec("/geo/geosql.sql-spec", parser));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Picky comment: can you put the geo script file addition after "ogc"? (this is the order in the other two changes in this class - line 37 and line 56)

tests.addAll(readScriptSpec("/ogc/ogc.sql-spec", parser));
return tests;
}
Expand All @@ -51,6 +53,9 @@ public void setupTestGeoDataIfNeeded() throws Exception {
if (client().performRequest(new Request("HEAD", "/ogc")).getStatusLine().getStatusCode() == 404) {
GeoDataLoader.loadOGCDatasetIntoEs(client(), "ogc");
}
if (client().performRequest(new Request("HEAD", "/geo")).getStatusLine().getStatusCode() == 404) {
GeoDataLoader.loadGeoDatasetIntoEs(client(), "geo");
}
}


Expand Down
13 changes: 6 additions & 7 deletions x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcAssert.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,20 @@
*/
package org.elasticsearch.xpack.sql.qa.jdbc;

import com.vividsolutions.jts.geom.Geometry;
import com.vividsolutions.jts.io.ParseException;
import com.vividsolutions.jts.io.WKTReader;
import com.carrotsearch.hppc.IntObjectHashMap;

import org.apache.logging.log4j.Logger;
import org.elasticsearch.geo.utils.WellKnownText;
import org.elasticsearch.xpack.sql.jdbc.EsType;
import org.elasticsearch.xpack.sql.proto.StringUtils;
import org.relique.jdbc.csv.CsvResultSet;

import java.io.IOException;
import java.sql.ResultSet;
import java.sql.ResultSetMetaData;
import java.sql.SQLException;
import java.sql.Types;
import java.text.ParseException;
import java.time.temporal.TemporalAmount;
import java.util.ArrayList;
import java.util.Calendar;
Expand All @@ -45,7 +45,6 @@
*/
public class JdbcAssert {
private static final Calendar UTC_CALENDAR = Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT);
private static final WKTReader wkt = new WKTReader();

private static final IntObjectHashMap<EsType> SQL_TO_TYPE = new IntObjectHashMap<>();

Expand Down Expand Up @@ -269,11 +268,11 @@ else if (type == Types.DOUBLE) {
} else if (type == Types.FLOAT) {
assertEquals(msg, (float) expectedObject, (float) actualObject, lenientFloatingNumbers ? 1f : 0.0f);
} else if (type == Types.OTHER) {
if (expectedObject instanceof Geometry && actualObject instanceof String) {
if (actualObject instanceof org.elasticsearch.geo.geometry.Geometry) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please import to avoid the qualifier.

Copy link
Member

@costin costin Feb 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bpintea can you please advice what type the ODBC driver indicates (if at all) for geo?

Own update: it looks like both geometry and geography are mapped to -151.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bpintea can you please advice what type the ODBC driver indicates (if at all) for geo?

Own update: it looks like both geometry and geography are mapped to -151.

@costin There isn't any specification on how the geo types should be conveyed in ODBC. Most DBMSes use private types/extensions (like MS SQL's "user defined" -151). WKT/B seems to have most support (also among some BI tools and Excel plug-ins), with GeoJSON picking up as well.

An idea would be to apply the same logic as with KEYWORD, TEXT and IP: give the new geography/geometry types respective names, but reuse a character (12) or the binary (-3) type.
Since Geo is not part of the ODBC standard, no 3rd party existing app could "out of the box" make use of whatever data types we'll implement. OTOH, the values encoding (WKT/B) could be used.

// We need to convert the actual object to Geometry for comparision
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here your comment mentions the actual object needing conversion, but two lines below you perform the operation for the "expected" value instead. Shouldn't be the "actual" one conversion attempt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is leftover from the previous implementation. Before we had string in actual and JTS Geometry in expected. With this change I am trying to reduce dependency on JTS and H2 in this code, so instead of converting everything to H2's JTS Geometry I am converting it to our geometry. I will update the comment. Good catch!

try {
actualObject = wkt.read(actualObject.toString());
} catch (ParseException ex) {
expectedObject = WellKnownText.fromWKT(expectedObject.toString());
} catch (IOException | ParseException ex) {
fail(ex.getMessage());
}
}
Expand Down
16 changes: 16 additions & 0 deletions x-pack/plugin/sql/qa/src/main/resources/geo/geo.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
city,region,region_point,shape
Mountain View,Americas,POINT(-105.2551 54.5260),point (-122.083843 37.386483)
Chicago,Americas,POINT(-105.2551 54.5260),point (-87.637874 41.888783)
New York,Americas,POINT(-105.2551 54.5260),point (-73.990027 40.745171)
San Francisco,Americas,POINT(-105.2551 54.5260),point (-122.394228 37.789541)
Phoenix,Americas,POINT(-105.2551 54.5260),point (-111.973505 33.376242)
Amsterdam,Europe,POINT(15.2551 54.5260),point (4.850312 52.347557)
Berlin,Europe,POINT(15.2551 54.5260),point (13.390889 52.486701)
Munich,Europe,POINT(15.2551 54.5260),point (11.537505 48.146321)
London,Europe,POINT(15.2551 54.5260),point (-0.121672 51.510871)
Paris,Europe,POINT(15.2551 54.5260),point (2.351773 48.845538)
Singapore,Asia,POINT(100.6197 34.0479),point (103.855535 1.295868)
Hong Kong,Asia,POINT(100.6197 34.0479),point (114.183925 22.281397)
Seoul,Asia,POINT(100.6197 34.0479),point (127.060851 37.509132)
Tokyo,Asia,POINT(100.6197 34.0479),point (139.76402225 35.669616)
Sydney,Asia,POINT(100.6197 34.0479),point (151.208629 -33.863385)
19 changes: 19 additions & 0 deletions x-pack/plugin/sql/qa/src/main/resources/geo/geosql.sql-spec
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//
// Commands on geo test data
//

selectAllPointsAsStrings
SELECT city, shape, region FROM "geo" ORDER BY "city";

selectAllPointsAsWKT
SELECT city, ST_GEOMFROMTEXT(ST_ASWKT(shape)) shape_wkt, region FROM "geo" ORDER BY "city";

selectRegionUsingWktToSqlWithoutConvertion
SELECT region, city, shape, ST_GEOMFROMTEXT(region_point) region_wkt FROM geo ORDER BY region, city;

// TODO: the ordering of geometries differs so ORDER BY needs to be wrapped into string for now
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would create an issue for this TODO, just not to lose track of it. If there is one already, apologies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like order of the elements is not defined by the standard. So, I will order by something else in this test and remove the TODO.

selectCitiesWithAGroupByWktToSql
SELECT COUNT(city) city_by_region, ST_GEOMFROMTEXT(region_point) region FROM geo WHERE city LIKE '%a%' GROUP BY ST_GEOMFROMTEXT(region_point) ORDER BY ST_ASWKT(ST_GEOMFROMTEXT(region_point));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a test with a group by alias: SELECT COUNT(city) city_by_region, ST_GEOMFROMTEXT(region_point) region FROM geo GROUP BY region.


selectCitiesWithEOrderByWktToSql
SELECT region, city FROM geo WHERE city LIKE '%e%' ORDER BY ST_ASWKT(ST_GEOMFROMTEXT(region_point)), city;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above - a query with ORDER BY aliased geo function.

9 changes: 9 additions & 0 deletions x-pack/plugin/sql/qa/src/main/resources/geo/setup_test_geo.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
DROP TABLE IF EXISTS "geo";
CREATE TABLE "geo" (
"city" VARCHAR(50),
"region" VARCHAR(50),
"region_point" VARCHAR(50),
"shape" GEOMETRY

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove empty line?

)
AS SELECT * FROM CSVREAD('classpath:/geo/geo.csv');