DRILL-7098: File Metadata Metastore Plugin #1754

vdiravka · 2019-04-17T13:28:31Z

Introducing Drill Metastore module with main Metastore API. It also should be used for creating Drill Metastore plugins, they can be created as separate modules inside main Metastore module.
The main reason of this improvement is decoupling drill-exec and drill-metastore code.
drill-metastore is used as dependency for drill-exec.

vvysotskyi

@vdiravka, there are two classes which I think it is better to leave in the exec module: SchemaContainer and SchemaTransformer.

Also, there are some other classes which would be good to move to the metastore-api module: TableMetadataProvider, MetadataProviderManager, TableMetadataProviderBuilder and others. Could you please move them also?

vvysotskyi · 2019-04-17T15:50:23Z

exec/java-exec/src/main/java/org/apache/drill/exec/expr/IsPredicate.java

@@ -76,7 +76,7 @@ static boolean isNullOrEmpty(ColumnStatistics stat) {
        || !stat.containsStatistic(ColumnStatisticsKind.MIN_VALUE)
        || !stat.containsStatistic(ColumnStatisticsKind.MAX_VALUE)
        || !stat.containsStatistic(ColumnStatisticsKind.NULLS_COUNT)
-        || (long) stat.getStatistic(ColumnStatisticsKind.NULLS_COUNT) == GroupScan.NO_COLUMN_STATS;
+        || (long) stat.getStatistic(ColumnStatisticsKind.NULLS_COUNT) ==  Statistic.NO_COLUMN_STATS;


Looks like there are some extra spaces before Statistic interface here and in other places were added. Could you please remove them?

Thanks. Done.

vdiravka

TableMetadataProviderBuilder and TableMetadataProvider are moved to metastore module.
MetadataProviderManager depends on DrillStatsTable, which hardly coupled with java-exec. I will consider of creating additional abstractions here. I will update this answer then.

I have returned SchemaContainer and SchemaTransformer, but if MetadataProviderManager is moved to metastore module, SchemaProvider along with SchemaContainer should be in metastore module too.

vdiravka · 2019-04-21T11:34:28Z

exec/java-exec/src/main/java/org/apache/drill/exec/expr/IsPredicate.java

@@ -76,7 +76,7 @@ static boolean isNullOrEmpty(ColumnStatistics stat) {
        || !stat.containsStatistic(ColumnStatisticsKind.MIN_VALUE)
        || !stat.containsStatistic(ColumnStatisticsKind.MAX_VALUE)
        || !stat.containsStatistic(ColumnStatisticsKind.NULLS_COUNT)
-        || (long) stat.getStatistic(ColumnStatisticsKind.NULLS_COUNT) == GroupScan.NO_COLUMN_STATS;
+        || (long) stat.getStatistic(ColumnStatisticsKind.NULLS_COUNT) ==  Statistic.NO_COLUMN_STATS;


Thanks. Done.

vvysotskyi

Added minor comments connected with formatting.

I have a question regarding packing these new modules into jdbc-all jar. Looks like a client should not use them, but they weren't excluded from the java-exec module in jdbc-all pom file. Should they be excluded, or they should be specified explicitly to be put into the jar?

vvysotskyi · 2019-04-22T08:04:10Z

...va-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractGroupScanWithMetadata.java

@@ -151,13 +152,13 @@ public long getColumnValueCount(SchemaPath column) {
    long colNulls;
    if (columnStats != null) {
      Long nulls = (Long) columnStats.getStatistic(ColumnStatisticsKind.NULLS_COUNT);
-      colNulls = nulls != null ? nulls : GroupScan.NO_COLUMN_STATS;
+      colNulls = nulls != null ? nulls :  Statistic.NO_COLUMN_STATS;


Please remove extra space here:

Suggested change

colNulls = nulls != null ? nulls : Statistic.NO_COLUMN_STATS;

colNulls = nulls != null ? nulls : Statistic.NO_COLUMN_STATS;

vvysotskyi · 2019-04-22T08:04:26Z

...va-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractGroupScanWithMetadata.java

-    return GroupScan.NO_COLUMN_STATS == tableRowCount
-        || GroupScan.NO_COLUMN_STATS == colNulls
-        ? GroupScan.NO_COLUMN_STATS : tableRowCount - colNulls;
+    return  Statistic.NO_COLUMN_STATS == tableRowCount


And here

Suggested change

return Statistic.NO_COLUMN_STATS == tableRowCount

return Statistic.NO_COLUMN_STATS == tableRowCount

vvysotskyi · 2019-04-22T08:04:42Z

...va-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractGroupScanWithMetadata.java

-        || GroupScan.NO_COLUMN_STATS == colNulls
-        ? GroupScan.NO_COLUMN_STATS : tableRowCount - colNulls;
+    return  Statistic.NO_COLUMN_STATS == tableRowCount
+        ||  Statistic.NO_COLUMN_STATS == colNulls


Suggested change

|| Statistic.NO_COLUMN_STATS == colNulls

|| Statistic.NO_COLUMN_STATS == colNulls

vvysotskyi · 2019-04-22T08:04:50Z

...va-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractGroupScanWithMetadata.java

-        ? GroupScan.NO_COLUMN_STATS : tableRowCount - colNulls;
+    return  Statistic.NO_COLUMN_STATS == tableRowCount
+        ||  Statistic.NO_COLUMN_STATS == colNulls
+        ?  Statistic.NO_COLUMN_STATS : tableRowCount - colNulls;


Suggested change

? Statistic.NO_COLUMN_STATS : tableRowCount - colNulls;

? Statistic.NO_COLUMN_STATS : tableRowCount - colNulls;

vvysotskyi · 2019-04-22T08:05:33Z

...java-exec/src/main/java/org/apache/drill/exec/store/parquet/BaseParquetMetadataProvider.java

      } else {
        // merges specified schema with schema from table
        fields.forEach((schemaPath, majorType) -> {
          if (SchemaPathUtils.getColumnMetadata(schemaPath, schema) == null) {
-            SchemaPathUtils.addColumnMetadata(schema, schemaPath, majorType);
+              MetadataUtils.addColumnMetadata(schema, schemaPath, majorType);


Suggested change

MetadataUtils.addColumnMetadata(schema, schemaPath, majorType);

MetadataUtils.addColumnMetadata(schema, schemaPath, majorType);

vvysotskyi · 2019-04-22T08:09:28Z

metastore/metastore-api/src/main/java/org/apache/drill/metastore/TableStatisticsKind.java

@@ -37,8 +36,8 @@ public Long mergeStatistics(Collection<? extends BaseMetadata> statistics) {
      long rowCount = 0;
      for (BaseMetadata statistic : statistics) {
        Long statRowCount = getValue(statistic);
-        if (statRowCount == null || statRowCount == GroupScan.NO_COLUMN_STATS) {
-          rowCount = GroupScan.NO_COLUMN_STATS;
+        if (statRowCount == null || statRowCount ==  Statistic.NO_COLUMN_STATS) {


Suggested change

if (statRowCount == null || statRowCount == Statistic.NO_COLUMN_STATS) {

if (statRowCount == null || statRowCount == Statistic.NO_COLUMN_STATS) {

vvysotskyi · 2019-04-22T08:09:35Z

metastore/metastore-api/src/main/java/org/apache/drill/metastore/TableStatisticsKind.java

-        if (statRowCount == null || statRowCount == GroupScan.NO_COLUMN_STATS) {
-          rowCount = GroupScan.NO_COLUMN_STATS;
+        if (statRowCount == null || statRowCount ==  Statistic.NO_COLUMN_STATS) {
+          rowCount =  Statistic.NO_COLUMN_STATS;


Suggested change

rowCount = Statistic.NO_COLUMN_STATS;

rowCount = Statistic.NO_COLUMN_STATS;

vvysotskyi · 2019-04-22T08:09:43Z

metastore/metastore-api/src/main/java/org/apache/drill/metastore/TableStatisticsKind.java

@@ -50,7 +49,7 @@ public Long mergeStatistics(Collection<? extends BaseMetadata> statistics) {
    @Override
    public Long getValue(BaseMetadata metadata) {
      Long rowCount = (Long) metadata.getStatistic(this);
-      return rowCount != null ? rowCount : GroupScan.NO_COLUMN_STATS;
+      return rowCount != null ? rowCount :  Statistic.NO_COLUMN_STATS;


Suggested change

return rowCount != null ? rowCount : Statistic.NO_COLUMN_STATS;

return rowCount != null ? rowCount : Statistic.NO_COLUMN_STATS;

vdiravka

To move MetadataProviderManager to metastore it is necessary to rework Stats classes. Since stats should be stored in Drill Metastore, additional abstractions should be created later.
drill-metastore classes are removed from jdbc-all, they are not needed for Drill clinets now. Possibly it can be reconsidered in future.
Spaces are removed, thanks

vvysotskyi

Thanks for making the PR and addressing CR comments.
Changes look good, +1.
Also, please squash the commits.

closes apache#1754

vdiravka · 2019-05-03T09:44:16Z

The commits are squashed. The branch is rebased onto Drill master branch

closes apache#1754

vdiravka requested a review from vvysotskyi April 17, 2019 13:28

vdiravka force-pushed the DRILL-7098 branch from 0358d65 to de3c4af Compare April 17, 2019 21:49

vvysotskyi reviewed Apr 18, 2019

View reviewed changes

vdiravka force-pushed the DRILL-7098 branch from de3c4af to 7f981a7 Compare April 21, 2019 12:13

vdiravka commented Apr 21, 2019

View reviewed changes

vvysotskyi reviewed Apr 22, 2019

View reviewed changes

vdiravka force-pushed the DRILL-7098 branch from 7f981a7 to c823638 Compare May 1, 2019 21:25

vdiravka commented May 1, 2019

View reviewed changes

vvysotskyi approved these changes May 2, 2019

View reviewed changes

sohami pushed a commit to sohami/drill that referenced this pull request May 2, 2019

DRILL-7098: File Metadata Metastore Plugin

3b1427d

closes apache#1754

sohami pushed a commit to sohami/drill that referenced this pull request May 2, 2019

DRILL-7098: File Metadata Metastore Plugin

56cbdb6

closes apache#1754

vdiravka force-pushed the DRILL-7098 branch 2 times, most recently from 1b1537d to bfc968b Compare May 3, 2019 09:16

DRILL-7098: File Metadata Metastore Plugin

cbf8dab

vdiravka force-pushed the DRILL-7098 branch from bfc968b to cbf8dab Compare May 3, 2019 09:42

sohami pushed a commit to sohami/drill that referenced this pull request May 3, 2019

DRILL-7098: File Metadata Metastore Plugin

f45f8e3

closes apache#1754

sohami closed this in 108ced0 May 3, 2019

lushuifeng pushed a commit to lushuifeng/drill that referenced this pull request Jun 21, 2019

DRILL-7098: File Metadata Metastore Plugin

6109343

closes apache#1754

xiangt920 pushed a commit to xiangt920/drill that referenced this pull request Dec 26, 2019

DRILL-7098: File Metadata Metastore Plugin

8841a1b

closes apache#1754

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRILL-7098: File Metadata Metastore Plugin #1754

DRILL-7098: File Metadata Metastore Plugin #1754

vdiravka commented Apr 17, 2019

vvysotskyi left a comment

vvysotskyi Apr 17, 2019

vdiravka Apr 21, 2019

vdiravka left a comment

vdiravka Apr 21, 2019

vvysotskyi left a comment

vvysotskyi Apr 22, 2019

vvysotskyi Apr 22, 2019

vvysotskyi Apr 22, 2019

vvysotskyi Apr 22, 2019

vvysotskyi Apr 22, 2019

vvysotskyi Apr 22, 2019

vvysotskyi Apr 22, 2019

vvysotskyi Apr 22, 2019

vdiravka left a comment

vvysotskyi left a comment

vdiravka commented May 3, 2019

	colNulls = nulls != null ? nulls : Statistic.NO_COLUMN_STATS;
	colNulls = nulls != null ? nulls : Statistic.NO_COLUMN_STATS;

	return Statistic.NO_COLUMN_STATS == tableRowCount
	return Statistic.NO_COLUMN_STATS == tableRowCount

	\|\| Statistic.NO_COLUMN_STATS == colNulls
	\|\| Statistic.NO_COLUMN_STATS == colNulls

	? Statistic.NO_COLUMN_STATS : tableRowCount - colNulls;
	? Statistic.NO_COLUMN_STATS : tableRowCount - colNulls;

	MetadataUtils.addColumnMetadata(schema, schemaPath, majorType);
	MetadataUtils.addColumnMetadata(schema, schemaPath, majorType);

	if (statRowCount == null \|\| statRowCount == Statistic.NO_COLUMN_STATS) {
	if (statRowCount == null \|\| statRowCount == Statistic.NO_COLUMN_STATS) {

	rowCount = Statistic.NO_COLUMN_STATS;
	rowCount = Statistic.NO_COLUMN_STATS;

	return rowCount != null ? rowCount : Statistic.NO_COLUMN_STATS;
	return rowCount != null ? rowCount : Statistic.NO_COLUMN_STATS;

DRILL-7098: File Metadata Metastore Plugin #1754

DRILL-7098: File Metadata Metastore Plugin #1754

Conversation

vdiravka commented Apr 17, 2019

vvysotskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdiravka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvysotskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdiravka left a comment

Choose a reason for hiding this comment

vvysotskyi left a comment

Choose a reason for hiding this comment

vdiravka commented May 3, 2019