Make defaults for optional SchemaTransformProvider methods #30560
Conversation
|
@ahmedabu98 I assigned myself as reviewer. I'll review after checks run. |
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
Failing test is a flake in SpannerChangeStreamErrorTest. @damondouglas it's ready for a review |
|
R: @robertwb |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
| @@ -58,10 +59,14 @@ default String description() { | |||
| SchemaTransform from(Row configuration); | |||
|
|
|||
| /** Returns the input collection names of this transform. */ | |||
| List<String> inputCollectionNames(); | |||
| default List<String> inputCollectionNames() { | |||
There was a problem hiding this comment.
intellij thinks these methods are unused - is intellij wrong? or could they be removed?
There was a problem hiding this comment.
In reality, developers make their providers by extending TypedSchemaTransformProvider, which implements SchemaTransformProvider. I guess intellij might grey it out because they're not directly used from SchemaTransformProvider
There was a problem hiding this comment.
So if they override these methods but we never call them, does it matter?
There was a problem hiding this comment.
Ahh these methods are eventually called by the expansion service to create the discover response. Reference here:
| protected abstract Class<ConfigT> configurationClass(); | ||
| @SuppressWarnings("unchecked") | ||
| protected Class<ConfigT> configurationClass() { | ||
| Optional<ParameterizedType> parameterizedType = |
There was a problem hiding this comment.
Since you just put it in an Optional and then take it out again, might be simpler to go ahead like
@Nullable ParameterizedType parameterizedType = (ParameterizedType) getClass().getGenericSuperclass();
checkStateNotNull(superClass, "Could not ...");
return (Class<ConfigT>) parameterizedType.getActualTypeArguments[0];FWIW I am not sure if getActualTypeArguments[0] could still be a type variable in some cases. You might want to check that it is a usefully defined type in some more ways... but also this is probably just always going to work, because of how these things are authored. Very nice!
There was a problem hiding this comment.
Thanks, applied the suggestion.
| @@ -58,10 +59,14 @@ default String description() { | |||
| SchemaTransform from(Row configuration); | |||
|
|
|||
| /** Returns the input collection names of this transform. */ | |||
| List<String> inputCollectionNames(); | |||
| default List<String> inputCollectionNames() { | |||
There was a problem hiding this comment.
So if they override these methods but we never call them, does it matter?
| @@ -61,7 +61,7 @@ | |||
| import org.junit.runners.JUnit4; | |||
|
|
|||
| @RunWith(JUnit4.class) | |||
| public class BigQueryStorageWriteApiSchemaTransformProviderTest { | |||
There was a problem hiding this comment.
This breaks DataflowV1 and V2 tests. Any reason moving them from unit test to integration test?
There was a problem hiding this comment.
CC: @ahmedabu98 @kennknowles @damondouglas
They use fake BigQuery service so has to be executed locally. Either exclude them from Dataflow test suites or move back to unit test?
There was a problem hiding this comment.
Ahh no reason, bad mistake. I'll open a PR to revert this part
There was a problem hiding this comment.
Thanks for catching this, pls take a look at #30623
Making some changes to reduce the necessary boilerplate when creating schema-aware transforms.
Some methods (
inputCollectionNames,outputCollectionNames) are not required to implement to use as a cross-language transform. These methods just provide information that may be helpful (e.g. to a remote SDK), but don't have any real functional use.Another method for TypedSchemaTransformProvider (
configurationClass) can have a default implementation via reflection.Of course, implementations can continue implementing these methods as they see fit, but it probably shouldn't be required to do so.