Skip to content

[GLUTEN-8782][VL] Add correct suffix to the name of merge_extract companion function#11210

Merged
zhouyuan merged 1 commit intoapache:mainfrom
rui-mo:wip_suffix
Nov 28, 2025
Merged

[GLUTEN-8782][VL] Add correct suffix to the name of merge_extract companion function#11210
zhouyuan merged 1 commit intoapache:mainfrom
rui-mo:wip_suffix

Conversation

@rui-mo
Copy link
Copy Markdown
Contributor

@rui-mo rui-mo commented Nov 27, 2025

What changes are proposed in this pull request?

Add correct suffix to the name of merge_extract companion function, so we no longer need the change made to Velox IBM/velox@b8da5d0.

How was this patch tested?

Verified with existing unit tests.

Related issue: #8782

@rui-mo rui-mo force-pushed the wip_suffix branch 2 times, most recently from 835ac86 to adfd329 Compare November 27, 2025 14:49
auto functionName = baseName + "_merge_extract";
auto signatures = exec::getAggregateFunctionSignatures(functionName);
if (signatures.has_value() && signatures.value().size() > 0) {
// The merge_extract function is registered without suffix.
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.

nit: Put the comment on line 280.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This comment is true only when the condition is met, so I assume keeping it here is more appropriate. Thanks.

@xinghuayu007
Copy link
Copy Markdown
Contributor

+1

VELOX_BRANCH=dft-2025_11_26
VELOX_ENHANCED_BRANCH=ibm-2025_11_26
VELOX_BRANCH=dft-2025_11_26_fix
VELOX_ENHANCED_BRANCH=ibm-2025_11_26_fix
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cc: @FelixYBW
removed one commit from original OAP patches

@zhouyuan zhouyuan merged commit 6d10695 into apache:main Nov 28, 2025
60 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.

3 participants