DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table #1602

ihuzenko · 2019-01-08T14:00:23Z

Added persistence of MAP key and value types in Drill views (affects .view.drill file) for avoiding cast problems in future.
Preserved backward compatibility of older view files by treating untyped maps as ANY.

arina-ielchiieva · 2019-01-14T09:09:30Z

@vdiravka could you please review?

vdiravka · 2019-01-16T15:12:46Z

exec/java-exec/src/main/java/org/apache/drill/exec/dotdrill/View.java

        break;
      }
+      keyType = dataType.getKeyType() == null ? null : new FieldType(null, dataType.getKeyType());
+      valueType = dataType.getValueType() == null ? null : new FieldType(null, dataType.getValueType());


why don't use dataType.getSqlTypeName().getName() instead of null?

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.

yes, good idea. Done the changes.

vdiravka · 2019-01-16T15:16:15Z

exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestViewSupport.java

+  @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`");


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?

Yes, I did some experiments with map column in avro and figured out that Drill detected MAP type exactly same as for binary table. And bug DRILL-6944 was reproduced for view created over avro file before I made the fix (this fact mentioned in Jira also). So I think we can rely on added tests to ensure that issue is gone.

vdiravka · 2019-01-16T15:22:24Z

exec/java-exec/src/test/java/org/apache/drill/exec/sql/TestViewSupport.java

+      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`")


Please escape mapf with back ticks or rename to map_f.

vdiravka · 2019-01-16T15:33:58Z

exec/java-exec/src/main/java/org/apache/drill/exec/dotdrill/View.java

      return isNullable;
    }

+    public FieldType getKeyType() {


Please add JavaDocs. All above similar methods have them.

added javadocs and @JsonIgnore for methods which won't take part in serialization.

vdiravka · 2019-01-16T15:41:11Z

exec/java-exec/src/main/java/org/apache/drill/exec/dotdrill/View.java

      default:
        break;
      }
+      keyType = dataType.getKeyType() == null ? null : new FieldType(null, dataType.getKeyType());


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.

Yes it's for maps only, good catch. Added new case for MAP above.

vdiravka · 2019-01-16T15:48:00Z

exec/java-exec/src/main/java/org/apache/drill/exec/dotdrill/View.java

+    } else if (typeName == SqlTypeName.MAP) {
+      type = field.isMapTypesPresent()
+          ? factory.createMapType(getType(field.getKeyType(), factory), getType(field.getValueType(), factory))
+          : factory.createSqlType(SqlTypeName.ANY);


Could you please put a comment here why ANY is used in case of untyped MAP?

Added explanation comment.

vdiravka · 2019-01-16T15:52:19Z

exec/java-exec/src/test/resources/view/vw_before_drill_6944.view.drill

+    "isNullable" : false
+  } ],
+  "workspaceSchemaPath" : [ ]
+}


I'm not sure whether it's required to add new lines for non source files
generated by Drill and used only for testing purposes.

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.

…B binary table 1. Added persistence of MAP key and value types in Drill views (affects .view.drill file) for avoiding cast problems in future. 2. Preserved backward compatibility of older view files by treating untyped maps as ANY.

ihuzenko · 2019-01-17T16:29:34Z

@vdiravka I've tried to address your comments, could you please review again ?

vdiravka

+1 LGTM

vdiravka · 2019-01-17T17:51:56Z

exec/java-exec/src/test/resources/view/vw_before_drill_6944.view.drill

+    "isNullable" : false
+  } ],
+  "workspaceSchemaPath" : [ ]
+}


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.

…B binary table 1. Added persistence of MAP key and value types in Drill views (affects .view.drill file) for avoiding cast problems in future. 2. Preserved backward compatibility of older view files by treating untyped maps as ANY. closes apache#1602

vdiravka reviewed Jan 16, 2019

View reviewed changes

ihuzenko force-pushed the DRILL-6944 branch from b4d4d3c to 8ad2986 Compare January 17, 2019 15:55

vdiravka approved these changes Jan 17, 2019

View reviewed changes

asfgit closed this in de863af Jan 18, 2019

DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table #1602

DRILL-6944: UnsupportedOperationException thrown for view over MapR-DB binary table #1602

Uh oh!

Conversation

ihuzenko commented Jan 8, 2019

Uh oh!

arina-ielchiieva commented Jan 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ihuzenko commented Jan 17, 2019

Uh oh!

vdiravka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants