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.

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

[CALCITE-2755] Expose document _id field when querying ElasticSearch #982

Closed
wants to merge 1 commit into from

Conversation

@asereda-gs
Copy link
Member

asereda-gs commented Dec 26, 2018

CALCITE-2755

Allow user to query (project) _id field explicitly.

Note that (by default) meta fields are not available for select * type of queries and have to be explicitly listed in projection like select _MAP['_id'], _MAP['a'] from elastic.

Add additional mapping between calcite expression EXPR$n and item name foo.bar (as part of _MAP['foo.bar']). This information is otherwise lost during query translation.

Pls check that saving mapping between expression and item RexCalls is the right (recommended?) approach.

@asereda-gs asereda-gs force-pushed the asereda-gs:elastic-idfield branch from 816222f to 3ea8394 Dec 27, 2018
Allow user to query (project) [_id](https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-id-field.html) field
explicitly.

Note that (by default) meta fields are not available for `select *` type of queries and have to be explicitly listed in projection
like `select _MAP['_id'], _MAP['a'] from elastic`.

Add additional mapping between calcite expression `EXPR$n` and item name `foo.bar` (as part of `_MAP['foo.bar']`).
This information is otherwise lost during query translation.
@asereda-gs asereda-gs force-pushed the asereda-gs:elastic-idfield branch from 3ea8394 to 449d21d Dec 28, 2018
@beikov
beikov approved these changes Dec 31, 2018
Copy link
Contributor

beikov left a comment

AFAICS this looks ok to me.

@michaelmior

This comment has been minimized.

Copy link
Member

michaelmior commented Jan 1, 2019

LGTM although it might be good to have a documentation update which explains this.

Copy link
Contributor

zabetak left a comment

Hi @asereda-gs,

I added a few comments but nothing major against the PR. They could potentially be treated in an another PR in the future if you think it is worth it.

@@ -68,6 +68,10 @@
final String name = pair.right;

This comment has been minimized.

Copy link
@zabetak

zabetak Jan 2, 2019

Contributor

I think I understand now why you are obliged to put explicit aliases in the query. It is because you are relying in the method getNamedProjects() just above. I think it would be better if you just used the method getProjects() and then identify which RexNodes correspond to Elastic fields using a RexVisitor similar to the one use in ElasticSearchRules.isItem(RexNode).

if (ElasticsearchRules.isItem(pair.left)) {
implementor.addExpressionItemMapping(name, ElasticsearchRules.stripQuotes(expr));
}

if (expr.equals("\"" + name + "\"")) {
fields.add(name);

This comment has been minimized.

Copy link
@zabetak

zabetak Jan 2, 2019

Contributor

After identifying the fields necessary you could store them in a list in the implementor so that other operators (e.g., Sort) can take advantage of them.

@@ -72,7 +78,8 @@ public static void setupInstance() throws Exception {
"select _MAP['a'] AS \"a\", "
+ " _MAP['b.a'] AS \"b.a\", "
+ " _MAP['b.b'] AS \"b.b\", "
+ " _MAP['b.c.a'] AS \"b.c.a\" "
+ " _MAP['b.c.a'] AS \"b.c.a\", "

This comment has been minimized.

Copy link
@zabetak

zabetak Jan 2, 2019

Contributor

It would be nice if the users are not forced to specify an alias for every field.

This comment has been minimized.

Copy link
@asereda-gs

asereda-gs Jan 2, 2019

Author Member

Yes I was planning to automatically generate elastic schema based on Mapping

*
* @see SqlStdOperatorTable#ITEM
*/
final List<Map.Entry<String, String>> expressionItemMap = new ArrayList<>();

This comment has been minimized.

Copy link
@zabetak

zabetak Jan 2, 2019

Contributor

Possibly, you need just a list with the field names in the right order and not a mapping of the complete expression.

@asereda-gs

This comment has been minimized.

Copy link
Member Author

asereda-gs commented Jan 2, 2019

Thanks @zabetak for your review. I will address your comments in this or next PR.

@asfgit asfgit closed this in 3dee82d Jan 2, 2019
@asereda-gs asereda-gs deleted the asereda-gs:elastic-idfield branch Jan 2, 2019
F21 added a commit to F21/calcite that referenced this pull request Jan 3, 2019
Allow user to query (project) [_id](https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-id-field.html) field
explicitly.

Note that (by default) meta fields are not available for `select *` type of queries and have to be explicitly listed in projection
like `select _MAP['_id'], _MAP['a'] from elastic`.

Add additional mapping between calcite expression `EXPR$n` and item name `foo.bar` (as part of `_MAP['foo.bar']`).
This information is otherwise lost during query translation.

Closes apache#982
zhztheplayer added a commit to zhztheplayer/calcite that referenced this pull request Jan 12, 2019
Allow user to query (project) [_id](https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-id-field.html) field
explicitly.

Note that (by default) meta fields are not available for `select *` type of queries and have to be explicitly listed in projection
like `select _MAP['_id'], _MAP['a'] from elastic`.

Add additional mapping between calcite expression `EXPR$n` and item name `foo.bar` (as part of `_MAP['foo.bar']`).
This information is otherwise lost during query translation.

Closes apache#982
ashlanderr added a commit to ashlanderr/calcite that referenced this pull request Jan 15, 2019
Allow user to query (project) [_id](https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-id-field.html) field
explicitly.

Note that (by default) meta fields are not available for `select *` type of queries and have to be explicitly listed in projection
like `select _MAP['_id'], _MAP['a'] from elastic`.

Add additional mapping between calcite expression `EXPR$n` and item name `foo.bar` (as part of `_MAP['foo.bar']`).
This information is otherwise lost during query translation.

Closes apache#982
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.