----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31066/#review75675 -----------------------------------------------------------
Looks good, except the noted issues. But it's a big change, so it probably needs more (and more MM experienced) eyes. elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchCreateTableBuilder.java <https://reviews.apache.org/r/31066/#comment122903> Reject mapping instead, _object_ is not useable for this purpose elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchCreateTableBuilder.java <https://reviews.apache.org/r/31066/#comment122904> Reject mapping instead, _object_ is not useable as a generic type. elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchDeleteBuilder.java <https://reviews.apache.org/r/31066/#comment122900> createQueryBuilderForSimpleWhere already does this for you. No need to do it again. - Dennis Krøger On March 8, 2015, 3:12 p.m., Kasper Sørensen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31066/ > ----------------------------------------------------------- > > (Updated March 8, 2015, 3:12 p.m.) > > > Review request for MetaModel. > > > Bugs: METAMODEL-79 > https://issues.apache.org/jira/browse/METAMODEL-79 > > > Repository: metamodel > > > Description > ------- > > Partial fix for METAMODEL-79 - see description > > > Diffs > ----- > > CHANGES.md 5c2e2eb > core/src/main/java/org/apache/metamodel/MetaModelHelper.java 4ee9798 > core/src/main/java/org/apache/metamodel/QueryPostprocessDataContext.java > 0eba921 > core/src/main/java/org/apache/metamodel/query/FilterClause.java fe981a1 > > core/src/test/java/org/apache/metamodel/QueryPostprocessDataContextTest.java > 3fdb711 > core/src/test/java/org/apache/metamodel/query/FilterItemTest.java 2fedfb9 > > elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchCreateTableBuilder.java > PRE-CREATION > > elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchDataContext.java > 06353f1 > > elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchDataSet.java > e4f1054 > > elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchDeleteBuilder.java > PRE-CREATION > > elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchDropTableBuilder.java > PRE-CREATION > > elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchInsertBuilder.java > PRE-CREATION > > elasticsearch/src/main/java/org/apache/metamodel/elasticsearch/ElasticSearchUpdateCallback.java > PRE-CREATION > > elasticsearch/src/test/java/org/apache/metamodel/elasticsearch/ElasticSearchDataContextTest.java > 449490b > > Diff: https://reviews.apache.org/r/31066/diff/ > > > Testing > ------- > > This is my initial/partial fix for METAMODEL-79. I want to share it because > 1) there's more to come but I want to confirm that I'm on the right way and > 2) I have questions for experts on E.S. :-) > > This patch adds support for the MetaModel ElasticSearch to do INSERT INTO, > CREATE TABLE and DROP TABLE statements. > > It does not (yet) have support for UPDATE or DELETE FROM statements. I wanted > to validate my initial work first. > > And I have a few questions regarding types and mappings. > > * Please check the ElasticSearchCreateTableBuilder.getType(Column) method. > Here I've attempted to convert ColumnTypes to ElasticSearch types. Are these > correct? I am not sure about generalizations such as the NUMERIC -> "double" > mapping etc. > * As a last resort I have used the type "object". But when I tried it out I > ran into the problem that "object" is not polymorphic like in Java, it is > actually the opposite of a "value type". So that means you cannot define an > "object" field and then insert a single value into it. This makes it a bad > fit for a "fallback" type. Is there a better way? Should we then simply NOT > define the field in the mapping maybe? > > > Thanks, > > Kasper Sørensen > >
