You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/10/19 14:58:29 UTC
[GitHub] [iceberg] marton-bod opened a new pull request #1632: Select correct ObjectInspector based on Hive version on classpath
marton-bod opened a new pull request #1632:
URL: https://github.com/apache/iceberg/pull/1632
To fix https://github.com/apache/iceberg/issues/1630, the ObjectInspector selection logic needs to depend on checking the presence of Hive3 classes on the classpath.
cc @rdblue @massdosage @pvary - thanks for checking!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1632: Select correct ObjectInspector based on Hive version on classpath
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1632:
URL: https://github.com/apache/iceberg/pull/1632#issuecomment-712395701
Thanks for the fix!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] marton-bod commented on a change in pull request #1632: Select correct ObjectInspector based on Hive version on classpath
Posted by GitBox <gi...@apache.org>.
marton-bod commented on a change in pull request #1632:
URL: https://github.com/apache/iceberg/pull/1632#discussion_r507919599
##########
File path: mr/src/main/java/org/apache/iceberg/mr/hive/serde/objectinspector/IcebergObjectInspector.java
##########
@@ -36,23 +37,29 @@
// get the correct inspectors depending on whether we're working with Hive2 or Hive3 dependencies
// we need to do this because there is a breaking API change in Date/TimestampObjectInspector between Hive2 and Hive3
- private static final ObjectInspector DATE_INSPECTOR = DynMethods.builder("get")
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergDateObjectInspectorHive3")
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergDateObjectInspector")
- .buildStatic()
- .invoke();
-
- private static final ObjectInspector TIMESTAMP_INSPECTOR = DynMethods.builder("get")
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspectorHive3", boolean.class)
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspector", boolean.class)
- .buildStatic()
- .invoke(false);
-
- private static final ObjectInspector TIMESTAMP_INSPECTOR_WITH_TZ = DynMethods.builder("get")
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspectorHive3", boolean.class)
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspector", boolean.class)
- .buildStatic()
- .invoke(true);
+ private static final ObjectInspector DATE_INSPECTOR = getObjectInspectorBasedOnHiveVersion("get",
+ "org.apache.iceberg.mr.hive.serde.objectinspector.IcebergDateObjectInspector",
+ "org.apache.iceberg.mr.hive.serde.objectinspector.IcebergDateObjectInspectorHive3",
+ new Class[] {}, new Object[] {});
+
+ private static final ObjectInspector TIMESTAMP_INSPECTOR = getObjectInspectorBasedOnHiveVersion("get",
+ "org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspector",
+ "org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspectorHive3",
+ new Class[] { boolean.class }, new Object[] { false });
+
+ private static final ObjectInspector TIMESTAMP_INSPECTOR_WITH_TZ = getObjectInspectorBasedOnHiveVersion("get",
+ "org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspector",
+ "org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspectorHive3",
+ new Class[] { boolean.class }, new Object[] { true });
+
+ private static ObjectInspector getObjectInspectorBasedOnHiveVersion(
+ String staticMethod, String hive2Class, String hive3Class, Class[] argTypes, Object[] args) {
+ String classToUse = MetastoreUtil.hive3PresentOnClasspath() ? hive3Class : hive2Class;
+ return DynMethods.builder(staticMethod)
+ .impl(classToUse, argTypes)
+ .buildStatic()
+ .invoke(args);
Review comment:
yep...much simpler, thanks:)
changed it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #1632: Select correct ObjectInspector based on Hive version on classpath
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #1632:
URL: https://github.com/apache/iceberg/pull/1632
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1632: Select correct ObjectInspector based on Hive version on classpath
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1632:
URL: https://github.com/apache/iceberg/pull/1632#discussion_r507895309
##########
File path: mr/src/main/java/org/apache/iceberg/mr/hive/serde/objectinspector/IcebergObjectInspector.java
##########
@@ -36,23 +37,29 @@
// get the correct inspectors depending on whether we're working with Hive2 or Hive3 dependencies
// we need to do this because there is a breaking API change in Date/TimestampObjectInspector between Hive2 and Hive3
- private static final ObjectInspector DATE_INSPECTOR = DynMethods.builder("get")
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergDateObjectInspectorHive3")
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergDateObjectInspector")
- .buildStatic()
- .invoke();
-
- private static final ObjectInspector TIMESTAMP_INSPECTOR = DynMethods.builder("get")
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspectorHive3", boolean.class)
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspector", boolean.class)
- .buildStatic()
- .invoke(false);
-
- private static final ObjectInspector TIMESTAMP_INSPECTOR_WITH_TZ = DynMethods.builder("get")
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspectorHive3", boolean.class)
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspector", boolean.class)
- .buildStatic()
- .invoke(true);
+ private static final ObjectInspector DATE_INSPECTOR = getObjectInspectorBasedOnHiveVersion("get",
+ "org.apache.iceberg.mr.hive.serde.objectinspector.IcebergDateObjectInspector",
+ "org.apache.iceberg.mr.hive.serde.objectinspector.IcebergDateObjectInspectorHive3",
+ new Class[] {}, new Object[] {});
+
+ private static final ObjectInspector TIMESTAMP_INSPECTOR = getObjectInspectorBasedOnHiveVersion("get",
+ "org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspector",
+ "org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspectorHive3",
+ new Class[] { boolean.class }, new Object[] { false });
+
+ private static final ObjectInspector TIMESTAMP_INSPECTOR_WITH_TZ = getObjectInspectorBasedOnHiveVersion("get",
+ "org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspector",
+ "org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspectorHive3",
+ new Class[] { boolean.class }, new Object[] { true });
+
+ private static ObjectInspector getObjectInspectorBasedOnHiveVersion(
+ String staticMethod, String hive2Class, String hive3Class, Class[] argTypes, Object[] args) {
+ String classToUse = MetastoreUtil.hive3PresentOnClasspath() ? hive3Class : hive2Class;
+ return DynMethods.builder(staticMethod)
+ .impl(classToUse, argTypes)
+ .buildStatic()
+ .invoke(args);
Review comment:
It seems like it would be a bit easier to read if you didn't wrap the `DynMethods` call and instead just swapped out the class:
```java
private static final TIMESTAMP_INSPECTOR_CLASS = MetastoreUtil.hive3PresentOnClasspath() ?
"org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspector" :
"org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspectorHive3";
private static final ObjectInspector TIMESTAMP_INSPECTOR = DynMethods.builder("get")
.impl(TIMESTAMP_INSPECTOR_CLASS, boolean.class)
.buildStatic()
.invoke(false);
```
That way you don't need all of the inline array literals.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1632: Select correct ObjectInspector based on Hive version on classpath
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1632:
URL: https://github.com/apache/iceberg/pull/1632#discussion_r507893486
##########
File path: mr/src/main/java/org/apache/iceberg/mr/hive/serde/objectinspector/IcebergObjectInspector.java
##########
@@ -36,23 +37,29 @@
// get the correct inspectors depending on whether we're working with Hive2 or Hive3 dependencies
// we need to do this because there is a breaking API change in Date/TimestampObjectInspector between Hive2 and Hive3
- private static final ObjectInspector DATE_INSPECTOR = DynMethods.builder("get")
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergDateObjectInspectorHive3")
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergDateObjectInspector")
- .buildStatic()
- .invoke();
-
- private static final ObjectInspector TIMESTAMP_INSPECTOR = DynMethods.builder("get")
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspectorHive3", boolean.class)
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspector", boolean.class)
- .buildStatic()
- .invoke(false);
-
- private static final ObjectInspector TIMESTAMP_INSPECTOR_WITH_TZ = DynMethods.builder("get")
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspectorHive3", boolean.class)
- .impl("org.apache.iceberg.mr.hive.serde.objectinspector.IcebergTimestampObjectInspector", boolean.class)
- .buildStatic()
- .invoke(true);
+ private static final ObjectInspector DATE_INSPECTOR = getObjectInspectorBasedOnHiveVersion("get",
+ "org.apache.iceberg.mr.hive.serde.objectinspector.IcebergDateObjectInspector",
Review comment:
Nit: indentation is off now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org