[GitHub] vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table
vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table URL: https://github.com/apache/drill/pull/1602#discussion_r248775891 ## File path: exec/java-exec/src/test/resources/view/vw_before_drill_6944.view.drill ## @@ -0,0 +1,10 @@ +{ + "name" : "vw_before_drill_6944", + "sql" : "SELECT `mapf`\nFROM dfs.`avro/map_string_to_long.avro`", + "fields" : [ { +"name" : "mapf", +"type" : "MAP", +"isNullable" : false + } ], + "workspaceSchemaPath" : [ ] +} Review comment: Good point. I think the proper way to generate it by Drill in the process of creating view. It is a good style to always put the newline as a last character if it is allowed by the file format. For some tools it can be the marker of EOF. Anyway it is pretty minor, since Drill views can be consumed only by Drill. You can leave it as is. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table
vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table URL: https://github.com/apache/drill/pull/1602#discussion_r248635359 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/dotdrill/View.java ## @@ -127,6 +135,8 @@ public FieldType(String name, RelDataType dataType) { default: break; } + keyType = dataType.getKeyType() == null ? null : new FieldType(null, dataType.getKeyType()); + valueType = dataType.getValueType() == null ? null : new FieldType(null, dataType.getValueType()); Review comment: I confused Field Type Name with Filed Name. Filed Name should be used for `FiledType`. The name of `FiledType` is confusing. Consider renaming `FiledType` to `Filed`. Regarding null in constructor, I agree there is no need to store the key, value names for every internal MAP (it will be not a view, but dataset then). But I think it will be good to introduce the new constructor without Field Name. This constructor will serve for representing internal Fields of complex Fields (MAP now, maybe List in future) in the View. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table
vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table URL: https://github.com/apache/drill/pull/1602#discussion_r248336156 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/dotdrill/View.java ## @@ -242,32 +269,38 @@ public RelDataType getRowType(RelDataTypeFactory factory) { return new RelDataTypeDrillImpl(new RelDataTypeHolder(), factory); } -List types = Lists.newArrayList(); -List names = Lists.newArrayList(); - +List types = new ArrayList<>(fields.size()); +List names = new ArrayList<>(fields.size()); for (FieldType field : fields) { names.add(field.getName()); - RelDataType type; - if ( SqlTypeFamily.INTERVAL_YEAR_MONTH == field.getType().getFamily() - || SqlTypeFamily.INTERVAL_DAY_TIME == field.getType().getFamily() ) { - type = factory.createSqlIntervalType( field.getIntervalQualifier() ); - } else if (field.getPrecision() == null && field.getScale() == null) { -type = factory.createSqlType(field.getType()); - } else if (field.getPrecision() != null && field.getScale() == null) { -type = factory.createSqlType(field.getType(), field.getPrecision()); - } else { -type = factory.createSqlType(field.getType(), field.getPrecision(), field.getScale()); - } - - if (field.getIsNullable()) { -types.add(factory.createTypeWithNullability(type, true)); - } else { -types.add(type); - } + types.add(getType(field, factory)); } return factory.createStructType(types, names); } + private RelDataType getType(FieldType field, RelDataTypeFactory factory) { +RelDataType type; +final SqlTypeName typeName = field.getType(); +final Integer precision = field.getPrecision(); +final Integer scale = field.getScale(); + +if (field.isInterval()) { + type = factory.createSqlIntervalType(field.getIntervalQualifier()); +} else if (nonNull(precision)) { + type = nonNull(scale) + ? factory.createSqlType(typeName, precision, scale) + : factory.createSqlType(typeName, precision); +} else if (typeName == SqlTypeName.MAP) { + type = field.isMapTypesPresent() + ? factory.createMapType(getType(field.getKeyType(), factory), getType(field.getValueType(), factory)) + : factory.createSqlType(SqlTypeName.ANY); Review comment: Could you please put a comment here why `ANY` is used in case of untyped `MAP`? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table
vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table URL: https://github.com/apache/drill/pull/1602#discussion_r248320140 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/dotdrill/View.java ## @@ -127,6 +135,8 @@ public FieldType(String name, RelDataType dataType) { default: break; } + keyType = dataType.getKeyType() == null ? null : new FieldType(null, dataType.getKeyType()); + valueType = dataType.getValueType() == null ? null : new FieldType(null, dataType.getValueType()); Review comment: why don't use `dataType.getSqlTypeName().getName()` instead of `null`? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table
vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table URL: https://github.com/apache/drill/pull/1602#discussion_r248333066 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/dotdrill/View.java ## @@ -127,6 +135,8 @@ public FieldType(String name, RelDataType dataType) { default: break; } + keyType = dataType.getKeyType() == null ? null : new FieldType(null, dataType.getKeyType()); Review comment: Should `keyType` and `valueType` be used with other data types than `MAP`? Looks like you can add `MAP` case for swtich statement and place initializing of the new fields there. _Note_: looks like `precision`, `sacle` and `intervalQualifier` also can be used only for specific data types, but if you are not sure, just leave them as is. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table
vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table URL: https://github.com/apache/drill/pull/1602#discussion_r248329753 ## File path: exec/java-exec/src/main/java/org/apache/drill/exec/dotdrill/View.java ## @@ -207,6 +217,23 @@ public Boolean getIsNullable() { return isNullable; } +public FieldType getKeyType() { Review comment: Please add JavaDocs. All above similar methods have them. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table
vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table URL: https://github.com/apache/drill/pull/1602#discussion_r248324527 ## File path: exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestViewSupport.java ## @@ -800,4 +801,32 @@ public void testViewIsFoundWithinWorkspaceWhenNameStartsWithSlash() throws Excep test("DROP VIEW IF EXISTS `%s`.`%s`", DFS_TMP_SCHEMA, viewName); } } + + @Test // DRILL-6944 + public void testMapColumnOfNewlyCreatedView() throws Exception { +try { + test("CREATE VIEW dfs.tmp.`mapf_view` AS SELECT mapf FROM dfs.`avro/map_string_to_long.avro`"); + test("SELECT * FROM dfs.tmp.`mapf_view`"); + testBuilder() + .sqlQuery("SELECT mapf['ki'] as ki FROM dfs.tmp.`mapf_view`") Review comment: Please escape `mapf` with back ticks or rename to `map_f`. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table
vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table URL: https://github.com/apache/drill/pull/1602#discussion_r248338167 ## File path: exec/java-exec/src/test/resources/view/vw_before_drill_6944.view.drill ## @@ -0,0 +1,10 @@ +{ + "name" : "vw_before_drill_6944", + "sql" : "SELECT `mapf`\nFROM dfs.`avro/map_string_to_long.avro`", + "fields" : [ { +"name" : "mapf", +"type" : "MAP", +"isNullable" : false + } ], + "workspaceSchemaPath" : [ ] +} Review comment: New line This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table
vdiravka commented on a change in pull request #1602: DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table URL: https://github.com/apache/drill/pull/1602#discussion_r248321769 ## File path: exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestViewSupport.java ## @@ -800,4 +801,32 @@ public void testViewIsFoundWithinWorkspaceWhenNameStartsWithSlash() throws Excep test("DROP VIEW IF EXISTS `%s`.`%s`", DFS_TMP_SCHEMA, viewName); } } + + @Test // DRILL-6944 + public void testMapColumnOfNewlyCreatedView() throws Exception { +try { + test("CREATE VIEW dfs.tmp.`mapf_view` AS SELECT mapf FROM dfs.`avro/map_string_to_long.avro`"); Review comment: I tried to create view for Parquet and Json with complex data types and the view involves `ANY` instead of `MAP` datatype in the result. Do you know the reason of that? What is the DESCRIBE output for this view for Avro? Does it involve `MAP`, not `ANY`? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services