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

Jump to bottom

SQL: Add support for SYS GEOMETRY_COLUMNS #30496

Closed
wants to merge 1 commit into from
Closed

SQL: Add support for SYS GEOMETRY_COLUMNS #30496

wants to merge 1 commit into from

Conversation

imotov
Copy link
Contributor

@imotov imotov commented May 9, 2018

Adds support for SYS GEOMETRY_COLUMNS, which returns the same
information as SELECT * FROM GEOMETRY_COLUMNS command in the standard
implementation.

Relates #29872

@imotov
SQL: Add support for SYS GEOMETRY_COLUMNS
0e8eea2 
Adds support for SYS GEOMETRY_COLUMNS, which returns the same
information as `SELECT * FROM GEOMETRY_COLUMNS` command in the standard
implementation.

Relates elastic#29872
@imotov imotov requested a review from costin May 9, 2018 21:59
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

costin
costin approved these changes May 10, 2018
View reviewed changes
Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

It's worth adding a parsing test as well (similar to SysTableTypesTests or any other under the command.sys package) just to make sure the grammar works correctly.

Otherwise LGTM.

By the way, how is the consumer of this command? What are the JDBC/ODBC API calls that this maps to?

...l/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysGeometryColumns.java
null, // schema is not supported
indexName, // table name
name, // column name
1, // storage type
Copy link
Member

Choose a reason for hiding this comment

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

Can you please put these constants into something like GeoUtils so DataTypes? I went with a similar approach in #30418 for readability and maintenance.

@imotov
Copy link
Contributor Author

imotov commented May 10, 2018

By the way, how is the consumer of this command? What are the JDBC/ODBC API calls that this maps to?

@costin that is basically equivalent to SELECT * FROM GEOMETRY_COLUMNS, but since we cannot really select from in-memory structures, I had to make it a SYS command. Can you think of a better way of handling this?

@imotov
Copy link
Contributor Author

imotov commented May 10, 2018

Discussed this with @costin and @bpintea. It seems that no drivers are using GEOMETRY_COLUMNS at the moment we might be able to skip implementing it all together. I am going to close this PR for now. We might need to reopen and merge it later on if needed.

@imotov imotov closed this May 10, 2018
@imotov imotov deleted the add-support-for-geometry-columns branch May 1, 2020 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@costin costin costin approved these changes

Assignees
No one assigned
Labels
:Analytics/SQL SQL querying >feature v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@imotov @elasticmachine @costin @colings86