Conversation
| </exclusion> | ||
| <exclusion> | ||
| <groupId>org.mortbay.jetty</groupId> | ||
| <artifactId>servlet-api</artifactId> |
There was a problem hiding this comment.
duplicate exclusion. Already excluded at the top
There was a problem hiding this comment.
You are right. Exclusion is removed. Thank you.
| hiveConfig.put(METASTORE_EXECUTE_SET_UGI.varname, hiveConf.get(METASTORE_EXECUTE_SET_UGI.varname)); | ||
| hiveConfig.put(HIVE_AUTHORIZATION_ENABLED.varname, hiveConf.get(HIVE_AUTHORIZATION_ENABLED.varname)); | ||
| hiveConfig.put(HIVE_AUTHENTICATOR_MANAGER.varname, SessionStateUserAuthenticator.class.getName()); | ||
| hiveConfig.put(HIVE_AUTHORIZATION_MANAGER.varname, SQLStdHiveAuthorizerFactory.class.getName()); |
There was a problem hiding this comment.
why is this removed ? The test seem to be for Authorization.
There was a problem hiding this comment.
Did it accidentally. I've returned this string.
| if (AcidUtils.isTablePropertyTransactional(properties)) { | ||
| AcidUtils.setTransactionalTableScan(job, true); | ||
| HiveUtilities.setColumnTypes(job, properties, true, sd); | ||
| } |
There was a problem hiding this comment.
How about refactoring this block of code to a new method in HiveUtilities ? Like verifyAndAddTransactionalProperty(). Then just call that method from both here and HiveAbstractReader
There was a problem hiding this comment.
It makes sense. Moreover since schema_evolution is required for acid tables HIVE-12799 I've combined it with setColumnTypes() helper method.
| colTypesBuilder.append(col.getType()); | ||
| } | ||
| colNames = colNamesBuilder.toString(); | ||
| colTypes = colTypesBuilder.toString(); |
There was a problem hiding this comment.
how about changing the loop as below:
final StringBuilder colNamesBuilder = new StringBuilder();
final StringBuilder colTypesBuilder = new StringBuilder();
for(FieldSchema col: sd.getCols()) {
colNamesBuilder.append(col.getName());
colTypesBuilder.append(col.getType());
colNamesBuilder.append(',');
colTypesBuilder.append(',');
}
colNames = colNamesBuilder.substring(0, colNamesBuilder.length() - 1);
colTypes = colTypesBuilder.substring(0, colTypesBuilder.length() - 1);
| hiveConfig.put(METASTORE_EXECUTE_SET_UGI.varname, hiveConf.get(METASTORE_EXECUTE_SET_UGI.varname)); | ||
| hiveConfig.put(HIVE_AUTHORIZATION_ENABLED.varname, hiveConf.get(HIVE_AUTHORIZATION_ENABLED.varname)); | ||
| hiveConfig.put(HIVE_AUTHENTICATOR_MANAGER.varname, SessionStateUserAuthenticator.class.getName()); | ||
| hiveConfig.put(HIVE_AUTHORIZATION_MANAGER.varname, SQLStdHiveAuthorizerFactory.class.getName()); |
There was a problem hiding this comment.
Did it accidentally. I've returned this string.
| </exclusion> | ||
| <exclusion> | ||
| <groupId>org.mortbay.jetty</groupId> | ||
| <artifactId>servlet-api</artifactId> |
There was a problem hiding this comment.
You are right. Exclusion is removed. Thank you.
| colTypesBuilder.append(col.getType()); | ||
| } | ||
| colNames = colNamesBuilder.toString(); | ||
| colTypes = colTypesBuilder.toString(); |
| if (AcidUtils.isTablePropertyTransactional(properties)) { | ||
| AcidUtils.setTransactionalTableScan(job, true); | ||
| HiveUtilities.setColumnTypes(job, properties, true, sd); | ||
| } |
There was a problem hiding this comment.
It makes sense. Moreover since schema_evolution is required for acid tables HIVE-12799 I've combined it with setColumnTypes() helper method.
| <dependency> | ||
| <groupId>org.apache.calcite</groupId> | ||
| <artifactId>calcite-core</artifactId> | ||
| <version>${calcite.version}</version> |
There was a problem hiding this comment.
Why is this change necessary? The version should come from <dependencyManagement> of the parent pom.
There was a problem hiding this comment.
hive-exec needs own calcite 1.6 version.
Calcite version in DependencyManagement of root pom leads to using Drill Calcite version over the whole project.
There was a problem hiding this comment.
Will it be better to explicitly specify the version of calcite-core in hive-exec?
There was a problem hiding this comment.
I have returned calcite-core version into DependencyManagement block. Drill Calcite version libraries are included into "drill-hive-exec-shaded" module.
I will tell you why it works for now:
When user submits query in Drill via Hive plugin, the query is validated and planned via Drill Calcite. So Hive Calcite isn't necessary for it.
Hive Calcite is used only in the process of Drill unit testing, where a lot of Hive specific queries are performed to setup Hive store for testing. But Drill Calcite and Avatica versions have conflicts with Hive old Calcite and Avatica versions. That's why I have disabled Calcite cost based optimizator conf.set(ConfVars.HIVE_CBO_ENABLED.varname, "false");. We can enable it again once Hive will leverage the newest Calcite version. The comment about it is added into drill-hive-exec-shaded POM.
| <dependency> | ||
| <groupId>org.apache.calcite</groupId> | ||
| <artifactId>calcite-core</artifactId> | ||
| <version>${calcite.version}</version> |
There was a problem hiding this comment.
The same question as for common/pom.xml.
| <groupId>commons-codec</groupId> | ||
| <artifactId>commons-codec</artifactId> | ||
| </exclusion> | ||
| <exclusion> |
There was a problem hiding this comment.
Is the exclusion necessary due to a version conflict or the dependency is not required?
There was a problem hiding this comment.
For a transitive dependency will it be better to use DependencyManagement? Please add comments with details.
There was a problem hiding this comment.
Both cases can resolve it. In details io.dropwizard.metrics:metrics-core is nowhere used in Drill. And this is a dependency for tephra-core and transitive for hive-metastore. But it conflicts with Drill's com.codahale.metrics.
hive-metastore uses 3.0.1 version of this dependency, but the last version in maven repository is 4.0.2.
I added this dependency to dependencyManagement block with 4.0.2 version and conflict is resolved as well. I think it is a better decision, because it can help to avoid similar conflicts in future.
Also metrics-core in hive-hbase-handler has not influence to Drill, so I've removed my exclusion of it.
| * @param properties table or partition properties | ||
| * @param sd storage descriptor | ||
| */ | ||
| public static void verifyAndAddTransactionalProperties(JobConf job, Properties properties, StorageDescriptor sd) { |
There was a problem hiding this comment.
Is it necessary to pass both JobConf and Properties? As far as I can see job is always populated using passed properties.
There was a problem hiding this comment.
job involves table properties, but JobConf hasn't any method to return that properties. It is possible to get only one property. But looks like this is not an issue.
There is in AcidUtils the isTablePropertyTransactional method with Configuration input parameter.
Changed. Thank you.
| colNames = job.get(serdeConstants.LIST_COLUMNS); | ||
| colTypes = job.get(serdeConstants.LIST_COLUMN_TYPES); | ||
| } else { | ||
| final StringBuilder colNamesBuilder = new StringBuilder(); |
There was a problem hiding this comment.
Thank you, it's cleaner.
|
|
||
| // Try to get get column names and types from table or partition properties. If they are absent there, get columns | ||
| // data from storage descriptor of the table | ||
| if (properties.containsKey(serdeConstants.LIST_COLUMNS) && properties.containsKey(serdeConstants.LIST_COLUMN_TYPES)) { |
There was a problem hiding this comment.
avoid double get() (containsKey()) if possible.
| <exclusion> | ||
| <artifactId>calcite-avatica</artifactId> | ||
| <groupId>org.apache.calcite</groupId> | ||
| <groupId>org.json</groupId> |
There was a problem hiding this comment.
Has new version of hive introduced the dependency on org.json:json?
There was a problem hiding this comment.
The master branch of Hive doesn't have org.json.
But Hive2.1 includes this:
https://github.com/apache/hive/blob/branch-2.1/pom.xml#L608
https://github.com/apache/hive/blob/branch-2.1/pom.xml#L608
and so on...
| <dependency> | ||
| <groupId>org.apache.parquet</groupId> | ||
| <artifactId>parquet-column</artifactId> | ||
| <version>${parquet.version}</version> |
There was a problem hiding this comment.
Any other parquet dependencies? If they are not needed, why the explicit dependency on org.apache.parquet:parquet-column is necessary?
There was a problem hiding this comment.
Hive parquet dependencies cause an issues in Drill. So relocating is decision.
But in this case one Drill test DRILL-3938 is failed, because Parquet 1.8.0 version started to prohibit empty struct/groups on MessageType level PARQUET-278.
But from 1.8.1 version it is allowed again - PARQUET-363.
So Drill parquet-column version without throwing an Exception solves the issue.
For Hive2.3 it can be changed.
There was a problem hiding this comment.
Why is it safe to change one library? Will it be safer to upgrade all hive dependencies on parquet to the same version?
There was a problem hiding this comment.
hive-exec uses only two parquet dependencies: parquet-column and parquet-hadoop-bundle.
But Drill doesn't use own version of parquet-hadoop-bundle and moreover Drill version for it is absent in maven repository.
It appears that Hive 2.3.2 uses parquet-column 1.8.1 version as well. But last Apache Hive master is updated to 1.9.0 version.
I have added comment about it into drill-hive-exec-shaded POM to update it in future.
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-shade-plugin</artifactId> | ||
| <version>2.1</version> | ||
| <version>3.1.0</version> |
There was a problem hiding this comment.
What is the reason for the version change and should it be applied to other modules where shading is used?
There was a problem hiding this comment.
It is a last stable version of this plugin. Update is not important.
We use 2.4 version around the project. But here was an older version.
There was a problem hiding this comment.
Can it be unified in the plugin management of the Drill root pom?
| <groupId>commons-codec</groupId> | ||
| <artifactId>commons-codec</artifactId> | ||
| </exclusion> | ||
| <exclusion> |
| <dependency> | ||
| <groupId>org.apache.calcite</groupId> | ||
| <artifactId>calcite-core</artifactId> | ||
| <version>${calcite.version}</version> |
There was a problem hiding this comment.
hive-exec needs own calcite 1.6 version.
Calcite version in DependencyManagement of root pom leads to using Drill Calcite version over the whole project.
| <dependency> | ||
| <groupId>org.apache.calcite</groupId> | ||
| <artifactId>calcite-core</artifactId> | ||
| <version>${calcite.version}</version> |
| * @param properties table or partition properties | ||
| * @param sd storage descriptor | ||
| */ | ||
| public static void verifyAndAddTransactionalProperties(JobConf job, Properties properties, StorageDescriptor sd) { |
There was a problem hiding this comment.
job involves table properties, but JobConf hasn't any method to return that properties. It is possible to get only one property. But looks like this is not an issue.
There is in AcidUtils the isTablePropertyTransactional method with Configuration input parameter.
Changed. Thank you.
|
|
||
| // Try to get get column names and types from table or partition properties. If they are absent there, get columns | ||
| // data from storage descriptor of the table | ||
| if (properties.containsKey(serdeConstants.LIST_COLUMNS) && properties.containsKey(serdeConstants.LIST_COLUMN_TYPES)) { |
| colNames = job.get(serdeConstants.LIST_COLUMNS); | ||
| colTypes = job.get(serdeConstants.LIST_COLUMN_TYPES); | ||
| } else { | ||
| final StringBuilder colNamesBuilder = new StringBuilder(); |
There was a problem hiding this comment.
Thank you, it's cleaner.
| <exclusion> | ||
| <artifactId>calcite-avatica</artifactId> | ||
| <groupId>org.apache.calcite</groupId> | ||
| <groupId>org.json</groupId> |
There was a problem hiding this comment.
The master branch of Hive doesn't have org.json.
But Hive2.1 includes this:
https://github.com/apache/hive/blob/branch-2.1/pom.xml#L608
https://github.com/apache/hive/blob/branch-2.1/pom.xml#L608
and so on...
| <dependency> | ||
| <groupId>org.apache.parquet</groupId> | ||
| <artifactId>parquet-column</artifactId> | ||
| <version>${parquet.version}</version> |
There was a problem hiding this comment.
Hive parquet dependencies cause an issues in Drill. So relocating is decision.
But in this case one Drill test DRILL-3938 is failed, because Parquet 1.8.0 version started to prohibit empty struct/groups on MessageType level PARQUET-278.
But from 1.8.1 version it is allowed again - PARQUET-363.
So Drill parquet-column version without throwing an Exception solves the issue.
For Hive2.3 it can be changed.
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-shade-plugin</artifactId> | ||
| <version>2.1</version> | ||
| <version>3.1.0</version> |
There was a problem hiding this comment.
It is a last stable version of this plugin. Update is not important.
We use 2.4 version around the project. But here was an older version.
| colNamesList.add(col.getName()); | ||
| colTypesList.add(col.getType()); | ||
| } | ||
| colNames = Joiner.on(",").join(colNamesList); |
There was a problem hiding this comment.
Consider Joiner.on(",").join(Iterables.transform(sd.getCols(), toName)); where toName is
private static Function<FieldSchema, String> toName = new Function<FieldSchema, String>()
{
@Nullable
@Override
public String apply(@Nullable FieldSchema input)
{
return input.getName();
}
};
There was a problem hiding this comment.
I have changed it. But we need call input.getName() and input.getType(), that's why I use two Functions and code became bigger. Once we will use Java8 it can be smaller. Or did I miss something here?
ab7130d to
96cd7a2
Compare
vdiravka
left a comment
There was a problem hiding this comment.
@vrozov I have addressed your comments. Changes are in a new commit. This is a description of the changes:
- Apache Hive version client updating to 2.3.2 version.
- "io.dropwizard.metrics:metrics-core" with last 4.0.2 version is added to dependencyManagement block in Drill root POM
- Exclusion of "hive-exec" in "hive-hbase-handler" is already in Drill root dependencyManagement POM
- Hive Calcite libraries are excluded (Calcite CBO was disabled in firs commit)
- "jackson-core" dependency is added to DependencyManagement block in Drill root POM file.
- For Hive 2.1 client older "com.fasterxml.jackson.core:jackson-databind" is included
- Exclusion of "log4j:log4j" dependency from "hive-exec" is replaced to dependencyManagement block of Drill root POM file.
- Exclusion of "com.codahale.metrics:metrics-core" from "hive-hbase-handler" is removed.
One Advanced test started to mysteriously fail
framework/resources/Advanced/tpch/tpch_sf100/parquet/01.q for some decimal values, not for hive but for parquet. I will investigate the reason.
| <groupId>commons-codec</groupId> | ||
| <artifactId>commons-codec</artifactId> | ||
| </exclusion> | ||
| <exclusion> |
There was a problem hiding this comment.
Both cases can resolve it. In details io.dropwizard.metrics:metrics-core is nowhere used in Drill. And this is a dependency for tephra-core and transitive for hive-metastore. But it conflicts with Drill's com.codahale.metrics.
hive-metastore uses 3.0.1 version of this dependency, but the last version in maven repository is 4.0.2.
I added this dependency to dependencyManagement block with 4.0.2 version and conflict is resolved as well. I think it is a better decision, because it can help to avoid similar conflicts in future.
Also metrics-core in hive-hbase-handler has not influence to Drill, so I've removed my exclusion of it.
| <dependency> | ||
| <groupId>org.apache.calcite</groupId> | ||
| <artifactId>calcite-core</artifactId> | ||
| <version>${calcite.version}</version> |
There was a problem hiding this comment.
I have returned calcite-core version into DependencyManagement block. Drill Calcite version libraries are included into "drill-hive-exec-shaded" module.
I will tell you why it works for now:
When user submits query in Drill via Hive plugin, the query is validated and planned via Drill Calcite. So Hive Calcite isn't necessary for it.
Hive Calcite is used only in the process of Drill unit testing, where a lot of Hive specific queries are performed to setup Hive store for testing. But Drill Calcite and Avatica versions have conflicts with Hive old Calcite and Avatica versions. That's why I have disabled Calcite cost based optimizator conf.set(ConfVars.HIVE_CBO_ENABLED.varname, "false");. We can enable it again once Hive will leverage the newest Calcite version. The comment about it is added into drill-hive-exec-shaded POM.
| colNamesList.add(col.getName()); | ||
| colTypesList.add(col.getType()); | ||
| } | ||
| colNames = Joiner.on(",").join(colNamesList); |
There was a problem hiding this comment.
I have changed it. But we need call input.getName() and input.getType(), that's why I use two Functions and code became bigger. Once we will use Java8 it can be smaller. Or did I miss something here?
| <dependency> | ||
| <groupId>org.apache.parquet</groupId> | ||
| <artifactId>parquet-column</artifactId> | ||
| <version>${parquet.version}</version> |
There was a problem hiding this comment.
hive-exec uses only two parquet dependencies: parquet-column and parquet-hadoop-bundle.
But Drill doesn't use own version of parquet-hadoop-bundle and moreover Drill version for it is absent in maven repository.
It appears that Hive 2.3.2 uses parquet-column 1.8.1 version as well. But last Apache Hive master is updated to 1.9.0 version.
I have added comment about it into drill-hive-exec-shaded POM to update it in future.
|
|
||
| // SerDe of the reading partition (or table if the table is non-partitioned) | ||
| protected SerDe partitionSerDe; | ||
| protected Deserializer partitionSerDe; |
There was a problem hiding this comment.
rename to 'partitionDeserializer'
| HiveUtilities.addConfToJob(job, partitionProperties); | ||
|
|
||
| final SerDe tableSerDe = createSerDe(job, table.getSd().getSerdeInfo().getSerializationLib(), tableProperties); | ||
| final Deserializer tableSerDe = createSerDe(job, table.getSd().getSerdeInfo().getSerializationLib(), tableProperties); |
There was a problem hiding this comment.
rename both function and the variable.
There was a problem hiding this comment.
Done. tableSerDe -> tableDeserializer and createSerDe -> createDeserializer
If you meant getSerdeInfo() as well, that function is a part of hive code.
| </exclusion> | ||
| </exclusions> | ||
| </dependency> | ||
| <!--Once newer hive-exec version leverages parquet-column 1.9.0, this dependency can be deleted --> |
There was a problem hiding this comment.
Can it be moved to the dependency management if this is still necessary?
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-shade-plugin</artifactId> | ||
| <version>2.1</version> | ||
| <version>3.1.0</version> |
There was a problem hiding this comment.
Can it be unified in the plugin management of the Drill root pom?
b798969 to
9ab2b76
Compare
|
|
||
| // SerDe of the reading partition (or table if the table is non-partitioned) | ||
| protected SerDe partitionSerDe; | ||
| protected Deserializer partitionSerDe; |
| HiveUtilities.addConfToJob(job, partitionProperties); | ||
|
|
||
| final SerDe tableSerDe = createSerDe(job, table.getSd().getSerdeInfo().getSerializationLib(), tableProperties); | ||
| final Deserializer tableSerDe = createSerDe(job, table.getSd().getSerdeInfo().getSerializationLib(), tableProperties); |
There was a problem hiding this comment.
Done. tableSerDe -> tableDeserializer and createSerDe -> createDeserializer
If you meant getSerdeInfo() as well, that function is a part of hive code.
| </exclusion> | ||
| </exclusions> | ||
| </dependency> | ||
| <!--Once newer hive-exec version leverages parquet-column 1.9.0, this dependency can be deleted --> |
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-shade-plugin</artifactId> | ||
| <version>2.1</version> | ||
| <version>3.1.0</version> |
53e75ce to
8834db7
Compare
|
@vrozov Thank you for CR. |
…2.1.2-mapr-1710 versions respectively * Improvements to allow of reading Hive bucketed transactional ORC tables; * Updating hive properties for tests and resolving dependencies and API conflicts: - Fix for "hive.metastore.schema.verification", MetaException(message: Version information not found in metastore) https://cwiki.apache.org/confluence/display/Hive/Hive+Schema+Tool METASTORE_SCHEMA_VERIFICATION="false" property is added - Added METASTORE_AUTO_CREATE_ALL="true", properties to tests, because some additional tables are necessary in Hive metastore - Disabling calcite CBO for (Hive's CalcitePlanner) for tests, because it is in conflict with Drill's Calcite version for Drill unit tests. HIVE_CBO_ENABLED="false" property - jackson and parquet libraries are relocated in hive-exec-shade module - org.apache.parquet:parquet-column Drill version is added to "hive-exec" to allow of using Parquet empty group on MessageType level (PARQUET-278) - Removing of commons-codec exclusion from hive core. This dependency is necessary for hive-exec and hive-metastore. - Setting Hive internal properties for transactional scan: HiveConf.HIVE_TRANSACTIONAL_TABLE_SCAN and for schema evolution: HiveConf.HIVE_SCHEMA_EVOLUTION, IOConstants.SCHEMA_EVOLUTION_COLUMNS, IOConstants.SCHEMA_EVOLUTION_COLUMNS_TYPES - "io.dropwizard.metrics:metrics-core" with last 4.0.2 version is added to dependencyManagement block in Drill root POM - Exclusion of "hive-exec" in "hive-hbase-handler" is already in Drill root dependencyManagement POM - Hive Calcite libraries are excluded (Calcite CBO was disabled) - "jackson-core" dependency is added to DependencyManagement block in Drill root POM file - For MapR Hive 2.1 client older "com.fasterxml.jackson.core:jackson-databind" is included - "log4j:log4j" dependency is excluded from "hive-exec", "hive-metastore", "hive-hbase-handler".
|
+1. merged the PR in commit 27aa236. |
Updating hive properties for tests and resolving dependencies and API conflicts:
Calcite version is removed from root POM Dependency Management
Version information not found in metastore)
https://cwiki.apache.org/confluence/display/Hive/Hive+Schema+Tool
METASTORE_SCHEMA_VERIFICATION="false" property is added
tables are necessary in Hive metastore
with Drill's Calcite version for Drill unit tests. HIVE_CBO_ENABLED="false" property
allow of using Parquet empty group on MessageType level (PARQUET-278)
necessary for hive-exec and hive-metastore.
HiveConf.HIVE_TRANSACTIONAL_TABLE_SCAN and for schema evolution: HiveConf.HIVE_SCHEMA_EVOLUTION,
IOConstants.SCHEMA_EVOLUTION_COLUMNS, IOConstants.SCHEMA_EVOLUTION_COLUMNS_TYPES