fix(bigquery): support optional fields in BigLakeConfiguration to prevent NPE on Iceberg/Lakehouse tables#13733
fix(bigquery): support optional fields in BigLakeConfiguration to prevent NPE on Iceberg/Lakehouse tables#13733keshavdandeva wants to merge 4 commits into
Conversation
…vent NPE on Iceberg/Lakehouse tables
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates BigLakeConfiguration to allow nullable fields, adding @Nullable annotations to the getters and builder setters, and removing the "[Required]" designation from the builder's Javadocs. It also adds unit tests to verify behavior when fields are null. The review feedback identifies copy-paste errors in the Javadocs for getTableFormat() and setTableFormat(), which incorrectly refer to "file format" and "PARQUET" instead of "table format" and "ICEBERG".
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates BigLakeConfiguration by adding @Nullable annotations to its getters and builder setters, updating Javadoc comments, and adding unit tests to verify null field handling. The feedback suggests using the more idiomatic assertNull(value) instead of assertEquals(null, value) in the new test cases.
| * | ||
| * @return value or {@code null} for none | ||
| */ | ||
| @Nullable |
There was a problem hiding this comment.
we are actually trying to migrate to use JSpecify (timeline for full migration in handwritten libraries is not set). Is it possible to use the JSpecify nullable annotations instead of javax (atleast for new annotations)?
| /** | ||
| * [Required] Required and immutable. Credential reference for accessing external storage | ||
| * system. Normalized as project_id.location_id.connection_id. | ||
| * Immutable. Credential reference for accessing external storage system. Normalized as |
There was a problem hiding this comment.
nit: What does immutable mean in this case (on a setter)? do you think we can just remove it as it would be implied to not be changing when the class is built?
lqiu96
left a comment
lqiu96
left a comment
There was a problem hiding this comment.
LGTM. Can you see if we can use JSpecify annotations? If there is some issue with autovalue integration, javax is fine for now as well
2 participants
b/534291682
This PR resolves a
NullPointerExceptionencountered when querying or retrieving metadata for 4-part PCNT (Project.Catalog.Namespace.Table) Apache Iceberg tables governed by external REST catalogs.Changes
connectionId,fileFormat,storageUri,tableFormat) and their corresponding setters in@AutoValue.Builderwith@NullableinBigLakeConfiguration.java.BigLakeConfiguration.javato remove incorrect[Required]notations and updatedgetTableFormat()Javadoc to reflect the correct table format reference (e.g., Iceberg) instead of file format.BigLakeConfigurationTest.java(testNullFields,testFromPbWithNullFields) to verify that constructing the configuration or mapping it from API models works successfully when properties are null.