Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The Field mode normalization for null values is incorrect and sets it as an empty String. #2730

Closed
keskival opened this issue Jun 1, 2023 · 2 comments · Fixed by #2863
Closed
Assignees
Labels
api: bigquery Issues related to the googleapis/java-bigquery API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@keskival
Copy link

keskival commented Jun 1, 2023

The Field mode normalization for null values is incorrect and sets it as an empty String.

/** Sets the mode of the field. When not specified {@link Mode#NULLABLE} is used. */
public Builder setMode(Mode mode) {
this.mode = mode != null ? mode.name() : Data.<String>nullOf(String.class);
return this;
}

Instead of setting the mode String as "" here, it should be set to Field.Mode.NULLABLE.name().

This causes the following problem: When the Field is deserialized from protobuf response from BigQuery, it might have mode set to null. Specifically this seems to happen when a table has been created with SQL CREATE TABLE as it doesn't support setting the field as NULLABLE explicitly.

Steps to reproduce

Create a BigQuery table with some nullable columns using CREATE TABLE.

Get the schema from Java with bigQuery.getTable().getSchema(), compare with a Schema you have created by setting the mode to Field.Mode.NULLABLE explicitly using Field.equals(field2).

Expected result: Both Schemas are equal.

Result: Schemas are inequal. This is because comparison is done for Protobuf representation where an empty String is a placeholder for null.

Code example

  @Test
  public void assertThatSchemaEqualsComparisonIsDeterministic() {
    final Field field1 = Field
      .of("field1", LegacySQLTypeName.INTEGER)
      .toBuilder()
      .setMode(Field.Mode.NULLABLE)
      .build();
    final Field field1b = Field
      .of("field1", LegacySQLTypeName.INTEGER)
      .toBuilder()
      .setMode(null)
      .build();
    final Schema schema1 = Schema.of(field1);
    final Schema schema2 = Schema.of(field1b);
    assertThat(schema1.equals(schema2)).isEqualTo(true);
  }
@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/java-bigquery API. label Jun 1, 2023
@keskival
Copy link
Author

keskival commented Jun 2, 2023

Alternatively, this should use Data.isNull:

return mode != null ? Mode.valueOf(mode) : null;

But while that fixes the getter, it doesn't fix the normalization of the default value.

@Neenu1995 Neenu1995 added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Jun 13, 2023
@obada-ab obada-ab self-assigned this Aug 10, 2023
@obada-ab
Copy link
Member

obada-ab commented Sep 5, 2023

Alternatively, this should use Data.isNull:

return mode != null ? Mode.valueOf(mode) : null;

But while that fixes the getter, it doesn't fix the normalization of the default value.

I think this change makes sense.

And as for normalization, I don't think null field mode needs to be equivalent to Field.Mode.NULLABLE at the client side. They might be treated the same by BigQuery but the client code shouldn't be coupled to this behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/java-bigquery API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants