Skip to content

HIVE-29478: FileSinkOperator shouldn't check for isMMTable for everyrow being processed#6341

Open
nareshpr wants to merge 1 commit intoapache:masterfrom
nareshpr:HIVE-29478
Open

HIVE-29478: FileSinkOperator shouldn't check for isMMTable for everyrow being processed#6341
nareshpr wants to merge 1 commit intoapache:masterfrom
nareshpr:HIVE-29478

Conversation

@nareshpr
Copy link
Contributor

What changes were proposed in this pull request?

conf.isMMTable() shouldn't be invoked for every row being processed. This came up as hotspot in ETL file write flow.

Why are the changes needed?

It improves write performance, specifically properties to map conversion is not required to validate a single property for every row being processed.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Tests were not added, as its performance improvement.

Copy link
Contributor

@abstractdog abstractdog left a comment

Choose a reason for hiding this comment

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

LGTM, pending tests

@abstractdog
Copy link
Contributor

@nareshpr : approved, but I've just realized there are a few more occurrences of Maps.fromProperties, can you take care of those please?

@nareshpr
Copy link
Contributor Author

yes, i will remove all Map conversion in AcidUtils.

@nareshpr
Copy link
Contributor Author

nareshpr commented Feb 28, 2026

@abstractdog I had done the changes, can you please review ?

@abstractdog
Copy link
Contributor

@abstractdog I had done the changes, can you please review ?

please take care of the sonarqube warnings (unused imports), other than that, looks good to me

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 2, 2026

@nareshpr
Copy link
Contributor Author

nareshpr commented Mar 2, 2026

Taken care of checkstyle unused import. Got successful green build.

Copy link
Contributor

@abstractdog abstractdog left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants