Skip to content

HIVE-29519: Redundant queryStartTime update#6386

Merged
deniskuzZ merged 1 commit into
apache:masterfrom
deniskuzZ:driver_refactor
Mar 25, 2026
Merged

HIVE-29519: Redundant queryStartTime update#6386
deniskuzZ merged 1 commit into
apache:masterfrom
deniskuzZ:driver_refactor

Conversation

@deniskuzZ

@deniskuzZ deniskuzZ commented Mar 21, 2026

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Removes redundant queryStartTime update call

Why are the changes needed?

The Compiler.createPlan() always sets queryStartTime from QueryDisplay. So every compileInternal call already
handles the start time.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Jenkins

@deniskuzZ deniskuzZ changed the title HIVE-29519: Redundant perfLogger reset and queryStartTime update HIVE-29519: PerfLogger reset and queryStartTime update refactor Mar 21, 2026
@deniskuzZ deniskuzZ changed the title HIVE-29519: PerfLogger reset and queryStartTime update refactor HIVE-29519: Redundant queryStartTime update Mar 21, 2026
@sonarqubecloud

Copy link
Copy Markdown

@Aggarwal-Raghav

Copy link
Copy Markdown
Contributor

LGTM +1 (non-binding)

@Aggarwal-Raghav

Copy link
Copy Markdown
Contributor

nit: unrelated to the patch, there is typo in destroyed:

}

public void recordQueryStartTime() {
plan.setQueryStartTime(queryDisplay.getQueryStartTime());

@abstractdog abstractdog Mar 25, 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.

if you remove this, the only usage of plan.setQueryStartTime() is from TestHiveProtoLoggingHook, does it make sense to add @VisibleForTesting to setQueryStartTime to let the code readers know that they should not set this directly?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this method was added by me, there were no usages except Driver

@abstractdog abstractdog self-requested a review March 25, 2026 10:14
@deniskuzZ deniskuzZ merged commit 1e75615 into apache:master Mar 25, 2026
2 checks passed
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.

4 participants