-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Fix writtenIntermediateBytes metric to capture correct intermediate w… #21626
Conversation
47e8b39
to
ed8638a
Compare
@@ -81,7 +81,7 @@ public SubPlan createSubPlans(Session session, Plan plan, boolean forceSingleNod | |||
sqlParser, | |||
idAllocator, | |||
variableAllocator, | |||
getTableWriterNodeIds(plan.getRoot())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main fix
@@ -245,6 +245,16 @@ public static Set<PlanNodeId> getTableWriterNodeIds(PlanNode plan) | |||
.collect(toImmutableSet()); | |||
} | |||
|
|||
public static Set<PlanNodeId> getOutputTableWriterNodeIds(PlanNode plan) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clone of the above function with a filter on the temporaryTableWriters
@@ -25,6 +25,7 @@ | |||
|
|||
public class CteUtils | |||
{ | |||
public static final String TEMPORARY_TABLE_SCHEMA = "temporary_table_schema"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed thanks
ed8638a
to
1184a83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I also briefly checked the code to make sure that the "outputTableWriterFragment" variable is only used for accounting purpose.
Can you maybe add a comment for the "outputTableWriterFragment" member in "PlanFragment" class stating that this is true only for final output fragment, but not for temporary table writer?
1184a83
to
37ecfb2
Compare
cc: @tdcmeehan |
Nit (commenting because it's not enough for a review): in Description, suggest deleting the lines in the release note entry after the line |
Thanks! |
The writtenIntermediateBytes metric was noop because plan.isOutputTableWriterFragment() will always be true and wrong metrics were updated when there are temporary table writes currently since there was a bug in plan Fragmenter where it would pass temporary tables' writer operator as output table writes hence populating the wrong metric.
code link.
Fixed by adding the information (whether temporary) to the tablewriterNode and filtering out those
Impact
Fixed writtenIntermediateBytes metric. Users of CTE and Exchange materialization should now be able to see the correct populated metric
Test Plan
Production tested
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.