You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metamodel.apache.org by jhorcicka <gi...@git.apache.org> on 2018/05/22 11:39:09 UTC
[GitHub] metamodel pull request #179: Feature/neo4j column types
GitHub user jhorcicka opened a pull request:
https://github.com/apache/metamodel/pull/179
Feature/neo4j column types
This PR adds column types to the neo4j table definitions. In the previous version all column types were treated as Strings.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jhorcicka/metamodel feature/neo4j-column-types
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metamodel/pull/179.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 #179
----
commit fdaaed05d7ed3aadfe79b3ce71dd49bc45b6b9f4
Author: jakub <j....@...>
Date: 2018-05-18T07:49:48Z
Neo4j: column types support.
commit b60601e76e0218ae314f1dbf3e50c6a27a0877cf
Author: jakub <j....@...>
Date: 2018-05-22T10:22:26Z
Refactoring.
commit 59ec9ccbd569b33a3281a7adc2153e244e4f8a5d
Author: jakub <j....@...>
Date: 2018-05-22T11:22:20Z
Node ID from metadata is Integer, not Long.
----
---
[GitHub] metamodel issue #179: Feature/neo4j column types
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/179
Looks good to me, but I think we should do a tiny bit more code hygeine in terms of centralizing all the string literals in a collection of constants somewhere.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r189906596
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ColumnTypeResolver.java ---
@@ -0,0 +1,117 @@
+/**
+ * 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.neo4j.utils;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.json.JSONException;
+import org.json.JSONObject;
+
+public class ColumnTypeResolver {
+ public ColumnType[] getColumnTypes(final JSONObject jsonObject, final String[] columnNamesArray) {
+ final List<String> columnNames = new ArrayList<>(Arrays.asList(columnNamesArray));
+ final List<ColumnType> columnTypes = new ArrayList<>();
+
+ try {
+ fillColumnTypesFromMetadata(jsonObject, columnNames, columnTypes);
+ fillColumnTypesFromData(jsonObject, columnNames, columnTypes);
+ } catch (final JSONException e) {
+ // ignore missing data
+ }
+
+ fillColumnTypesFromRemainingColumns(columnNames, columnTypes);
+ return columnTypes.toArray(new ColumnType[columnTypes.size()]);
+ }
+
+ private void fillColumnTypesFromData(final JSONObject jsonObject, final List<String> columnNames,
+ final List<ColumnType> columnTypes) throws JSONException {
+ final String dataKey = "data";
+
+ if (jsonObject.has(dataKey)) {
+ final JSONObject data = jsonObject.getJSONObject(dataKey);
+ final Iterator keysIterator = data.keys();
--- End diff --
Iterator is a raw type. References to generic type Iterator<E> should be parameterized.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r190560674
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ColumnTypeResolver.java ---
@@ -0,0 +1,126 @@
+/**
+ * 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.neo4j.utils;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.json.JSONArray;
+import org.json.JSONException;
+import org.json.JSONObject;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ColumnTypeResolver {
+ private static final Logger logger = LoggerFactory.getLogger(ColumnTypeResolver.class);
+
+ public ColumnType[] getColumnTypes(final JSONObject jsonObject, final String[] columnNamesArray) {
+ final List<String> columnNames = new ArrayList<>(Arrays.asList(columnNamesArray));
+ final List<ColumnType> columnTypes = new ArrayList<>();
+
+ try {
+ fillColumnTypesFromMetadata(jsonObject, columnNames, columnTypes);
+ fillColumnTypesFromData(jsonObject, columnNames, columnTypes);
+ } catch (final JSONException e) {
+ // ignore missing data
+ }
+
+ fillColumnTypesFromRemainingColumns(columnNames, columnTypes);
+ return columnTypes.toArray(new ColumnType[columnTypes.size()]);
+ }
+
+ private void fillColumnTypesFromData(final JSONObject jsonObject, final List<String> columnNames,
+ final List<ColumnType> columnTypes) throws JSONException {
+ final String dataKey = "data";
+
+ if (jsonObject.has(dataKey)) {
+ final JSONObject data = jsonObject.getJSONObject(dataKey);
+ final Iterator<?> keysIterator = data.keys();
+
+ while (keysIterator.hasNext()) {
+ final String key = (String) keysIterator.next();
+ final ColumnType type = getTypeFromValue(data, key);
+ columnTypes.add(type);
+ removeIfAvailable(columnNames, key);
+ }
+ }
+ }
+
+ private void fillColumnTypesFromMetadata(final JSONObject jsonObject, final List<String> columnNames,
+ final List<ColumnType> columnTypes) throws JSONException {
+ final String metadataKey = "metadata";
+
+ if (jsonObject.has(metadataKey)) {
+ final JSONObject metadata = jsonObject.getJSONObject(metadataKey);
+
+ if (metadata.has("id")) {
+ columnTypes.add(ColumnType.BIGINT);
+ removeIfAvailable(columnNames, "_id");
+ }
+ }
+ }
+
+ private void fillColumnTypesFromRemainingColumns(final List<String> columnNames,
+ final List<ColumnType> columnTypes) {
+ for (final String remainingColumnName : columnNames) {
+ if (remainingColumnName.contains("rel_")) {
+ if (remainingColumnName.contains("#")) {
+ columnTypes.add(ColumnType.ARRAY);
+ } else {
+ columnTypes.add(ColumnType.BIGINT);
+ }
+ } else {
+ columnTypes.add(ColumnType.STRING);
+ }
+ }
+ }
+
+ private void removeIfAvailable(final List<String> list, final String key) {
+ if (list.contains(key)) {
+ list.remove(key);
+ }
+ }
+
+ private ColumnType getTypeFromValue(final JSONObject data, final String key) {
+ try {
+ final Class<? extends Object> keyClass = data.get(key).getClass();
+
+ if (keyClass.equals(Boolean.class)) {
+ return ColumnType.BOOLEAN;
+ } else if (keyClass.equals(Integer.class)) {
+ return ColumnType.INTEGER;
+ } else if (keyClass.equals(Long.class)) {
+ return ColumnType.BIGINT;
+ } else if (keyClass.equals(Double.class)) {
+ return ColumnType.DOUBLE;
+ } else if (keyClass.equals(JSONArray.class)) {
+ return ColumnType.ARRAY;
--- End diff --
I don't think we should return `ColumnType.ARRAY` in this case, but `ColumnType.LIST` and make sure that when we get the value of such a column we make sure the value is "cast" to a `List`. In that manner we can access it better down the line.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r190142943
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ColumnTypeResolver.java ---
@@ -0,0 +1,117 @@
+/**
+ * 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.neo4j.utils;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.json.JSONException;
+import org.json.JSONObject;
+
+public class ColumnTypeResolver {
+ public ColumnType[] getColumnTypes(final JSONObject jsonObject, final String[] columnNamesArray) {
+ final List<String> columnNames = new ArrayList<>(Arrays.asList(columnNamesArray));
+ final List<ColumnType> columnTypes = new ArrayList<>();
+
+ try {
+ fillColumnTypesFromMetadata(jsonObject, columnNames, columnTypes);
+ fillColumnTypesFromData(jsonObject, columnNames, columnTypes);
+ } catch (final JSONException e) {
+ // ignore missing data
+ }
+
+ fillColumnTypesFromRemainingColumns(columnNames, columnTypes);
+ return columnTypes.toArray(new ColumnType[columnTypes.size()]);
+ }
+
+ private void fillColumnTypesFromData(final JSONObject jsonObject, final List<String> columnNames,
+ final List<ColumnType> columnTypes) throws JSONException {
+ final String dataKey = "data";
+
+ if (jsonObject.has(dataKey)) {
+ final JSONObject data = jsonObject.getJSONObject(dataKey);
+ final Iterator keysIterator = data.keys();
+
+ while (keysIterator.hasNext()) {
+ final String key = (String) keysIterator.next();
+ final ColumnType type = getTypeFromValue(data, key);
+ columnTypes.add(type);
+ removeIfAvailable(columnNames, key);
+ }
+ }
+ }
+
+ private void fillColumnTypesFromMetadata(final JSONObject jsonObject, final List<String> columnNames,
+ final List<ColumnType> columnTypes) throws JSONException {
+ final String metadataKey = "metadata";
+
+ if (jsonObject.has(metadataKey)) {
+ final JSONObject metadata = jsonObject.getJSONObject(metadataKey);
+
+ if (metadata.has("id")) {
+ columnTypes.add(ColumnType.BIGINT);
+ removeIfAvailable(columnNames, "_id");
+ }
+ }
+ }
+
+ private void fillColumnTypesFromRemainingColumns(final List<String> columnNames,
+ final List<ColumnType> columnTypes) {
+ for (final String remainingColumnName : columnNames) {
+ if (remainingColumnName.contains("rel_")) {
+ if (remainingColumnName.contains("#")) {
+ columnTypes.add(ColumnType.ARRAY);
+ } else {
+ columnTypes.add(ColumnType.BIGINT);
+ }
+ } else {
+ columnTypes.add(ColumnType.STRING);
+ }
+ }
+ }
+
+ private void removeIfAvailable(final List<String> list, final String key) {
+ if (list.contains(key)) {
+ list.remove(key);
+ }
+ }
+
+ private ColumnType getTypeFromValue(final JSONObject data, final String key) {
--- End diff --
Instead of implementing the algorithm to determine the type in here using a Chain of Responsibility pattern, maybe just use some good old "if ... else" statements, something like this:
```
try {
Class<? extends Object> keyClass = data.get(key).getClass();
if (keyClass.equals(Boolean.class)) {
return ColumnType.BOOLEAN;
} else if (keyClass.equals(Integer.class)) {
return ColumnType.INTEGER;
} else if (keyClass.equals(Long.class)) {
return ColumnType.BIGINT;
} else if (keyClass.equals(Double.class)) {
return ColumnType.DOUBLE;
} else if (keyClass.equals(Map.class)) {
return ColumnType.MAP;
} else if (keyClass.equals(JSONArray.class)) {
return ColumnType.ARRAY;
}
} catch (JSONException e) {
// TODO: Log something
}
return ColumnType.STRING;
```
In this manner you don't depend on exceptions to indicate a column isn't of a certain type and you don't need loads of code and even though this contains quite a number of "if ... else" statements, it is clear to everyone who sees the code what it is trying to achieve.
Note that I haven't tested the MAP and ARRAY parts in this code, so they may be incorrect :)
---
[GitHub] metamodel issue #179: Feature/neo4j column types
Posted by jhorcicka <gi...@git.apache.org>.
Github user jhorcicka commented on the issue:
https://github.com/apache/metamodel/pull/179
Updated, @arjansh.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r190897592
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/Neo4jDataContext.java ---
@@ -158,102 +159,127 @@ protected String getMainSchemaName() throws MetaModelException {
}
public SimpleTableDef[] detectTableDefs() {
- List<SimpleTableDef> tableDefs = new ArrayList<SimpleTableDef>();
-
- String labelsJsonString = _requestWrapper.executeRestRequest(new HttpGet(_serviceRoot + "/labels"));
-
- JSONArray labelsJsonArray;
+ final List<SimpleTableDef> tableDefs = new ArrayList<>();
+ final String labelsJsonString = _requestWrapper.executeRestRequest(new HttpGet(_serviceRoot + "/labels"));
+ final JSONArray labelsJsonArray;
+
try {
labelsJsonArray = new JSONArray(labelsJsonString);
+
for (int i = 0; i < labelsJsonArray.length(); i++) {
- String label = labelsJsonArray.getString(i);
+ final SimpleTableDef tableDefFromLabel = createTableDefFromLabel(labelsJsonArray.getString(i));
+
+ if (tableDefFromLabel != null) {
+ tableDefs.add(tableDefFromLabel);
+ }
+ }
+
+ return tableDefs.toArray(new SimpleTableDef[tableDefs.size()]);
+ } catch (final JSONException e) {
+ logger.error("Error occurred in parsing JSON while detecting the schema: ", e);
+ throw new IllegalStateException(e);
+ }
+ }
+
+ private SimpleTableDef createTableDefFromLabel(final String label) throws JSONException {
+ final List<JSONObject> nodesPerLabel = getAllNodesPerLabel(label);
+ final List<String> propertiesPerLabel = getPropertiesFromLabelNodes(nodesPerLabel);
+ final Set<String> relationshipPropertiesPerLabel = new LinkedHashSet<>();
+
+ for (final JSONObject node : nodesPerLabel) {
+ final Integer nodeId = (Integer) node.getJSONObject("metadata").get("id");
+ final Set<String> relationshipPropertiesForNode = createRelationshipPropertiesForNode(nodeId);
+ relationshipPropertiesPerLabel.addAll(relationshipPropertiesForNode);
+ }
+
+ propertiesPerLabel.addAll(relationshipPropertiesPerLabel);
- List<JSONObject> nodesPerLabel = getAllNodesPerLabel(label);
+ if (nodesPerLabel.isEmpty()) {
+ return null; // Do not add a table if label has no nodes (empty tables are considered non-existent)
+ } else {
+ final String[] columnNames = propertiesPerLabel.toArray(new String[propertiesPerLabel.size()]);
+ final ColumnTypeResolver columnTypeResolver = new ColumnTypeResolver(nodesPerLabel.get(0), columnNames);
+ return new SimpleTableDef(label, columnNames, columnTypeResolver.getColumnTypes());
+ }
+ }
- List<String> propertiesPerLabel = new ArrayList<String>();
- for (JSONObject node : nodesPerLabel) {
- List<String> propertiesPerNode = getAllPropertiesPerNode(node);
- for (String property : propertiesPerNode) {
- if (!propertiesPerLabel.contains(property)) {
- propertiesPerLabel.add(property);
- }
- }
- }
+ private Set<String> createRelationshipPropertiesForNode(final Integer nodeId) throws JSONException {
+ final List<JSONObject> relationshipsPerNode = getOutgoingRelationshipsPerNode(nodeId);
+ final Set<String> relationshipProperties = new LinkedHashSet<>();
+
+ for (final JSONObject relationship : relationshipsPerNode) {
+ // Add the relationship as a column in the table
+ final String relationshipName = relationship.getString("type");
--- End diff --
Same here.
---
[GitHub] metamodel issue #179: Feature/neo4j column types
Posted by jhorcicka <gi...@git.apache.org>.
Github user jhorcicka commented on the issue:
https://github.com/apache/metamodel/pull/179
@arjansh:
* JUnit test was added.
* I decided to use the simple&clear if-then block for the column type decision.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r190898432
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ColumnTypeResolver.java ---
@@ -0,0 +1,126 @@
+/**
+ * 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.neo4j.utils;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.json.JSONArray;
+import org.json.JSONException;
+import org.json.JSONObject;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ColumnTypeResolver {
+ private static final Logger logger = LoggerFactory.getLogger(ColumnTypeResolver.class);
+ private final JSONObject _jsonObject;
+ private final List<String> _columnNames = new ArrayList<>();
+ private final List<ColumnType> _columnTypes = new ArrayList<>();
+
+ public ColumnTypeResolver(final JSONObject jsonObject, final String[] columnNamesArray) {
+ _jsonObject = jsonObject;
+ _columnNames.addAll(Arrays.asList(columnNamesArray));
+ }
+
+ public ColumnType[] getColumnTypes() {
+ try {
+ fillColumnTypesFromMetadata();
+ fillColumnTypesFromData();
+ } catch (final JSONException e) {
+ // ignore missing data
+ }
+
+ fillColumnTypesFromRemainingColumns();
+ return _columnTypes.toArray(new ColumnType[_columnTypes.size()]);
+ }
+
+ private void fillColumnTypesFromData() throws JSONException {
+ final String dataKey = "data";
+
+ if (_jsonObject.has(dataKey)) {
+ final JSONObject data = _jsonObject.getJSONObject(dataKey);
+ final Iterator<?> keysIterator = data.keys();
+
+ while (keysIterator.hasNext()) {
+ final String key = (String) keysIterator.next();
+ final ColumnType type = getTypeFromValue(data, key);
+ _columnTypes.add(type);
+ removeIfAvailable(_columnNames, key);
+ }
+ }
+ }
+
+ private void fillColumnTypesFromMetadata() throws JSONException {
+ final String metadataKey = "metadata";
+
+ if (_jsonObject.has(metadataKey)) {
+ final JSONObject metadata = _jsonObject.getJSONObject(metadataKey);
+
+ if (metadata.has("id")) {
--- End diff --
On closer inspection, I see that "_id" is also a neo4j name. I thought it was OUR name for it. In this case my suggested name would be something like `NEO4J_KEY_ID` or something.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r190878656
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ColumnTypeResolver.java ---
@@ -0,0 +1,126 @@
+/**
+ * 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.neo4j.utils;
--- End diff --
If you move this class from the `org.apache.metamodel.neo4j.utils` package to the `org.apache.metamodel.neo4j` package, you can make its visibility `package protected` instead of `public`, that way it is not right away implicitly part of the MetaModel api. And we are not using it elsewhere anyway. From my point of view it's best to keep visibility of classes and methods as hidden as possible, we can always make them more visible later. It's however not really possible the other way around. Once some class or method is public, it's implicitly part of the api and it's hard to reduce its visibility later on.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r189905854
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ColumnTypeResolver.java ---
@@ -0,0 +1,117 @@
+/**
+ * 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.neo4j.utils;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.json.JSONException;
+import org.json.JSONObject;
+
+public class ColumnTypeResolver {
--- End diff --
If I look at the manner in which this class is instantiated and in the manner in which the getColumnTypes method is invoked, I would either refactor it into a helper class with only static methods and a private no args constructor, or leave it like it is, but then add a constructor to it which takes a `JSONObject` as an argument, so it doesn't need to be passed through in all methods on the class.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r189916493
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ArrayColumnTypeHandler.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.neo4j.utils;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.json.JSONException;
+import org.json.JSONObject;
+
+public class ArrayColumnTypeHandler extends AbstractColumnTypeHandler {
+ @Override
+ public ColumnType getTypeFromValue(final JSONObject data, final String key) {
+ try {
+ data.getJSONArray(key);
--- End diff --
This pattern of code gets repeated in all `ColumnTypeHandler` implementations. I would create an abstract method which contains just the code within between the `try` and `catch` and implement that within each `ColumnTypeHandler` implementation and move the remaining part of the code into the abstract class, so that would contain a concrete and an abstract method in this context.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r190558268
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ColumnTypeResolver.java ---
@@ -0,0 +1,117 @@
+/**
+ * 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.neo4j.utils;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.json.JSONException;
+import org.json.JSONObject;
+
+public class ColumnTypeResolver {
--- End diff --
Even though you reacted with a thumbs up on my previous comment here, you didn't address it :)
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by jhorcicka <gi...@git.apache.org>.
Github user jhorcicka commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r190797849
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ColumnTypeResolver.java ---
@@ -0,0 +1,117 @@
+/**
+ * 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.neo4j.utils;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.json.JSONException;
+import org.json.JSONObject;
+
+public class ColumnTypeResolver {
--- End diff --
Yes. I missed that one. Thanks for noticing.
It should be better now.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by jhorcicka <gi...@git.apache.org>.
Github user jhorcicka commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r191154706
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ColumnTypeResolver.java ---
@@ -0,0 +1,126 @@
+/**
+ * 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.neo4j.utils;
--- End diff --
I agree. This was caused by my previous implementation when there were multiple classes in that new package. At that time the reason for that was to separate all of these classes from the rest, because they served just one internal purpose. Having just the one class in a new package is probably useless. I'll move it.
Thanks for noticing.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r191345706
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/Neo4jDataContext.java ---
@@ -52,23 +52,24 @@
* DataContext implementation for Neo4j
*/
public class Neo4jDataContext extends QueryPostprocessDataContext implements DataContext, DocumentSourceProvider {
-
- public static final Logger logger = LoggerFactory.getLogger(Neo4jDataContext.class);
-
public static final String SCHEMA_NAME = "neo4j";
-
public static final int DEFAULT_PORT = 7474;
-
- public static final String RELATIONSHIP_PREFIX = "rel_";
-
- public static final String RELATIONSHIP_COLUMN_SEPARATOR = "#";
+ public static final String NEO4J_KEY_METADATA = "metadata";
--- End diff --
Can you make these newly added constants (all starting with "NEO4J_") package protected? They are all only used for internal logic within this package here and I don't think we want to expose them by default as part of the api.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by jhorcicka <gi...@git.apache.org>.
Github user jhorcicka commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r191188566
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ColumnTypeResolver.java ---
@@ -0,0 +1,126 @@
+/**
+ * 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.neo4j.utils;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.json.JSONArray;
+import org.json.JSONException;
+import org.json.JSONObject;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ColumnTypeResolver {
+ private static final Logger logger = LoggerFactory.getLogger(ColumnTypeResolver.class);
+ private final JSONObject _jsonObject;
+ private final List<String> _columnNames = new ArrayList<>();
+ private final List<ColumnType> _columnTypes = new ArrayList<>();
--- End diff --
Sure. What is the benefit of that?
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by jhorcicka <gi...@git.apache.org>.
Github user jhorcicka commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r191153847
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ColumnTypeResolver.java ---
@@ -0,0 +1,126 @@
+/**
+ * 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.neo4j.utils;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.json.JSONArray;
+import org.json.JSONException;
+import org.json.JSONObject;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ColumnTypeResolver {
+ private static final Logger logger = LoggerFactory.getLogger(ColumnTypeResolver.class);
+ private final JSONObject _jsonObject;
+ private final List<String> _columnNames = new ArrayList<>();
+ private final List<ColumnType> _columnTypes = new ArrayList<>();
+
+ public ColumnTypeResolver(final JSONObject jsonObject, final String[] columnNamesArray) {
--- End diff --
I have two reasons for that.
1) Already existing interfaces (methods, constructors) work with arrays instead of lists; for both, column names and column types. I think it's easier to understand the code when I use both of these "collections" in the same format/type (both arrays or both lists).
2) If I change it for ColumnTypeResolver, I get another confusing situation 2 lines below:
Both of these highlighted variables contain the same value, just different data type. That would be confusing for me.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r189903958
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ColumnTypeHandler.java ---
@@ -0,0 +1,38 @@
+/**
+ * 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.neo4j.utils;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.json.JSONObject;
+
+public interface ColumnTypeHandler {
--- End diff --
Why introduce this `ColumnTypeHandler` interface? It is only used within this module, why not remove this interface and rename the `AbstractColumnTypeHandler` class to `ColumnTypeHandler`? This feels a bit like premature optimization to me.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r189892629
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/Neo4jDataContext.java ---
@@ -158,60 +159,79 @@ protected String getMainSchemaName() throws MetaModelException {
}
public SimpleTableDef[] detectTableDefs() {
- List<SimpleTableDef> tableDefs = new ArrayList<SimpleTableDef>();
-
- String labelsJsonString = _requestWrapper.executeRestRequest(new HttpGet(_serviceRoot + "/labels"));
-
- JSONArray labelsJsonArray;
+ final List<SimpleTableDef> tableDefs = new ArrayList<>();
+ final String labelsJsonString = _requestWrapper.executeRestRequest(new HttpGet(_serviceRoot + "/labels"));
+ final JSONArray labelsJsonArray;
+
try {
labelsJsonArray = new JSONArray(labelsJsonString);
+
for (int i = 0; i < labelsJsonArray.length(); i++) {
- String label = labelsJsonArray.getString(i);
+ fillTableDefsFromLabel(labelsJsonArray.getString(i), tableDefs);
+ }
+
+ return tableDefs.toArray(new SimpleTableDef[tableDefs.size()]);
+ } catch (final JSONException e) {
+ logger.error("Error occured in parsing JSON while detecting the schema: ", e);
+ throw new IllegalStateException(e);
+ }
+ }
+
+ private void fillTableDefsFromLabel(final String label, final List<SimpleTableDef> tableDefs) throws JSONException {
+ final List<JSONObject> nodesPerLabel = getAllNodesPerLabel(label);
+ final List<String> propertiesPerLabel = getPropertiesFromLabelNodes(nodesPerLabel);
+ final Set<String> relationshipPropertiesPerLabel = new LinkedHashSet<>();
+
+ for (final JSONObject node : nodesPerLabel) {
+ final Integer nodeId = (Integer) node.getJSONObject("metadata").get("id");
+ fillRelationshipPropertiesPerLabel(nodeId, relationshipPropertiesPerLabel);
+ }
+
+ propertiesPerLabel.addAll(relationshipPropertiesPerLabel);
+
+ // Do not add a table if label has no nodes (empty tables are considered non-existent)
+ if (!nodesPerLabel.isEmpty()) {
+ final String[] columnNames = propertiesPerLabel.toArray(new String[propertiesPerLabel.size()]);
+ final ColumnTypeResolver columnTypeResolver = new ColumnTypeResolver();
+ final SimpleTableDef tableDef = new SimpleTableDef(label, columnNames,
+ columnTypeResolver.getColumnTypes(nodesPerLabel.get(0), columnNames));
+ tableDefs.add(tableDef);
+ }
+ }
+
+ private void fillRelationshipPropertiesPerLabel(final Integer nodeId, final Set<String> relationshipPropertiesPerLabel)
--- End diff --
Essentially this method has nothing to do with labels, it gets relationship properties for a node and adds them to the given set, so I would call it `addRelationshipPropertiesForNode` instead and rename the `relationshipPropertiesPerLabel` parameter to `relationshipProperties`.
Personally, again, but this is just my personal preference, I would refactor the method so it only takes `nodeId` as an argument and returns the set of of relationship properties for that `nodeId` and then have the method which invokes it add all of these to its own set using the Set#addAll(Set) method.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r189893280
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/Neo4jDataContext.java ---
@@ -236,8 +256,8 @@ protected String getMainSchemaName() throws MetaModelException {
}
}
- private List<JSONObject> getOutgoingRelationshipsPerNode(Integer nodeId) {
- List<JSONObject> outgoingRelationshipsPerNode = new ArrayList<JSONObject>();
+ private List<JSONObject> getOutgoingRelationshipsPerNode(final Integer nodeId) {
+ List<JSONObject> outgoingRelationshipsPerNode = new ArrayList<>();
--- End diff --
If you improve this line of code, why not make the variable `final` too?
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r190560349
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ColumnTypeResolver.java ---
@@ -0,0 +1,126 @@
+/**
+ * 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.neo4j.utils;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.json.JSONArray;
+import org.json.JSONException;
+import org.json.JSONObject;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ColumnTypeResolver {
+ private static final Logger logger = LoggerFactory.getLogger(ColumnTypeResolver.class);
+
+ public ColumnType[] getColumnTypes(final JSONObject jsonObject, final String[] columnNamesArray) {
+ final List<String> columnNames = new ArrayList<>(Arrays.asList(columnNamesArray));
+ final List<ColumnType> columnTypes = new ArrayList<>();
+
+ try {
+ fillColumnTypesFromMetadata(jsonObject, columnNames, columnTypes);
+ fillColumnTypesFromData(jsonObject, columnNames, columnTypes);
+ } catch (final JSONException e) {
+ // ignore missing data
+ }
+
+ fillColumnTypesFromRemainingColumns(columnNames, columnTypes);
+ return columnTypes.toArray(new ColumnType[columnTypes.size()]);
+ }
+
+ private void fillColumnTypesFromData(final JSONObject jsonObject, final List<String> columnNames,
+ final List<ColumnType> columnTypes) throws JSONException {
+ final String dataKey = "data";
+
+ if (jsonObject.has(dataKey)) {
+ final JSONObject data = jsonObject.getJSONObject(dataKey);
+ final Iterator<?> keysIterator = data.keys();
+
+ while (keysIterator.hasNext()) {
+ final String key = (String) keysIterator.next();
+ final ColumnType type = getTypeFromValue(data, key);
+ columnTypes.add(type);
+ removeIfAvailable(columnNames, key);
+ }
+ }
+ }
+
+ private void fillColumnTypesFromMetadata(final JSONObject jsonObject, final List<String> columnNames,
+ final List<ColumnType> columnTypes) throws JSONException {
+ final String metadataKey = "metadata";
+
+ if (jsonObject.has(metadataKey)) {
+ final JSONObject metadata = jsonObject.getJSONObject(metadataKey);
+
+ if (metadata.has("id")) {
+ columnTypes.add(ColumnType.BIGINT);
+ removeIfAvailable(columnNames, "_id");
+ }
+ }
+ }
+
+ private void fillColumnTypesFromRemainingColumns(final List<String> columnNames,
+ final List<ColumnType> columnTypes) {
+ for (final String remainingColumnName : columnNames) {
+ if (remainingColumnName.contains("rel_")) {
+ if (remainingColumnName.contains("#")) {
+ columnTypes.add(ColumnType.ARRAY);
+ } else {
+ columnTypes.add(ColumnType.BIGINT);
+ }
+ } else {
+ columnTypes.add(ColumnType.STRING);
+ }
+ }
+ }
+
+ private void removeIfAvailable(final List<String> list, final String key) {
+ if (list.contains(key)) {
+ list.remove(key);
+ }
+ }
+
+ private ColumnType getTypeFromValue(final JSONObject data, final String key) {
+ try {
+ final Class<? extends Object> keyClass = data.get(key).getClass();
+
+ if (keyClass.equals(Boolean.class)) {
+ return ColumnType.BOOLEAN;
+ } else if (keyClass.equals(Integer.class)) {
+ return ColumnType.INTEGER;
+ } else if (keyClass.equals(Long.class)) {
+ return ColumnType.BIGINT;
+ } else if (keyClass.equals(Double.class)) {
+ return ColumnType.DOUBLE;
+ } else if (keyClass.equals(JSONArray.class)) {
+ return ColumnType.ARRAY;
+ } else if (keyClass.equals(JSONObject.class)) {
--- End diff --
In practice a property will never be a JSONObject (or MAP), because Neo4j informed me that `Neo.ClientError.Statement.TypeError: Property values can only be of primitive types or arrays thereof`, so I think we can remove this option.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r189885903
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/Neo4jDataContext.java ---
@@ -158,60 +159,79 @@ protected String getMainSchemaName() throws MetaModelException {
}
public SimpleTableDef[] detectTableDefs() {
- List<SimpleTableDef> tableDefs = new ArrayList<SimpleTableDef>();
-
- String labelsJsonString = _requestWrapper.executeRestRequest(new HttpGet(_serviceRoot + "/labels"));
-
- JSONArray labelsJsonArray;
+ final List<SimpleTableDef> tableDefs = new ArrayList<>();
+ final String labelsJsonString = _requestWrapper.executeRestRequest(new HttpGet(_serviceRoot + "/labels"));
+ final JSONArray labelsJsonArray;
+
try {
labelsJsonArray = new JSONArray(labelsJsonString);
+
for (int i = 0; i < labelsJsonArray.length(); i++) {
- String label = labelsJsonArray.getString(i);
+ fillTableDefsFromLabel(labelsJsonArray.getString(i), tableDefs);
--- End diff --
The `fillTableDefsFromLabel` method doesn't fill the `tableDefs` list, it just adds one `SimpleTableDef` in case a `SimpleTableDef` can be created for the given label. I would rather see a method called `getTableDefForLabel` or `createTableDefFromLabel`, which returns a `SimpleTableDef` (or null in case none can be created for the given label) and adds it to the `tableDefs` list if the value is not null. From my point of view, in that manner that method has one simple responsibility and the code is easier to understand.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metamodel/pull/179
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r190897254
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ColumnTypeResolver.java ---
@@ -0,0 +1,126 @@
+/**
+ * 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.neo4j.utils;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.json.JSONArray;
+import org.json.JSONException;
+import org.json.JSONObject;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ColumnTypeResolver {
+ private static final Logger logger = LoggerFactory.getLogger(ColumnTypeResolver.class);
+ private final JSONObject _jsonObject;
+ private final List<String> _columnNames = new ArrayList<>();
+ private final List<ColumnType> _columnTypes = new ArrayList<>();
+
+ public ColumnTypeResolver(final JSONObject jsonObject, final String[] columnNamesArray) {
+ _jsonObject = jsonObject;
+ _columnNames.addAll(Arrays.asList(columnNamesArray));
+ }
+
+ public ColumnType[] getColumnTypes() {
+ try {
+ fillColumnTypesFromMetadata();
+ fillColumnTypesFromData();
+ } catch (final JSONException e) {
+ // ignore missing data
+ }
+
+ fillColumnTypesFromRemainingColumns();
+ return _columnTypes.toArray(new ColumnType[_columnTypes.size()]);
+ }
+
+ private void fillColumnTypesFromData() throws JSONException {
+ final String dataKey = "data";
+
+ if (_jsonObject.has(dataKey)) {
+ final JSONObject data = _jsonObject.getJSONObject(dataKey);
+ final Iterator<?> keysIterator = data.keys();
+
+ while (keysIterator.hasNext()) {
+ final String key = (String) keysIterator.next();
+ final ColumnType type = getTypeFromValue(data, key);
+ _columnTypes.add(type);
+ removeIfAvailable(_columnNames, key);
+ }
+ }
+ }
+
+ private void fillColumnTypesFromMetadata() throws JSONException {
+ final String metadataKey = "metadata";
+
+ if (_jsonObject.has(metadataKey)) {
+ final JSONObject metadata = _jsonObject.getJSONObject(metadataKey);
+
+ if (metadata.has("id")) {
--- End diff --
I would prefer that these String literals where defined as constants in the top of the class. With a name that describes them a bit, e.g. `NEO4J_METADATA_KEY_ID` (because it's something that neo4j exposes) and `COLUMN_ID` (for our own "_id" column).
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r190879379
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ColumnTypeResolver.java ---
@@ -0,0 +1,126 @@
+/**
+ * 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.neo4j.utils;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.json.JSONArray;
+import org.json.JSONException;
+import org.json.JSONObject;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ColumnTypeResolver {
+ private static final Logger logger = LoggerFactory.getLogger(ColumnTypeResolver.class);
+ private final JSONObject _jsonObject;
+ private final List<String> _columnNames = new ArrayList<>();
+ private final List<ColumnType> _columnTypes = new ArrayList<>();
--- End diff --
Can you move this field to a local variable inside the getColumnTypes() method and refactor the `fillColumnTypesFromMetadata`, `fillColumnTypesFromData` and `fillColumnTypesFromRemainingColumns`, so they all return a List of ColumnType objects instead of filling this field, then renaming them to `get...` or `create...` instead?
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r190899732
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/Neo4jDataSet.java ---
@@ -44,28 +44,32 @@ public Neo4jDataSet(List<SelectItem> selectItems, JSONObject resultJSONObject) {
@Override
public boolean next() {
try {
- JSONArray resultsArray = _resultJSONObject.getJSONArray("results");
+ final JSONArray resultsArray = _resultJSONObject.getJSONArray("results");
+
if (resultsArray.length() > 0) {
- JSONObject results = resultsArray.getJSONObject(0);
- JSONArray data = results.getJSONArray("data");
+ final JSONObject results = resultsArray.getJSONObject(0);
+ final JSONArray data = results.getJSONArray("data");
+
if (_currentRowIndex < data.length()) {
- JSONObject row = data.getJSONObject(_currentRowIndex);
- JSONArray jsonValues = row.getJSONArray("row");
-
- Object[] objectValues = new Object[jsonValues.length()];
+ final JSONObject row = data.getJSONObject(_currentRowIndex);
+ final JSONArray jsonValues = row.getJSONArray("row");
+ final Object[] objectValues = new Object[jsonValues.length()];
+
for (int i = 0; i < jsonValues.length(); i++) {
- objectValues[i] = jsonValues.getString(i);
+ objectValues[i] = jsonValues.get(i);
--- End diff --
Now that the `ColumnTypeResolver` resolves JSONArray objects to List objects, we should add some logic here that the values which are actually returned are of the type List.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r190877953
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/ColumnTypeResolver.java ---
@@ -0,0 +1,126 @@
+/**
+ * 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.neo4j.utils;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.json.JSONArray;
+import org.json.JSONException;
+import org.json.JSONObject;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ColumnTypeResolver {
+ private static final Logger logger = LoggerFactory.getLogger(ColumnTypeResolver.class);
+ private final JSONObject _jsonObject;
+ private final List<String> _columnNames = new ArrayList<>();
+ private final List<ColumnType> _columnTypes = new ArrayList<>();
+
+ public ColumnTypeResolver(final JSONObject jsonObject, final String[] columnNamesArray) {
--- End diff --
Why use an array for the `columnNamesArray` parameter, why not use a List instead? The `ColumnTypeResolver` is only instantiated once on line 201 in the `Neo4jDataContext` and there you already have a List of column names, which you have to convert to an Array (on line 200) to be able to pass it to the constructor and then here inside the constructor you convert it back into a List right away, that seems like two unnecessary conversion to me.
---
[GitHub] metamodel pull request #179: Feature/neo4j column types
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/179#discussion_r189915253
--- Diff: neo4j/src/main/java/org/apache/metamodel/neo4j/utils/AbstractColumnTypeHandler.java ---
@@ -0,0 +1,33 @@
+/**
+ * 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.neo4j.utils;
+
+import org.apache.metamodel.schema.ColumnType;
+import org.json.JSONObject;
+
+public abstract class AbstractColumnTypeHandler implements ColumnTypeHandler {
+ protected ColumnTypeHandler _successor;
+
+ @Override
+ public void setSuccessor(final ColumnTypeHandler successor) {
--- End diff --
In theory this setSuccessor method makes the code very flexible, but in this use case it adds flexibility which is not actually used, because each type of `ColumnTypeHandler` has one successor, which is now non-statically defined in the ColumnTypeResolver#getTypeFromValue(JSONObject, String) method, but which is actually static and could as well be defined within the class's definition.
---
[GitHub] metamodel issue #179: Feature/neo4j column types
Posted by jhorcicka <gi...@git.apache.org>.
Github user jhorcicka commented on the issue:
https://github.com/apache/metamodel/pull/179
@arjansh, @kaspersorensen: Updated.
I plan to create another PR to fix some code style issues like missing "final" keywords after this one is accepted.
---