Skip to content

Core: Make PlanTableScanResponse buider withCredentials replace instead of append - #17751

Open
dramaticlly wants to merge 1 commit into
apache:mainfrom
dramaticlly:builder-credentials-replace-semantics
Open

Core: Make PlanTableScanResponse buider withCredentials replace instead of append#17751
dramaticlly wants to merge 1 commit into
apache:mainfrom
dramaticlly:builder-credentials-replace-semantics

Conversation

@dramaticlly

@dramaticlly dramaticlly commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Follow up of #17638 (comment) with changes in PlanTableScanResponse.Builder and FetchPlanningResultResponse.Builder

  1. withCredentials(List<Credential>) now replaces the builder's credentials instead of appending to them.
  2. It now rejects a null list and null elements as the follow up of Core: Add storage credentials to FetchPlanningResultResponse #14994 (comment)
  3. build() now passes ImmutableList.copyOf(credentials) instead of the builder's live ArrayList.

AI Disclosure

@github-actions github-actions Bot added the core label Aug 21, 2026
@dramaticlly dramaticlly changed the title Core: Make builder of withCredentials replace instead of append Core: Make PlanTableScanResponse buider withCredentials replace instead of append Aug 21, 2026
@dramaticlly

Copy link
Copy Markdown
Contributor Author

@singhpk234 @nastra @huaxingao if you want to take a look

credentials.addAll(credentialsToAdd);
public Builder withCredentials(List<Credential> newCredentials) {
Preconditions.checkArgument(null != newCredentials, "Invalid credentials list: null");
Preconditions.checkArgument(!newCredentials.contains(null), "Invalid credential: null");

@nastra nastra Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we actually need this check? We should get this for free when doing this.credentials = ImmutableList.copyOf(credentialsToAdd)

@dramaticlly dramaticlly Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think ImmutableList.copyOf(credentialsToAdd) will throw NPE instead of IllegalArgumentException instead, from AGENTS.md seem to favor "Preconditions.checkArgument over NPE"

and I saw we have some precedence in

Preconditions.checkArgument(!toAdd.contains(null), "Invalid table identifier: null");
and
Preconditions.checkArgument(!toAdd.contains(null), "Invalid namespace: null");

@dramaticlly
dramaticlly requested a review from nastra August 21, 2026 22:31
public Builder withCredentials(List<Credential> credentialsToAdd) {
credentials.addAll(credentialsToAdd);
public Builder withCredentials(List<Credential> newCredentials) {
Preconditions.checkArgument(null != newCredentials, "Invalid credentials: null");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

per

Suggested change
Preconditions.checkArgument(null != newCredentials, "Invalid credentials: null");
Preconditions.checkArgument(newCredentials, "Invalid credentials list : null");

Preconditions.checkNotNull(toAdd, "Invalid table identifier list: null");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @singhpk234 , I think ListTableResponse is relative old (merged Feb 2022) compare to the latest agents.md guideline in

- Use `ConcurrentMap` for shared mutable state. `Preconditions.checkArgument` over NPE.
which prefer IllegalArgumentException over NPE. Let me know if you want to switch to NPE instead

@dramaticlly
dramaticlly force-pushed the builder-credentials-replace-semantics branch from 1ed9d07 to 9f2604d Compare August 28, 2026 17:20
@dramaticlly
dramaticlly requested a review from singhpk234 August 28, 2026 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants