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

Add builder.default annotations to fields in CreateUpdateTableRequestBody with default values defined #97

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ctrezzo
Copy link
Collaborator

@ctrezzo ctrezzo commented May 3, 2024

Summary

stageCreate and tableType fields have defaults defined in CreateUpdateTableRequestBody, but they are missing the @Builder.Default annotation. This means that when doing CreateUpdateTableRequestBody.builder().build() it will not have the default values populated in the returned instance. This pull request adds the annotations to ensure the builder is consistent with the default values specified.

Changes

  • Client-facing API Changes
  • Internal API Changes
  • Bug Fixes
  • New Features
  • Performance Improvements
  • Code Style
  • Refactoring
  • Documentation
  • Tests

See summary.

Testing Done

  • Manually Tested on local docker setup. Please include commands ran, and their output.
  • Added new tests for the changes made.
  • Updated existing tests to reflect the changes made.
  • No tests added or updated. Please explain why. If unsure, please feel free to ask for help.
  • Some other form of testing like staging or soak time in production. Please explain.

I ran existing unit tests and all passed. There should be no behavior change here, just preventing potential issues in the future.

Additional Information

  • Breaking Changes
  • Deprecations
  • Large PR broken into smaller PRs, and PR plan linked in the description.

N/A

@@ -103,6 +104,7 @@ public class CreateUpdateTableRequestBody {

@Schema(description = "The type of a table")
@Valid
@Builder.Default
private TableType tableType = TableType.PRIMARY_TABLE;

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we do a log-screenshot check again at tableservice layer that verifies:

  1. if request field is not provided by the client, springboot sets it as the respective default
  2. if request field is provided by the client, springboot honors it
    ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do!

Copy link
Collaborator Author

@ctrezzo ctrezzo Jun 19, 2024

Choose a reason for hiding this comment

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

@HotSushi Finally got around to this. Here are screenshots of 2 curl commands. One does not specify a table type and the other specifies a replica table type (i.e. not the default). The screenshots show the proper responses.
Screenshot 2024-06-19 at 11 44 12 AM
Screenshot 2024-06-19 at 11 46 03 AM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also here are screenshots of the persisted metadata in the iceberg metadata file for the tables showing the proper table type.
Screenshot 2024-06-19 at 3 55 18 PM
Screenshot 2024-06-19 at 3 56 42 PM

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.

2 participants