[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
