jhorcicka commented on a change in pull request #229: Upgrade to Elasticsearch
7.3.1
URL: https://github.com/apache/metamodel/pull/229#discussion_r331909686
##########
File path:
elasticsearch/rest/src/main/java/org/apache/metamodel/elasticsearch/rest/ElasticSearchRestDataContext.java
##########
@@ -133,13 +142,12 @@ public ElasticSearchRestDataContext(final
ElasticSearchRestClient client, String
final List<SimpleTableDef> result = new ArrayList<>();
if (mappings.isEmpty()) {
- logger.warn("No metadata returned for index name '{}' - no tables
will be detected.");
+ logger.warn("No metadata returned for index name '{}' - no tables
will be detected.", indexName);
} else {
- for (Entry<String, Object> mapping : mappings) {
- final String documentType = mapping.getKey();
-
- @SuppressWarnings("unchecked")
- Map<String, Object> mappingConfiguration = (Map<String,
Object>) mapping.getValue();
+ for (final Entry<String, MappingMetaData> mapping :
mappings.entrySet()) {
+ final String documentType = mapping.getValue().type();
Review comment:
This `documentType` variable is in the end used as a "table name". I find
the naming a bit confusing. Current situation makes me think it has to do
something with deprecated "mapping types".
I know there are no real "tables" in ES, but since this is already used e.
g. in "SimpleTableDef", I'd rather have it consistent.
I suggest to rename `documentType` on this line, and also the parameter name
of `detectTable` method, which, by the way, calls the document type an index
type.
Document type VS. index type VS. index name VS. table name VS. mapping type.
Perhaps I just lack ES terms being used here. :-) Then just ignore this
comment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
