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/15 17:08:03 UTC
[GitHub] [iceberg] RussellSpitzer opened a new pull request #1616: Seperate Action Implementations into Spark Specific Modules - With Reflection
RussellSpitzer opened a new pull request #1616:
URL: https://github.com/apache/iceberg/pull/1616
Uses reflection in the base Actions class to determine the actual
implementation on the classloader to use. This allows both Spark2
and Spark3 to use different implementations of Actions while not
changing the location of the original Actions class.
----------------------------------------------------------------
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 #1616: Seperate Action Implementations into Spark Specific Modules - With Reflection
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1616:
URL: https://github.com/apache/iceberg/pull/1616#discussion_r505954090
##########
File path: spark/src/main/java/org/apache/iceberg/actions/Actions.java
##########
@@ -19,25 +19,57 @@
package org.apache.iceberg.actions;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
import org.apache.iceberg.Table;
import org.apache.spark.sql.SparkSession;
public class Actions {
+ private static final String IMPL_NAME = "SparkActions";
+
+ private static Class<Actions> implClass;
+ private static Constructor<Actions> implConstructor;
private SparkSession spark;
private Table table;
- private Actions(SparkSession spark, Table table) {
+ protected Actions(SparkSession spark, Table table) {
this.spark = spark;
this.table = table;
}
+ private static Constructor actionConstructor() {
Review comment:
Using our reflection helpers, this could be simpler:
```java
try {
DynConstructors.Ctor<Actions> ctor = DynConstructors.builder()
.impl(SparkActions.class, SparkSession.class, Table.class)
.build();
return ctor;
} catch (NoSuchMethodException e) {
throw new UnsupportedOperationException("Cannot find Actions implementation on the classpath");
}
```
----------------------------------------------------------------
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] RussellSpitzer commented on pull request #1616: Seperate Action Implementations into Spark Specific Modules - With Reflection
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on pull request #1616:
URL: https://github.com/apache/iceberg/pull/1616#issuecomment-710292514
@rdblue All cleaned up with the Dyn utility class. If this is the route you want to go let's merge it and I'll rebase the Create PR
----------------------------------------------------------------
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] aokolnychyi commented on pull request #1616: Seperate Action Implementations into Spark Specific Modules - With Reflection
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #1616:
URL: https://github.com/apache/iceberg/pull/1616#issuecomment-710557961
I am not a big fun of reflection but this does make the change smaller so seems reasonable.
----------------------------------------------------------------
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 #1616: Seperate Action Implementations into Spark Specific Modules - With Reflection
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1616:
URL: https://github.com/apache/iceberg/pull/1616#issuecomment-709671206
I think I prefer this way because it's so much smaller. I don't see much benefit to avoiding reflection, and this ensures that the class that users interact with is common between the two implementations. If we go with the other approach, then a user can compile against the Spark 3 `Actions` methods and have missing methods at runtime, which would be more confusing than failing with `UnsupportedOperationException` thrown by the superclass.
----------------------------------------------------------------
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] aokolnychyi merged pull request #1616: Seperate Action Implementations into Spark Specific Modules - With Reflection
Posted by GitBox <gi...@apache.org>.
aokolnychyi merged pull request #1616:
URL: https://github.com/apache/iceberg/pull/1616
----------------------------------------------------------------
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] RussellSpitzer commented on pull request #1616: Seperate Action Implementations into Spark Specific Modules - With Reflection
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on pull request #1616:
URL: https://github.com/apache/iceberg/pull/1616#issuecomment-709688422
I'm not sure it's likely that a user can compile against the Spark 3 Module and not have many many other things go wrong at runtime :)
----------------------------------------------------------------
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] RussellSpitzer commented on a change in pull request #1616: Seperate Action Implementations into Spark Specific Modules - With Reflection
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #1616:
URL: https://github.com/apache/iceberg/pull/1616#discussion_r507836371
##########
File path: spark2/src/main/java/org/apache/iceberg/actions/SparkActions.java
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.actions;
+
+import org.apache.iceberg.Table;
+import org.apache.spark.sql.SparkSession;
+
+public class SparkActions extends Actions {
Review comment:
I guess not, I'll close them
----------------------------------------------------------------
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] RussellSpitzer edited a comment on pull request #1616: Seperate Action Implementations into Spark Specific Modules - With Reflection
Posted by GitBox <gi...@apache.org>.
RussellSpitzer edited a comment on pull request #1616:
URL: https://github.com/apache/iceberg/pull/1616#issuecomment-709688422
I'm not sure it's likely that a user can compile against the Spark 3 Module and not have many many other things go wrong at runtime if they are actually on Spark 2 :)
----------------------------------------------------------------
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 #1616: Seperate Action Implementations into Spark Specific Modules - With Reflection
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1616:
URL: https://github.com/apache/iceberg/pull/1616#discussion_r505954090
##########
File path: spark/src/main/java/org/apache/iceberg/actions/Actions.java
##########
@@ -19,25 +19,57 @@
package org.apache.iceberg.actions;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
import org.apache.iceberg.Table;
import org.apache.spark.sql.SparkSession;
public class Actions {
+ private static final String IMPL_NAME = "SparkActions";
+
+ private static Class<Actions> implClass;
+ private static Constructor<Actions> implConstructor;
private SparkSession spark;
private Table table;
- private Actions(SparkSession spark, Table table) {
+ protected Actions(SparkSession spark, Table table) {
this.spark = spark;
this.table = table;
}
+ private static Constructor actionConstructor() {
Review comment:
Using our reflection helpers, this could be simpler:
```java
try {
DynConstructors.Ctor<Actions> ctor = DynConstructors.builder()
.impl(SparkActions.class, SparkSession.class, Table.class)
.build();
ctor.newInstance(spark, table);
} catch (NoSuchMethodException e) {
throw new UnsupportedOperationException("Cannot find Actions implementation on the classpath");
}
```
----------------------------------------------------------------
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] RussellSpitzer commented on a change in pull request #1616: Seperate Action Implementations into Spark Specific Modules - With Reflection
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #1616:
URL: https://github.com/apache/iceberg/pull/1616#discussion_r506525777
##########
File path: spark/src/main/java/org/apache/iceberg/actions/Actions.java
##########
@@ -19,25 +19,57 @@
package org.apache.iceberg.actions;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
import org.apache.iceberg.Table;
import org.apache.spark.sql.SparkSession;
public class Actions {
+ private static final String IMPL_NAME = "SparkActions";
+
+ private static Class<Actions> implClass;
+ private static Constructor<Actions> implConstructor;
private SparkSession spark;
private Table table;
- private Actions(SparkSession spark, Table table) {
+ protected Actions(SparkSession spark, Table table) {
this.spark = spark;
this.table = table;
}
+ private static Constructor actionConstructor() {
Review comment:
we need build checked :| which has this code which kinda makes me laugh a little
```java
@SuppressWarnings("unchecked")
public <C> Ctor<C> buildChecked() throws NoSuchMethodException {
```
----------------------------------------------------------------
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] RussellSpitzer commented on a change in pull request #1616: Seperate Action Implementations into Spark Specific Modules - With Reflection
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #1616:
URL: https://github.com/apache/iceberg/pull/1616#discussion_r506523695
##########
File path: spark/src/main/java/org/apache/iceberg/actions/Actions.java
##########
@@ -19,25 +19,57 @@
package org.apache.iceberg.actions;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
import org.apache.iceberg.Table;
import org.apache.spark.sql.SparkSession;
public class Actions {
+ private static final String IMPL_NAME = "SparkActions";
+
+ private static Class<Actions> implClass;
+ private static Constructor<Actions> implConstructor;
private SparkSession spark;
private Table table;
- private Actions(SparkSession spark, Table table) {
+ protected Actions(SparkSession spark, Table table) {
this.spark = spark;
this.table = table;
}
+ private static Constructor actionConstructor() {
Review comment:
Unfortunately DynConstructor wraps up exceptions (no explicit throws) a bit differently so we can't just catch a NSME, let me see if I can do something similar.
----------------------------------------------------------------
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] aokolnychyi commented on a change in pull request #1616: Seperate Action Implementations into Spark Specific Modules - With Reflection
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1616:
URL: https://github.com/apache/iceberg/pull/1616#discussion_r506702177
##########
File path: spark2/src/main/java/org/apache/iceberg/actions/SparkActions.java
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.actions;
+
+import org.apache.iceberg.Table;
+import org.apache.spark.sql.SparkSession;
+
+public class SparkActions extends Actions {
Review comment:
Do we need to make these public?
----------------------------------------------------------------
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 #1616: Seperate Action Implementations into Spark Specific Modules - With Reflection
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1616:
URL: https://github.com/apache/iceberg/pull/1616#discussion_r506621068
##########
File path: spark/src/main/java/org/apache/iceberg/actions/Actions.java
##########
@@ -19,25 +19,57 @@
package org.apache.iceberg.actions;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
import org.apache.iceberg.Table;
import org.apache.spark.sql.SparkSession;
public class Actions {
+ private static final String IMPL_NAME = "SparkActions";
+
+ private static Class<Actions> implClass;
+ private static Constructor<Actions> implConstructor;
private SparkSession spark;
private Table table;
- private Actions(SparkSession spark, Table table) {
+ protected Actions(SparkSession spark, Table table) {
this.spark = spark;
this.table = table;
}
+ private static Constructor actionConstructor() {
Review comment:
Yeah, the API allows you to catch checked exceptions if you want to handle them, or the normal path will wrap them with error messages and unchecked exceptions.
----------------------------------------------------------------
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