make FieldValueTypeInformation creators take a TypeDescriptor parameter #32081

tilgalas · 2024-08-05T15:18:27Z

This is a 2nd PR in the series of PRs (based on the now closed #31648, with the first PR being #31785) whose ultimate goal is to add support for generic classes to schema providers.

FieldValueTypeInformation creators will now accept a TypeDescriptor parameter describing the field's containing class, that will let them infer more accurate type information about that field. For example - consider a MyClass<T> class - now with a TypeDescriptor of MyClass<String> and the getter of type T the type resolver can infer the type of field to be String , see the added test class that shows the new functionality

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

…parameter

github-actions · 2024-08-05T16:37:16Z

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

tilgalas · 2024-08-05T17:15:34Z

Run Java PreCommit

tilgalas · 2024-08-06T11:37:24Z

Run Java_GCP_IO_Direct PreCommit

tilgalas · 2024-08-06T12:38:45Z

Run Java PreCommit

tilgalas · 2024-08-06T14:30:01Z

Run Java_Hadoop_IO_Direct PreCommit

tilgalas · 2024-08-06T16:14:33Z

assign set of reviewers

github-actions · 2024-08-06T16:15:46Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @m-trieu for label java.
R: @damondouglas for label io.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

damccorm · 2024-08-14T09:36:08Z

@damondouglas could you please take a look at this one?

damondouglas

@tilgalas Thank you for working on this! Would you mind to first remove the "nullness" value in the @SuppressWarnings annotation for the involved classes in this PR? Please consider this suggestion as non-blocking but I would like to see if this is possible with your helpful changes. As a second phase, may we also consider removing the "rawtypes"? Again, non-blocking but it's something we would like to strive for to improve the code quality.

tilgalas · 2024-08-19T10:37:23Z

@tilgalas Thank you for working on this! Would you mind to first remove the "nullness" value in the @SuppressWarnings annotation for the involved classes in this PR? Please consider this suggestion as non-blocking but I would like to see if this is possible with your helpful changes. As a second phase, may we also consider removing the "rawtypes"? Again, non-blocking but it's something we would like to strive for to improve the code quality.

sure, with pleasure!

github-actions · 2024-08-26T12:14:17Z

Reminder, please take a look at this pr: @m-trieu @damondouglas

github-actions · 2024-08-28T12:14:29Z

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @Abacn for label java.
R: @Abacn for label io.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

github-actions · 2024-09-05T12:14:04Z

Reminder, please take a look at this pr: @Abacn @Abacn

Abacn · 2024-09-05T15:40:45Z

waiting on author

github-actions · 2024-10-08T12:14:12Z

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @Abacn for label java.
R: @chamikaramj for label io.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

tilgalas · 2024-10-14T10:37:55Z

hi reviewers, I'm going to split this PR and put the nullness and rawness refactoring commit into its own PR

tilgalas · 2024-10-16T12:04:34Z

since there's a #32757, I'm closing this PR

tilgalas · 2024-10-22T14:39:30Z

reopening after discussing offline

reuvenlax · 2024-10-22T20:57:07Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/CachingFactory.java

@@ -39,9 +41,9 @@
 public class CachingFactory<CreatedT> implements Factory<CreatedT> {
  private transient @Nullable ConcurrentHashMap<TypeDescriptor<?>, CreatedT> cache = null;

-  private final Factory<CreatedT> innerFactory;
+  private final @NotOnlyInitialized Factory<CreatedT> innerFactory;


what are these new tags for?

so in RowValueGettersFactory there's this code:

RowValueGettersFactory(Factory<List<FieldValueGetter<T, Object>>> gettersFactory) { this.gettersFactory = gettersFactory; this.cachingGettersFactory = new CachingFactory<>(this); }

since we're in the constructor, the this parameter is still not fully initialized, so by using this annotation we can temporarily (for the duration of the initalization of the CachingFactory object) allow assigning the innerFactory field a value which itself is under initialization (or indeed whose initialization status is unknown, as the signature of the CachingFactory constructor tells us)

see also https://checkerframework.org/manual/#circular-initialization

reuvenlax · 2024-10-22T20:57:30Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/FieldValueGetter.java

@@ -29,7 +30,7 @@
 * <p>Implementations of this interface are generated at runtime to map object fields to Row fields.
 */
 @Internal
-public interface FieldValueGetter<ObjectT, ValueT> extends Serializable {
+public interface FieldValueGetter<ObjectT extends @NonNull Object, ValueT> extends Serializable {


why do you have ObjectT extends Object?

so apparently there's a difference (I don't understand why it would be so, though) between a @NonNull T and T extends @NonNull Object - here's an example, consider:

public interface I<T extends @NonNull Object> { T getT(); }

with a signature as above, we can only write:

public static <T extends @NonNull Object> void f(I<T> i) { System.out.println(i.getT().getClass()); }

or alternatively:

public static <T> void f(I<@NonNull T> i) { System.out.println(i.getT().getClass()); }

The checkerframework will make sure that we propagate the non-nullness of the parameter.
This won't work for example:

public static <@Nullable T> void f(I<T> i) { T t = i.getT(); if (t != null) { System.out.println(t.getClass()); } }

nor will this:

public static <T> void f(I<@Nullable T> i) { T t = i.getT(); if (t != null) { System.out.println(t.getClass()); } }

but it will work if I change the signature of I to:

public interface I<@NonNull T> { T getT(); }

in other words, the @NonNull T annotation on the interface is not really enforced anywhere, it's more like a suggestion, and so it's not really expressing the concept of the interface only works with @NonNull types

Generally in Beam I've mostly seen us using @nullable, not @nonnull. I believe that NonNull is assumed unless @nullable is specified. Are you not finding that to be the case?

yeah, at least not when we're talking type parameters, for example:

public interface I<T> { T getT(); } public static <T> void f(I<T> i) { System.out.println(i.getT().getClass()); }

gives me

error: [dereference.of.nullable] dereference of possibly-null reference i.getT() System.out.println(i.getT().getClass()); ^

reuvenlax · 2024-10-22T20:58:39Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/FieldValueTypeInformation.java

@@ -126,17 +123,26 @@ public static FieldValueTypeInformation forOneOf(
  }

  public static FieldValueTypeInformation forField(Field field, int index) {
-    TypeDescriptor<?> type = TypeDescriptor.of(field.getGenericType());
+    return forField(null, field, index);
+  }


This class wasn't really meant to be public. I would add an @internal tag to the class, and not bother keeping this backwards-compatible method

reuvenlax · 2024-10-22T21:04:11Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/GetterBasedSchemaProvider.java

    }

    @Override
    public Row apply(T input) {
-      return Row.withSchema(schema).withFieldValueGetters(getterFactory, input);
+      return Row.withSchema(schema).withFieldValueGetters((Factory) getterFactory, input);


why do you need this cast?

yeah, good question - I'm playing with that code right now and I think I could get rid of it actually

reuvenlax · 2024-10-22T23:39:05Z

sdks/java/core/src/main/java/org/apache/beam/sdk/values/Row.java

@@ -836,10 +838,10 @@ public int nextFieldId() {
    }

    @Internal
-    public Row withFieldValueGetters(
-        Factory<List<FieldValueGetter>> fieldValueGetterFactory, Object getterTarget) {
+    public <T extends @NonNull Object> Row withFieldValueGetters(


unclear on this bound as well

I could change it to <@NonNull T> I suppose - it compiles, but then my IDE complains that "Non-null type argument is expected" on the T parameter of the FieldValueGetter<T, ?> type

I think I prefer @nonnull T (is this simply a weakness in the IDE?), but I guess I don't have strong feelings here.

reuvenlax · 2024-10-22T23:39:43Z

sdks/java/core/src/main/java/org/apache/beam/sdk/values/RowWithGetters.java

    super(schema);
    this.getterTarget = getterTarget;
    this.getters = getterFactory.create(TypeDescriptor.of(getterTarget.getClass()), schema);
  }

  @Override
  @SuppressWarnings({"TypeParameterUnusedInFormals", "unchecked"})
-  public <T> @Nullable T getValue(int fieldIdx) {
+  public @Nullable Object getValue(int fieldIdx) {


why remove type parameter?

I guess it's probably a matter of taste - my view is that this class can't possibly know what type of value a particular getter is going to return, so it's not commiting to anything by making the method generic - "I only know it's an Object, if you need a specific type, you'll have to cast it yourself" - I should probably also change the type of the getters list to List<FieldValueGetter<T, Object>> but then it would probably open another can of worms of covariant types, that is whether a, say, FieldValueGetter<T, String> is a FieldValueGetter<T, Object> - the current type of List<FieldValueGetter<T, ?>> is slightly incorrect as the ? wildcard means an unknown type, but which is at least shared by all members of the list (which is not true)

I can understand the approach where the caller requests a specific type via a generic type parameter and is happy to accept the method throwing a ClassCastException if they had wrong assumptions about the getter - let me know if you want me to make it back a generic method

I feel like I will change the type of the list elements to FieldValueGetter<T, Object> anyway, as I find it more correct

The base class uses a parameter here, so I think for now we should just match the base class.

reuvenlax · 2024-10-22T23:43:11Z

...tensions/avro/src/main/java/org/apache/beam/sdk/extensions/avro/schemas/utils/AvroUtils.java

+      result = false)
+  private static boolean isNullOrEmpty(@Nullable String str) {
+    return str == null || str.isEmpty();
+  }


what is wrong with Strings.isNullOrEmpty?

for some reason the checkerframework doesn't understand the nullness assertion when using it:

/usr/local/google/home/mszwaja/src/beam/sdks/java/extensions/avro/src/main/java/org/apache/beam/sdk/extensions/avro/schemas/utils/AvroUtils.java:538: error: [argument] incompatible argument for parameter namespace of AvroUtils.toAvroField. org.apache.avro.Schema.Field recordField = toAvroField(field, childNamespace); ^ found : @Initialized @Nullable String required: @Initialized @NonNull String /usr/local/google/home/mszwaja/src/beam/sdks/java/extensions/avro/src/main/java/org/apache/beam/sdk/extensions/avro/schemas/utils/AvroUtils.java:541: error: [argument] incompatible argument for parameter name of Schema.createRecord. return org.apache.avro.Schema.createRecord(schemaName, "", schemaNamespace, false, fields); ^ found : @Initialized @Nullable String required: @Initialized @NonNull String

I don't understand what this example has to do with isNullOrEmpty (which returns a boolean)?

this is the error that appears when I use Strings.isNullOrEmpty

in other words, checkerframework doesn't understand that by using this method (and getting a false returned from it) we've just asserted, that the variable is @NonNull - my "polyfill" method uses the checkerframework's annotation:

@EnsuresNonNullIf( expression = {"#1"}, result = false)

which tells it that if it returns false the first argument is verified to be @NonNull - I don't know why checkerframwork doesn't understand guava method's postconditions here (maybe it's fixed in some newer version?)

ahmedabu98 · 2024-10-25T20:28:24Z

Fixes #32081

reuvenlax · 2024-10-28T17:08:25Z

A bunch of tests are failing. PTAL to see if this is due to your PR or just flaky tests.

tilgalas · 2024-10-29T15:54:29Z

Run Java PreCommit

tilgalas · 2024-10-30T12:53:07Z

Run Java PreCommit

tilgalas · 2024-10-30T12:54:43Z

Run Java_GCP_IO_Direct PreCommit

reuvenlax · 2024-10-30T17:05:24Z

lgtm

damccorm · 2024-11-13T19:18:04Z

...tensions/avro/src/main/java/org/apache/beam/sdk/extensions/avro/schemas/utils/AvroUtils.java

@@ -451,8 +484,7 @@ public static Field toBeamField(org.apache.avro.Schema.Field field) {
  public static org.apache.avro.Schema.Field toAvroField(Field field, String namespace) {
    org.apache.avro.Schema fieldSchema =
        getFieldSchema(field.getType(), field.getName(), namespace);
-    return new org.apache.avro.Schema.Field(
-        field.getName(), fieldSchema, field.getDescription(), (Object) null);
+    return new org.apache.avro.Schema.Field(field.getName(), fieldSchema, field.getDescription());


This seems to break https://github.com/apache/beam/actions/workflows/beam_PostCommit_Java_Avro_Versions.yml

I think it is because in some older versions of avro, the constructor is different - https://avro.apache.org/docs/1.8.2/api/java/org/apache/avro/Schema.Field.html

I think #33102 should fix this (I cherry-picked it to the current release branch as well)

Abacn · 2024-11-15T17:55:16Z

This likely breaks DataflowTemplate unit test: https://github.com/GoogleCloudPlatform/DataflowTemplates/pull/2014/checks?check_run_id=33057418339

I'm trying to understand the scope of breaking change

The stacktrace

Caused by: java.lang.IllegalStateException: getters require withGetterTarget.
	at org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions.checkState(Preconditions.java:512)
	at org.apache.beam.sdk.values.Row$Builder.withFieldValueGetters(Row.java:843)
	at org.apache.beam.sdk.schemas.GetterBasedSchemaProvider$ToRowWithValueGetters.apply(GetterBasedSchemaProvider.java:135)
	at org.apache.beam.sdk.schemas.GetterBasedSchemaProvider$ToRowWithValueGetters.apply(GetterBasedSchemaProvider.java:114)
	at org.apache.beam.sdk.schemas.SchemaCoder.encode(SchemaCoder.java:121)
	at org.apache.beam.sdk.coders.MapCoder.encode(MapCoder.java:92)
	at org.apache.beam.sdk.coders.MapCoder.encode(MapCoder.java:41)
	at org.apache.beam.sdk.coders.MapCoder.encode(MapCoder.java:93)
	at org.apache.beam.sdk.coders.MapCoder.encode(MapCoder.java:41)
	at org.apache.beam.sdk.coders.KvCoder.encode(KvCoder.java:73)
	at org.apache.beam.sdk.coders.KvCoder.encode(KvCoder.java:37)
	at org.apache.beam.sdk.util.CoderUtils.encodeToSafeStream(CoderUtils.java:86)
	at org.apache.beam.sdk.util.CoderUtils.encodeToByteArray(CoderUtils.java:70)
	at org.apache.beam.sdk.util.CoderUtils.encodeToByteArray(CoderUtils.java:55)
	at org.apache.beam.sdk.util.CoderUtils.clone(CoderUtils.java:168)
	at org.apache.beam.sdk.util.MutationDetectors$CodedValueMutationDetector.<init>(MutationDetectors.java:118)
	at org.apache.beam.sdk.util.MutationDetectors.forValueWithCoder(MutationDetectors.java:49)
	at org.apache.beam.runners.direct.ImmutabilityCheckingBundleFactory$ImmutabilityEnforcingBundle.add(ImmutabilityCheckingBundleFactory.java:115)
	at org.apache.beam.runners.direct.ParDoEvaluator$BundleOutputManager.output(ParDoEvaluator.java:305)

MapCoder.encode -> SchemaCoder.encode ->GetterBasedSchemaProvider.ToRowWithValueGetters passes a null value to withFieldValueGetters somehow, causing IllegalStateException

This PR made substantial change to the related code path. Somehow some encoding worked before now crash with the Exception above

Update: I now suspect it is due to unintended consequence of nullable annotation fix. I checked that where CodedValueMutationDetector.<init>(MutationDetectors.java:118) throws Exception, the element KV Coder trying to encode is sth like

KV{BigQueryTable{project=test-project1, dataset=test-dataset1, tableName=unpartitioned_table,
partitioningColumn=null, partitions=null, lastModificationTime=1731699015913000,
schemaSupplier=com.google.cloud.teleport.v2.utils.SerializableSchemaSupplier@2a2435f9,
dataplexEntityName=unpartitioned_table_entity},
null}, org.apache.beam.sdk.coders.KvCoder

make FieldValueTypeInformation static creators take a TypeDescriptor …

ce83352

…parameter

github-actions bot added java io extensions protobuf thrift labels Aug 5, 2024

github-actions bot added the Next Action: Reviewers label Aug 6, 2024

damondouglas self-requested a review August 14, 2024 19:52

damondouglas requested changes Aug 15, 2024

View reviewed changes

github-actions bot added the slow-review label Aug 26, 2024

github-actions bot removed the slow-review label Aug 28, 2024

github-actions bot added the slow-review label Sep 5, 2024

github-actions bot added Next Action: Author and removed Next Action: Reviewers slow-review labels Sep 5, 2024

unsuppress nullness (and sometimes rawtypes) warnings

d2e3478

github-actions bot added the aws label Sep 13, 2024

github-actions bot removed the slow-review label Oct 8, 2024

tilgalas closed this Oct 16, 2024

stankiewicz mentioned this pull request Oct 17, 2024

Schema inference parameterized types #32757

Merged

tilgalas reopened this Oct 22, 2024

github-actions bot added aws and removed aws labels Oct 22, 2024

reuvenlax reviewed Oct 22, 2024

View reviewed changes

ahmedabu98 mentioned this pull request Oct 25, 2024

[Bug]: ByteBuddy doesn't do well with Map<T, ByteBuffer> #32701

Open

17 tasks

tilgalas added 4 commits October 29, 2024 12:44

remove warning suppression in CachingFactory

ce5ff31

update typing around Row classes

70be685

remove old forField method

6ef957a

traverse class hierarchy in ReflectUtils.getMethods

0e7018d

github-actions bot added aws and removed aws labels Oct 29, 2024

reuvenlax merged commit dd556b2 into apache:master Oct 30, 2024
27 of 29 checks passed

damccorm mentioned this pull request Nov 13, 2024

[DO NOT MERGE] Run all PostCommit and PreCommit Tests against Release Branch #33101

Closed

damccorm reviewed Nov 13, 2024

View reviewed changes

damccorm mentioned this pull request Nov 13, 2024

Make AvroUtils compatible with older versions of Avro #33102

Merged

3 tasks

damccorm mentioned this pull request Nov 26, 2024

Upgrade to Beam 2.61.0 GoogleCloudPlatform/DataflowTemplates#2033

Merged

make FieldValueTypeInformation creators take a TypeDescriptor parameter #32081

make FieldValueTypeInformation creators take a TypeDescriptor parameter #32081

Conversation

tilgalas commented Aug 5, 2024 • edited Loading

GitHub Actions Tests Status (on master branch)

github-actions bot commented Aug 5, 2024

tilgalas commented Aug 5, 2024

tilgalas commented Aug 6, 2024

tilgalas commented Aug 6, 2024

tilgalas commented Aug 6, 2024

tilgalas commented Aug 6, 2024

github-actions bot commented Aug 6, 2024

damccorm commented Aug 14, 2024

damondouglas left a comment

Choose a reason for hiding this comment

tilgalas commented Aug 19, 2024

github-actions bot commented Aug 26, 2024

github-actions bot commented Aug 28, 2024

github-actions bot commented Sep 5, 2024

Abacn commented Sep 5, 2024

github-actions bot commented Oct 8, 2024

tilgalas commented Oct 14, 2024

tilgalas commented Oct 16, 2024

tilgalas commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tilgalas Oct 25, 2024 • edited Loading

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

tilgalas Oct 25, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmedabu98 commented Oct 25, 2024

reuvenlax commented Oct 28, 2024

tilgalas commented Oct 29, 2024

tilgalas commented Oct 30, 2024

tilgalas commented Oct 30, 2024

reuvenlax commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Abacn commented Nov 15, 2024 • edited Loading

tilgalas commented Aug 5, 2024 •

edited

Loading

tilgalas Oct 25, 2024 •

edited

Loading

tilgalas Oct 25, 2024 •

edited

Loading

Abacn commented Nov 15, 2024 •

edited

Loading