Skip to content

Conversation

@Jefffrey
Copy link
Contributor

Which issue does this PR close?

Part of #12725

Rationale for this change

Prefer to avoid user_defined for consistency in function definitions.

What changes are included in this PR?

Refactor signature of substr to use coercion API instead of being user defined.

Are these changes tested?

Existing tests.

Are there any user-facing changes?

No

@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Nov 12, 2025
Copy link
Member

@martin-g martin-g left a comment

Choose a reason for hiding this comment

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

LGTM!

signature: Signature::one_of(
vec![
TypeSignature::Coercible(vec![string.clone(), int64.clone()]),
TypeSignature::Coercible(vec![
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

],
Volatility::Immutable,
)
.with_parameter_names(vec![
Copy link
Contributor

Choose a reason for hiding this comment

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

would be even nice to refactor such blocks in future so param names and types put together like

f.withNames("str", "str_pos").withTpyes("string", "i64")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Raised for tracking: #18685

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

Thanks @Jefffrey

@Jefffrey Jefffrey added this pull request to the merge queue Nov 14, 2025
Merged via the queue into apache:main with commit 6685bbe Nov 14, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants