Skip to content

Conversation

@dybyte
Copy link
Contributor

@dybyte dybyte commented Oct 6, 2025

Purpose of this pull request

  • Introduce a new configuration option physical-dag-enabled that allows users to control whether the engine generates and executes a physical DAG (optimized for execution), or uses the logical DAG as defined in the configuration.

  • Fix a bug in pipeline lookup logic where key comparison used different types, which could cause failures when mapping logical DAG vertices to pipelines.

Does this PR introduce any user-facing change?

Yes. Users can now use the new configuration option physical-dag-enabled to control DAG execution mode.
The default value maintains backward compatibility.

How was this patch tested?

I verified that JobDAGInfo outputs differ between physical-dag-enabled = true and physical-dag-enabled = false with a new test case.

Check list

@Hisoka-X
Copy link
Member

what's different of behavior of engine or job when enable or disable physical-dag-enabled? Or what benefits will this bring to users?

@dybyte
Copy link
Contributor Author

dybyte commented Oct 10, 2025

what's different of behavior of engine or job when enable or disable physical-dag-enabled? Or what benefits will this bring to users?

The engine or job behavior does not change depending on the physical-dag-enabled setting.
This option only controls whether the user sees the physical DAG info or the logical DAG info for visualization. I misunderstood this before.
In fact, the code to show the logical DAG was implemented long ago in #3649 , but since isPhysicalDAGInfo was always set to true, this logic was never actually used until now. @Hisoka-X

@dybyte
Copy link
Contributor Author

dybyte commented Oct 10, 2025

I think we may also need to update this file: RestHttpGetCommandProcessor.java.
Do you think that's correct?

@Hisoka-X
Copy link
Member

This option only controls whether the user sees the physical DAG info or the logical DAG info for visualization

So I think we should not add an config in seatunnel.yaml for this. We can only support it in restful api, WDYT?

@dybyte
Copy link
Contributor Author

dybyte commented Oct 14, 2025

So I think we should not add an config in seatunnel.yaml for this. We can only support it in restful api, WDYT?

I agree. Should we support it in restful api and client too?

@Hisoka-X
Copy link
Member

I agree. Should we support it in restful api and client too?

LGTM :)

@dybyte dybyte marked this pull request as draft November 4, 2025 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants