You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metamodel.apache.org by ardlema <gi...@git.apache.org> on 2015/08/24 09:20:08 UTC
[GitHub] metamodel pull request: METAMODEL-172: Adding Date parsing to ES m...
GitHub user ardlema opened a pull request:
https://github.com/apache/metamodel/pull/43
METAMODEL-172: Adding Date parsing to ES module
When working with ElasticSearch indexes that contains Date types the API is returning Strings instead of proper Date objects. We found out this problem querying an index with a time-zone format field.
Changes made in this PR:
- Added time-zone format dates support in the TimeComparator class
- Added a string to date direct conversion through the TimeComparator class in the ElasticSearchUtils class that will convert the string from ES into a proper Date (when the format will be supported by Metamodel)
- Added a new unit test for the ElasticSearchUtils class
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ardlema/metamodel METAMODEL-172
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metamodel/pull/43.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 #43
----
commit 8ab1394b1fbb0bd144ccd9a2b8df959b7c82f729
Author: Alberto Rodriguez <ar...@stratio.com>
Date: 2015-08-24T07:07:32Z
Added time zone pattern
commit c12cb11e8d1b11de64e2657f2e444b4a47f037e5
Author: Alberto Rodriguez <ar...@stratio.com>
Date: 2015-08-24T07:08:00Z
Convert dates column types to proper Date object
----
---
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: METAMODEL-172: Adding Date parsing to ES m...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/43#discussion_r37730850
--- Diff: elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchUtils.java ---
@@ -44,8 +47,17 @@ public static Row createRow(Map<String, Object> sourceMap, String documentId, Da
values[i] = documentId;
} else {
Object value = sourceMap.get(column.getName());
-
- values[i] = value;
+
+ if (column.getType() == ColumnType.DATE) {
+ Date valueToDate = TimeComparator.toDate(value);
--- End diff --
Maybe it's better that we put in the ES module the time format that ES uses. I guess the TimeComparator is a bit more general purpose while I imagine (?) that ES is always using one specific date pattern.
---
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: METAMODEL-172: Adding Date parsing to ES m...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/43#discussion_r37984410
--- Diff: elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchDateConverter.java ---
@@ -0,0 +1,41 @@
+/**
+ * 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;
+
+import org.apache.metamodel.util.TimeComparator;
+
+import java.text.ParseException;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+
+/**
+ * Util class to convert date strings from ElasticSearch to
+ * proper java Dates.
+ */
+final class ElasticSearchDateConverter {
+
+ public static Date tryToConvert(String dateAsString) {
+ try {
+ SimpleDateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSX");
+ return dateFormat.parse(dateAsString);
+ } catch (ParseException e) {
+ return TimeComparator.toDate(dateAsString);
--- End diff --
Doesn't ES have a utility method to convert the date string to a Date object or milliseconds since 1970 or something else, using the format of a field? It doesn't really make sense that we allow any other format than what is dictated by the schema.
---
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: METAMODEL-172: Adding Date parsing to ES m...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metamodel/pull/43
---
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: METAMODEL-172: Adding Date parsing to ES m...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/43#discussion_r37983155
--- Diff: elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchDateConverter.java ---
@@ -0,0 +1,41 @@
+/**
+ * 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;
+
+import org.apache.metamodel.util.TimeComparator;
+
+import java.text.ParseException;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+
+/**
+ * Util class to convert date strings from ElasticSearch to
+ * proper java Dates.
+ */
+final class ElasticSearchDateConverter {
+
+ public static Date tryToConvert(String dateAsString) {
+ try {
+ SimpleDateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSX");
+ return dateFormat.parse(dateAsString);
+ } catch (ParseException e) {
+ return TimeComparator.toDate(dateAsString);
--- End diff --
It's an improvement to add it, but I still would like to ask out (also to others?) what is best to do here ... In case we cannot parse the date format, then I tend to suggest that we simply return the dateAsString (so the method must return Object). Reason is that otherwise we might simply throw away date and that can give users the wrong impression, thinking that there is no date value in the data itself and fueling wrong application-logic then. Another approach is to throw an exception. In such a case at least the user knows that something went wrong. What I don't like is to let the user think everything is fine but that the data is null.
---
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: METAMODEL-172: Adding Date parsing to ES m...
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/43#discussion_r37948995
--- Diff: elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchDateConverter.java ---
@@ -0,0 +1,39 @@
+/**
+ * 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;
+
+import java.text.ParseException;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+
+/**
+ * Util class to convert date strings from ElasticSearch to
+ * proper java Dates.
+ */
+final class ElasticSearchDateConverter {
+
+ public static Date tryToConvert(String dateAsString) {
+ try {
+ SimpleDateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSX");
+ return dateFormat.parse(dateAsString);
+ } catch (ParseException e) {
+ return null;
--- End diff --
There is plenty of built-in date formats in ES, see: https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-date-format.html
My intention with this PR is provide a place where we can add the different formats for ES dates on-demand (when someone using our ES connector and working with a date format that we are not currently supporting we'll be able to easily include that format)
Calling the general purpose date converter after trying the ES specific ones might be a good idea 'cos we get all the existing MM date parsing conversions for free.
---
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: METAMODEL-172: Adding Date parsing to ES m...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/43#discussion_r37730788
--- Diff: core/src/test/java/org/apache/metamodel/util/TimeComparatorTest.java ---
@@ -66,6 +66,9 @@ public void testToDate() throws Exception {
assertEquals("2000-12-31 02:30:05.100",
dateFormat.format(TimeComparator
.toDate("2000-12-31 02:30:05.100")));
+
+ assertEquals("Fri Jan 04 15:55:51 CET 2013",
+ TimeComparator.toDate("2013-01-04T15:55:51.217+01:00").toString());
--- End diff --
This seems to fail in Travis.
---
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: METAMODEL-172: Adding Date parsing to ES m...
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on the pull request:
https://github.com/apache/metamodel/pull/43#issuecomment-134537945
@kaspersorensen as per your suggestion I moved the date parsing to the ES module instead of using the TypeComparator class
---
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: METAMODEL-172: Adding Date parsing to ES m...
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/43#discussion_r38073796
--- Diff: elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchDateConverter.java ---
@@ -0,0 +1,41 @@
+/**
+ * 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;
+
+import org.apache.metamodel.util.TimeComparator;
+
+import java.text.ParseException;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+
+/**
+ * Util class to convert date strings from ElasticSearch to
+ * proper java Dates.
+ */
+final class ElasticSearchDateConverter {
+
+ public static Date tryToConvert(String dateAsString) {
+ try {
+ SimpleDateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSX");
+ return dateFormat.parse(dateAsString);
+ } catch (ParseException e) {
+ return TimeComparator.toDate(dateAsString);
--- End diff --
@kaspersorensen if the TimeComparator method can't parse the date format and returns null we are just setting the value as String, see the code from ElasticSearchUtils below:
```
Date valueToDate = ElasticSearchDateConverter.tryToConvert((String) value);
if (valueToDate == null) {
values[i] = value;
} else {
values[i] = valueToDate;
}
```
I don't like a method that returns null though. I'd love to have the concept of Optional in Java7 so this method would return a Optional[Date] but for the time being I think this is a solution that might work.
@LosD Unfortunately I don't think that there is such utility method in ES.
---
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: METAMODEL-172: Adding Date parsing to ES m...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/43#issuecomment-135426789
+1 from me then :)
---
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: METAMODEL-172: Adding Date parsing to ES m...
Posted by ardlema <gi...@git.apache.org>.
Github user ardlema commented on the pull request:
https://github.com/apache/metamodel/pull/43#issuecomment-135530537
@LosD is this also ok with you?
---
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: METAMODEL-172: Adding Date parsing to ES m...
Posted by ardlema <gi...@git.apache.org>.
Github user ardlema commented on the pull request:
https://github.com/apache/metamodel/pull/43#issuecomment-135537440
Yes I agree. I guess this is the best worst option. I'll merge this into master tomorrow morning then.
Thank you @kaspersorensen @LosD for your comments!
---
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: METAMODEL-172: Adding Date parsing to ES m...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/43#discussion_r38093822
--- Diff: elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchDateConverter.java ---
@@ -0,0 +1,41 @@
+/**
+ * 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;
+
+import org.apache.metamodel.util.TimeComparator;
+
+import java.text.ParseException;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+
+/**
+ * Util class to convert date strings from ElasticSearch to
+ * proper java Dates.
+ */
+final class ElasticSearchDateConverter {
+
+ public static Date tryToConvert(String dateAsString) {
+ try {
+ SimpleDateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSX");
+ return dateFormat.parse(dateAsString);
+ } catch (ParseException e) {
+ return TimeComparator.toDate(dateAsString);
--- End diff --
Ah right, I didn't see the utils class yet, I was just looking 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: METAMODEL-172: Adding Date parsing to ES m...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the pull request:
https://github.com/apache/metamodel/pull/43#issuecomment-135536364
Yep! I'm no fan of allowing stuff the schema doesn't, but if we can't get ES to help us out, we don't really have a choice.
---
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: METAMODEL-172: Adding Date parsing to ES m...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/43#discussion_r37858020
--- Diff: elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchDateConverter.java ---
@@ -0,0 +1,39 @@
+/**
+ * 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;
+
+import java.text.ParseException;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+
+/**
+ * Util class to convert date strings from ElasticSearch to
+ * proper java Dates.
+ */
+final class ElasticSearchDateConverter {
+
+ public static Date tryToConvert(String dateAsString) {
+ try {
+ SimpleDateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSX");
+ return dateFormat.parse(dateAsString);
+ } catch (ParseException e) {
+ return null;
--- End diff --
I don't like to swallow an exception like this since it may give the wrong impression. Are we sure that this date format is always used by ES? If so I would throw an exception in this case. Or else at least make a loud warning or try the general purpose date converter or so.
---
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.
---