Skip to content

fix(bigquery): update internal Field.mode type to enum instead of string#13332

Open
logachev wants to merge 1 commit into
mainfrom
kirl/field_mode_type
Open

fix(bigquery): update internal Field.mode type to enum instead of string#13332
logachev wants to merge 1 commit into
mainfrom
kirl/field_mode_type

Conversation

@logachev
Copy link
Copy Markdown
Contributor

@logachev logachev commented Jun 2, 2026

There is constant conversion from string to enum in getter/setter. Updating Field private property to be Mode rather than string

@logachev logachev requested review from a team as code owners June 2, 2026 01:51
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the Field class and its builder to use the Mode enum internally instead of a String representation. While this simplifies getter and setter methods, feedback highlights that changing the type of this serialized field breaks backward serialization compatibility. This could lead to deserialization failures (such as InvalidClassException) in distributed environments like Apache Beam or Spark, so keeping the internal representation as a String is recommended if compatibility must be preserved.

private final LegacySQLTypeName type;
private final FieldList subFields;
private final String mode;
private final Mode mode;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Changing the internal field mode from String to Mode breaks backward serialization compatibility for the Field class, which implements Serializable. If instances of Field (or classes containing it, like Schema) are serialized and shared across different versions of the library (e.g., in distributed processing frameworks like Apache Beam or Spark), deserialization will fail with an InvalidClassException or ClassCastException due to the type mismatch. Since the performance overhead of Mode.valueOf(mode) is negligible, please consider whether breaking serialization compatibility is acceptable for this change. If compatibility must be preserved, it is safer to keep the internal representation as String.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant