Skip to content
This repository was archived by the owner on Mar 5, 2026. It is now read-only.

fix: required table fields with value expression should be proto optional#532

Merged
alvarowolfx merged 3 commits intomainfrom
fix-proto-required-field
Jan 8, 2025
Merged

fix: required table fields with value expression should be proto optional#532
alvarowolfx merged 3 commits intomainfrom
fix-proto-required-field

Conversation

@alvarowolfx
Copy link
Copy Markdown
Contributor

When a BigQuery table column mode is set to REQUIRED, when converted to a protobuf label, it was being converted as LABEL_REQUIRED. This would make protobufjs encoding methods to set an empty value to it when converting to the wire format. But if the column has a defaultValueExpression and we want the backend to fill the data, we need to let the value stay null | undefined.

This PR checks if there is a defaultValueExpression defined on the column and change the label to LABEL_OPTIONAL to let the backend fill the data.

Fixes #531 🦕

@alvarowolfx alvarowolfx requested review from a team, leahecole and shollyman January 6, 2025 16:22
@product-auto-label product-auto-label Bot added size: s Pull request size is small. api: bigquerystorage Issues related to the googleapis/nodejs-bigquery-storage API. labels Jan 6, 2025
@leahecole leahecole changed the title fix: required table fields with value expresssion should be proto optional fix: required table fields with value expression should be proto optional Jan 6, 2025
Copy link
Copy Markdown
Contributor

@leahecole leahecole left a comment

Choose a reason for hiding this comment

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

LGTM, but would like @shollyman to do a confidence check on this logic

Comment thread protos/protos.d.ts
Comment thread protos/protos.js
Copy link
Copy Markdown
Contributor

@shollyman shollyman left a comment

Choose a reason for hiding this comment

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

This is an interesting interaction with schema inference, given that the client side required-ness isn't strictly compatible with the default value feature. Is it worth adding some more documentation to the adapt package to explain why this happens?

@product-auto-label product-auto-label Bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jan 8, 2025
@alvarowolfx alvarowolfx added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 8, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 8, 2025
@alvarowolfx alvarowolfx added the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 8, 2025
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 8, 2025
@alvarowolfx alvarowolfx added the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 8, 2025
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 8, 2025
@alvarowolfx alvarowolfx merged commit f125792 into main Jan 8, 2025
@alvarowolfx alvarowolfx deleted the fix-proto-required-field branch January 8, 2025 21:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: bigquerystorage Issues related to the googleapis/nodejs-bigquery-storage API. size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can I specify CURRENT_TIMESTAMP for rows by Storage Write API?

4 participants