Skip to content

Conversation

@ggevay
Copy link
Contributor

@ggevay ggevay commented Nov 26, 2025

(This is on top of both #34283 and #34289. Only the last 2 commits are new.)

This does some refactoring and fixes a minor issue in 2 commits:

  1. commit: Refactor Command::ExecuteCopyTo to take a DataflowDescription instead of GlobalLirPlan. (Plus some minor code movement.)
  2. commit: Adds emit_optimizer_notices for COPY TO.

Sorry, I didn't add a test for this. Testing of COPY TO and notices are hard enough by themselves (COPY TO needs Testdrive, notices need pgtest), testing the two together seems impossibly hard.

@ggevay ggevay requested review from a team as code owners November 26, 2025 20:12
@ggevay ggevay added the A-ADAPTER Topics related to the ADAPTER layer label Nov 26, 2025
@ggevay ggevay requested a review from aljoscha November 26, 2025 20:12
@ggevay ggevay changed the title Frontend peek sequencing -- emit_optimizer_notices also for COPY TO Frontend peek sequencing -- emit_optimizer_notices also for COPY TO S3 Nov 26, 2025
Copy link
Contributor

@aljoscha aljoscha left a comment

Choose a reason for hiding this comment

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

looking good! that is the last two commits of this one.

global_lir_plan,
source_ids,
} => {
let (df_desc, df_meta) = global_lir_plan.unapply();
Copy link
Contributor

Choose a reason for hiding this comment

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

might have to be _df_meta?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used in the next commit: passed in to emit_optimizer_notices.

/// # Panics
///
/// Panics if the dataflow has no sink exports.
pub fn sink_id(&self) -> GlobalId {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this also wants to panic when there are more than one sink? but I'm not familiar enough with this code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, yes. I've now made it enforce that we have exactly one sink.

@ggevay ggevay force-pushed the frontend-peek-copy-to-emit-optimizer-notices branch from 3f278af to e679ba8 Compare November 27, 2025 17:23
@ggevay ggevay merged commit b24e7f0 into MaterializeInc:main Nov 27, 2025
249 of 334 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ADAPTER Topics related to the ADAPTER layer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants