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

Refactor to use ColumnMetadata builder #24616

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

agrawalreetika
Copy link
Member

Description

Refactor to use ColumnMetadata builder

Motivation and Context

Refactor to use ColumnMetadata builder

Impact

Code clean up

Test Plan

Existing CI

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

== NO RELEASE NOTE ==

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Feb 24, 2025
Copy link
Contributor

@ZacBlanco ZacBlanco left a comment

Choose a reason for hiding this comment

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

I see that this builder class existed before, but do we gain anything by unilaterally forcing developers to use this API this across the code base, other than some small usability improvements?

Also, one concern I have is that ColumnMetadata pretty much always requires having at least name and type field by default, so I wonder if having those in the constructor of the Builder class would make sense. The rest of the fields are "optional" in a sense that they may not be set by users as frequently. Atleast then we could force correct usage at compile-time rather than fail at runtime in the case someone forgets to set the name or type on the builder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:IBM PR from IBM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants