You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metamodel.apache.org by LosD <gi...@git.apache.org> on 2015/10/28 10:23:35 UTC
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
GitHub user LosD opened a pull request:
https://github.com/apache/metamodel/pull/67
Jest (Rest) based ElasticSearch implementation
After looking at updating ElasticSearch 2.0, I got a bit fed up with the compatibility issues marring our current ElasticSearch implementation.
Here is a suggestion for a REST-based ElasticSearch DataContext using Jest. It is still dependent on ElasticSearch, as it uses SearchSourceBuilder for generating the queries. This also means that we're not completely independent from ES version, **but** the problem is much more limited now, and run time replacement of the Jest library version should be possible. Until a working version of Jest 2.0.0 (The current SNAPSHOT seems to have some problems with HTTPClient and AsyncHttpClient compatibility), we can't really verify that, though.
Over time, we should be able to get rid of SearchSourceBuilder by generating the JSON manually.
Testing is still directly dependent on the ES version, as it is using the ES node client.
I didn't find a simple way to get the HTTP port from the node client, 9201 is currently assumed.
A worry is the workaround in JestElasticSearchUtils.getDataFromColumnType(). I'd love to find a better way. The issue is that GSON uses LazilyParsedNumber for all numbers, which is not recognized as a Number (by either Jest or GSON) when feeding back into Jest for updates.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/LosD/metamodel master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metamodel/pull/67.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #67
----
commit bcdeb81bdecf185911466950751026b1be59ad15
Author: Dennis Du Krøger <de...@humaninference.com>
Date: 2015-10-23T14:26:31Z
Attempt of Jest based ElasticSearch client
Nowhere close to being done
commit a7f6dd2c890ac8caf1b94564e2d37a71fd0ba809
Author: Dennis Du Krøger <de...@process-manager.dk>
Date: 2015-10-23T23:36:38Z
ElasticSearchDataContext seems done.
ElasticSearchDataContext compiles now (no guaranteed, of course), the directly supporting cast as well.
commit 48e49f90fc2e8dd3cb62c03b0cdc0d7936d42dcd
Author: Dennis Du Krøger <de...@humaninference.com>
Date: 2015-10-26T16:23:16Z
Querying works now
All querying seems to work now, except for group by. However, that
works if standalone, or only run with the other query tests, so I'm
pretty sure that is because a failure in the
drop/create/update/etf tests leaves the ES in a bad state.
commit 25a6b2a74742ffc6a72dc9d67b65b49f4993242d
Author: Dennis Du Krøger <de...@humaninference.com>
Date: 2015-10-27T13:57:46Z
Everything seems to be working now.
I'm a bit unsure about the workaround for LazilyParsedNumber. What
about BitDecimal and BigInteger?
commit ddf488b2564dccde0281f671d92447f2b688e182
Author: Dennis Du Krøger <de...@humaninference.com>
Date: 2015-10-27T14:14:16Z
Differentiate native ElasticSearch* classes by prefix
Avoids confusion by prefixing all Jest-based classes with Jest. Also
makes Jest ES available in DataContextFactory.
commit f675a4e62548aa29a030b8692a35bb766c7f6b81
Author: Dennis Du Krøger <de...@humaninference.com>
Date: 2015-10-27T14:15:26Z
Merge remote-tracking branch 'origin/master' into personal-master
commit 891a4678508c87d06ea7128fe0774e9c10b19769
Author: Dennis Du Krøger <de...@humaninference.com>
Date: 2015-10-27T14:35:56Z
Merge remote-tracking branch 'origin/master' into personal-master
commit 29d5faa5be0faab3db8ac1aac27ae952390605e4
Author: Dennis Du Krøger <de...@humaninference.com>
Date: 2015-10-27T15:04:55Z
License and dependency issues resolved
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/67#discussion_r46023678
--- Diff: elasticsearch/rest/src/main/java/org/apache/metamodel/elasticsearch/rest/JestElasticSearchCreateTableBuilder.java ---
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.metamodel.elasticsearch.rest;
+
+import io.searchbox.indices.mapping.PutMapping;
+import org.apache.metamodel.MetaModelException;
+import org.apache.metamodel.create.AbstractTableCreationBuilder;
+import org.apache.metamodel.elasticsearch.common.ElasticSearchUtils;
+import org.apache.metamodel.schema.*;
+
+import java.util.ArrayList;
--- End diff --
Unused import
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metamodel/pull/67
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/67#discussion_r46023696
--- Diff: elasticsearch/rest/src/main/java/org/apache/metamodel/elasticsearch/rest/JestElasticSearchUtils.java ---
@@ -0,0 +1,80 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.metamodel.elasticsearch.rest;
+
+import com.google.gson.JsonElement;
+import com.google.gson.JsonObject;
+import org.apache.metamodel.data.DataSetHeader;
+import org.apache.metamodel.data.DefaultRow;
+import org.apache.metamodel.data.Row;
+import org.apache.metamodel.elasticsearch.common.ElasticSearchDateConverter;
+import org.apache.metamodel.query.SelectItem;
+import org.apache.metamodel.schema.Column;
+import org.apache.metamodel.schema.ColumnType;
+import org.apache.metamodel.util.NumberComparator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Date;
+
+/**
+ * Shared/common util functions for the ElasticSearch MetaModel module.
+ */
+final class JestElasticSearchUtils {
+ private static final Logger logger = LoggerFactory.getLogger(JestElasticSearchUtils.class);
--- End diff --
logger never used
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-160062012
Strange thing, in native/src/test/java the package is represented as one big folder instead of a tree of folders:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-160583868
Hi @balubollam
When does this happen? During initialization of the MetaModel context, or when querying it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-153158692
Okay, I think I fixed the Travis error (we'll see soon), and I made the wrapper for the Exception.
There's still the ES dependency issue remaining, though
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by balubollam <gi...@git.apache.org>.
Github user balubollam commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-160580122
HI Everyone
I am implementing Jest client for Amazon client client connection.But iam getting this following error:
Exception in thread "main" java.lang.NoClassDefFoundError: org/apache/http/ssl/SSLContexts
at org.apache.http.conn.ssl.SSLConnectionSocketFactory.getSocketFactory(SSLConnectionSocketFactory.java:172)
at io.searchbox.client.config.HttpClientConfig$Builder.build(HttpClientConfig.java:247)
at com.ktree.amazon.esclient.ConnectToAmazon.main(ConnectToAmazon.java:14)
Caused by: java.lang.ClassNotFoundException: org.apache.http.ssl.SSLContexts
But when i R&D on this i had come to know that
*I need to add patch to the httpcontext-osji using dependencies i.e adding pom.xml
Can Any one Please help me out how to add this patch or How to solve this error?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/67#discussion_r46023823
--- Diff: elasticsearch/native/src/test/java/org.apache.metamodel.elasticsearch.nativeclient/ElasticSearchUtilsTest.java ---
@@ -38,7 +39,7 @@ public void testAssignDocumentIdForPrimaryKeys() throws Exception {
DataSetHeader header = new SimpleDataSetHeader(selectItems1);
Map values = new HashMap<String, Object>();
--- End diff --
Map is raw type
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-154977148
Reading it again, that was pretty unclear. With "merging", I meant making a common module now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-153672147
:+1:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-153660918
Looks like very solid work. One general thought I have is to not prefix the classes with "Jest" though, but rather with "Rest". That way we don't tie it too much with the underlying library used, but rather with the protocol towards ElasticSearch. It will probably be more recognizable to ES users and it will also allow us to switch library and so on without changing class names.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-152835939
- I think the Travis error is because I made an assumption: I couldn't find a way to get the ES node to tell what HTTP port it actually bound to (and I didn't want to force it to something that may conflict), so I just assumed 9201, which is normally the first port, and planned to look into it later if it gave complications on Travis. I guess that time has come! :)
- I agree on the ES dependency. I'm not sure the code will be prettier using i.e. GSON, but shedding a huge dependency from a module (or at least making it test-only, we probably wont get rid of the embedded ES node) is always nice. Luckily, our use of the search functionality is rather limited, so I think it should be manageable.
- I think I might a least be able to make a few things less duplicate-y, like wrapping the client's ```IOException``` inside ```MetaModelException```. The request objects are generic and contains the type returned in ```Action<T extends JestResult>```, so a simple wrapper should suffice.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/67#discussion_r43858407
--- Diff: elasticsearch-jest/src/main/java/org/apache/metamodel/jest/elasticsearch/JestElasticSearchUpdateCallback.java ---
@@ -0,0 +1,88 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.metamodel.jest.elasticsearch;
+
+import io.searchbox.indices.Refresh;
+import org.apache.metamodel.AbstractUpdateCallback;
+import org.apache.metamodel.UpdateCallback;
+import org.apache.metamodel.create.TableCreationBuilder;
+import org.apache.metamodel.delete.RowDeletionBuilder;
+import org.apache.metamodel.drop.TableDropBuilder;
+import org.apache.metamodel.insert.RowInsertionBuilder;
+import org.apache.metamodel.schema.Schema;
+import org.apache.metamodel.schema.Table;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * {@link UpdateCallback} implementation for {@link JestElasticSearchDataContext}.
+ */
+final class JestElasticSearchUpdateCallback extends AbstractUpdateCallback {
+ private static final Logger logger = LoggerFactory.getLogger(JestElasticSearchUpdateCallback.class);
--- End diff --
'logger' is never used
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-158878006
Whoops, forgot to comment that I added a new commit (it would be wonderful if Github pinged subscribers on new commits):
_This adds a new package with common methods for both the native- and
rest-based clients. I'm actually not super fond with the solution,
but without completely restructuring the code (encapsulating all
communication into objects with common interfaces and introducing DTOs
for all requests and results), I think this is close to the best we can do._
The Travis failure seems unrelated, it's Excel going crazy in Java 8 for some reason.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-154696317
Merging them now would be fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-153665815
Please be aware that I raised another topic regarding this PR (and #68) in the mailing list under subject "[DISCUSS] ElasticSearch and MongoDB granularity"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-154653531
I would suggest to make the module restructure [1] already in this PR since it involves a bunch of code reuse. Or do you want to do that separately? What do you think?
[1] discussed in mail thread "[DISCUSS] ElasticSearch and MongoDB granularity" on dev@metamodel
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/67#discussion_r46023832
--- Diff: elasticsearch/native/src/test/java/org.apache.metamodel.elasticsearch.nativeclient/ElasticSearchUtilsTest.java ---
@@ -53,7 +54,7 @@ public void testCreateRowWithParseableDates() throws Exception {
Map values = new HashMap<String, Object>();
--- End diff --
And same here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-160064534
Yeah, I'm not exactly sure what happened with that folder. Fixed!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-153159528
D'oh. License.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-153670560
I called it "ElasticSearchRestDataContext", and changed the package name to "org.apache.metamodel.elasticsearch.rest". I left the other class names as-is, since they are very much tied to Jest, and aren't part of the API.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-153163695
Fixed. Also found another silly mistake: I had put used ```org.metamodel``` instead of ``` org.apache.metamodel``` as the package name.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-160103707
OK I think it's time to merge this ... Would you like to do the honors? I think maybe a squash first would be nice so that we have less commits on the master :-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-159938125
LGTM!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/67#discussion_r46032293
--- Diff: elasticsearch/rest/src/main/java/org/apache/metamodel/elasticsearch/rest/JestElasticSearchUtils.java ---
@@ -0,0 +1,78 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.metamodel.elasticsearch.rest;
+
+import com.google.gson.JsonElement;
+import com.google.gson.JsonObject;
+import org.apache.metamodel.data.DataSetHeader;
+import org.apache.metamodel.data.DefaultRow;
+import org.apache.metamodel.data.Row;
+import org.apache.metamodel.elasticsearch.common.ElasticSearchDateConverter;
+import org.apache.metamodel.query.SelectItem;
+import org.apache.metamodel.schema.Column;
+import org.apache.metamodel.schema.ColumnType;
+import org.apache.metamodel.util.NumberComparator;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
--- End diff --
Unused imports now ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/67#discussion_r43858350
--- Diff: elasticsearch-jest/src/main/java/org/apache/metamodel/jest/elasticsearch/JestElasticSearchCreateTableBuilder.java ---
@@ -0,0 +1,127 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.metamodel.jest.elasticsearch;
+
+import com.google.common.base.Strings;
+import io.searchbox.indices.mapping.PutMapping;
+import org.apache.metamodel.MetaModelException;
+import org.apache.metamodel.create.AbstractTableCreationBuilder;
+import org.apache.metamodel.schema.*;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.ArrayList;
+import java.util.List;
+
+final class JestElasticSearchCreateTableBuilder extends AbstractTableCreationBuilder<JestElasticSearchUpdateCallback> {
+
+ private static final Logger logger = LoggerFactory.getLogger(JestElasticSearchCreateTableBuilder.class);
--- End diff --
'logger' is never used
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Jest (Rest) based ElasticSearch implementa...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/67#issuecomment-152834096
I gave this one a quick scan through, and for me it's looking quite OK. I have some remarks:
* The build is breaking on Travis. This should be fixed one way or the other.
* It's a pity about the dependency still on elasticsearch. But not because of the dependency itself I think, but because it still complicates the code when you build query filter etc. and have to use reflection there. This would be a lot better to build dynamically using a Json request builder of sorts. For me this is not a blocker, but something we should probably make sure to report in JIRA as an improvement we would like to make eventually.
* There's a lot of code duplication. I don't see a good way out of this though (the obvious would be to have an "abstract" E.S. module but that would also be kinda overkill).
I hope this is useful feedback. A lot of code to review - and a very big contribution, so thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---