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

Use builder pattern for IcebergQueryRunner #24515

Merged
merged 1 commit into from
Feb 20, 2025

Conversation

ZacBlanco
Copy link
Contributor

@ZacBlanco ZacBlanco commented Feb 7, 2025

Description

Currently we create IcebergQueryRunner through a complicated set of method with various overloads. We tend to just create a new constructor/method for every time we need to set a separate property which can make figuring out
the arguments you're trying to explicitly change for a query runner more difficult to determine.

This change instead introduces a class which uses builder pattern to make it easier to add new features and use.

Motivation and Context

Simpler and easier to understand query runner.

Impact

N/A. Testing only

Test Plan

Existing tests

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 7, 2025
@ZacBlanco ZacBlanco force-pushed the upstream-iceberg-qr-builder branch 6 times, most recently from fa98c76 to 427a2f4 Compare February 10, 2025 20:42
@ZacBlanco ZacBlanco marked this pull request as ready for review February 11, 2025 18:19
@ZacBlanco ZacBlanco requested review from hantangwangd and a team as code owners February 11, 2025 18:19
Copy link
Member

@hantangwangd hantangwangd left a comment

Choose a reason for hiding this comment

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

Thanks for this refactor. Overall lgtm, the builder pattern is wonderful. One thing I can think of is that multiple catalogs with the same connector type may affect each other in JMX statistics. But we can add comments on IcebergQueryRunner.addCatalog(...) to clarify this.

jaystarshot
jaystarshot previously approved these changes Feb 14, 2025
@jaystarshot
Copy link
Member

lgtm, some properties are removed as @hantangwangd pointed out

@ZacBlanco ZacBlanco force-pushed the upstream-iceberg-qr-builder branch from 427a2f4 to 2bcf851 Compare February 17, 2025 22:23
hantangwangd
hantangwangd previously approved these changes Feb 18, 2025
hantangwangd
hantangwangd previously approved these changes Feb 19, 2025
Additionally, add support for tracking catalog properties
added to iceberg. This can be useful for tests that may
require their own catalogs
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @ZacBlanco LGTM for Native engine changes.

@ZacBlanco ZacBlanco merged commit 15afc5c into prestodb:master Feb 20, 2025
60 checks passed
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.

5 participants