Skip to content

feat: adding math sec expression#4371

Merged
comphead merged 1 commit into
apache:mainfrom
athlcode:math_sec
May 20, 2026
Merged

feat: adding math sec expression#4371
comphead merged 1 commit into
apache:mainfrom
athlcode:math_sec

Conversation

@athlcode
Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part #4150

Rationale for this change

sec already has a native implementation in the datafusion-spark crate, but was not yet wired into Comet, so queries using it fell back to Spark.

What changes are included in this PR?

Wires the Spark Sec expression to the datafusion-spark SparkSec UDF (Pattern B: register the UDF in jni_api.rs + one-line passthrough in QueryPlanSerde.scala). Also updates the supported-expression docs.

How are these changes tested?

Added a SQL file test (math/sec.sql).

Copy link
Copy Markdown
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 @athlcode just wondering, is this PR created with wire_datafusion_function skill?

@athlcode
Copy link
Copy Markdown
Contributor Author

Thanks @athlcode just wondering, is this PR created with wire_datafusion_function skill?

welcome @comphead. Yes, the PR was created referring the wire_datafusion_function skill

@comphead
Copy link
Copy Markdown
Contributor

welcome @comphead. Yes, the PR was created referring the wire_datafusion_function skill

thanks for trying it, it tested for simple wiring, and we need to make it also work for more complicated usecases, where datatypes can be partially supported or behavior is different, etc

@athlcode
Copy link
Copy Markdown
Contributor Author

welcome @comphead. Yes, the PR was created referring the wire_datafusion_function skill

thanks for trying it, it tested for simple wiring, and we need to make it also work for more complicated usecases, where datatypes can be partially supported or behavior is different, etc

Shall I try by expanding the skill file for more complicated usecases?

@comphead
Copy link
Copy Markdown
Contributor

Shall I try by expanding the skill file for more complicated usecases?

Thats the end goal, to let it cover most of usecases, leaving manual work as less as possible

Copy link
Copy Markdown
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 @athlcode
CI pending

@athlcode
Copy link
Copy Markdown
Contributor Author

Shall I try by expanding the skill file for more complicated usecases?

Thats the end goal, to let it cover most of usecases, leaving manual work as less as possible

yep completely agree with you

@comphead comphead merged commit 41d2e0c into apache:main May 20, 2026
148 of 149 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants