Skip to content

Conversation

@davidlghellin
Copy link
Contributor

@davidlghellin davidlghellin commented Sep 22, 2025

Which issue does this PR close?

part of #15914

Rationale for this change

Migrate spark functions from https://github.com/lakehq/sail/ to datafusion engine to unify codebase

What changes are included in this PR?

implement spark udf elt
https://spark.apache.org/docs/latest/api/sql/index.html#elt

Are these changes tested?

unit-tests and sqllogictests added

Are there any user-facing changes?

now can be called in queries

@davidlghellin davidlghellin marked this pull request as draft September 22, 2025 22:03
@github-actions github-actions bot added the spark label Sep 22, 2025
@davidlghellin davidlghellin changed the title feat(spark): implement Spark elt function feat(spark): implement Spark elt function Sep 22, 2025
@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Sep 24, 2025
@davidlghellin davidlghellin marked this pull request as ready for review September 24, 2025 20:13
continue;
};

let ansi_enable: bool = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

this field seems is not changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will see to put this comments
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can put these comments
To know and don´t forget

Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove this code itself but leave a TODO comment for ANSI support, otherwise it can be confusing why we have a static boolean variable here

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 @davidlghellin it is good overall, just some minor comments

@davidlghellin davidlghellin marked this pull request as draft September 25, 2025 05:30
@davidlghellin davidlghellin marked this pull request as ready for review October 12, 2025 11:14
Copy link
Contributor

@Jefffrey Jefffrey left a comment

Choose a reason for hiding this comment

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

Functionally looks good, have some further suggestions for refactoring

Comment on lines 113 to 117
for a in args.iter().skip(1) {
let casted = cast(a, &Utf8)?;
let sa = casted.as_string::<i32>().clone();
cols.push(Arc::new(sa));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically coerce_types() should have done this casting for us by now

continue;
};

let ansi_enable: bool = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove this code itself but leave a TODO comment for ANSI support, otherwise it can be confusing why we have a static boolean variable here

@alamb
Copy link
Contributor

alamb commented Oct 14, 2025

Thank you for the contribution @davidlghellin and the review @Jefffrey

We are picking up steam now on the spark functions 🚂

@alamb alamb added this pull request to the merge queue Oct 14, 2025
Merged via the queue into apache:main with commit 8b081e9 Oct 14, 2025
28 checks passed
tobixdev pushed a commit to tobixdev/datafusion that referenced this pull request Nov 2, 2025
* feat(spark): implement Spark elt function

* feat(spark): implement Spark elt function license

* feat(spark): implement Spark elt function license complettly

* test slt

* doc

* wip

* change error

* fmt

* some commts

* some changes

* coerce basic <- udf

* coerce basic drop 32

* change n,k,j names

* first value int64 or casteable

* change revision

* change revision test

* change revision test fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

spark sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants