Skip to content

feat: Add support for Spark ToDegrees and ToRadians math expressions#3786

Merged
andygrove merged 5 commits intoapache:mainfrom
rafafrdz:feat/add-degrees-radians-functions
May 5, 2026
Merged

feat: Add support for Spark ToDegrees and ToRadians math expressions#3786
andygrove merged 5 commits intoapache:mainfrom
rafafrdz:feat/add-degrees-radians-functions

Conversation

@rafafrdz
Copy link
Copy Markdown
Contributor

@rafafrdz rafafrdz commented Mar 25, 2026

Summary

  • Add support for Spark's ToDegrees and ToRadians angle conversion expressions
  • Delegates to DataFusion's built-in degrees and radians functions via CometScalarFunction
  • No native Rust code needed — DataFusion's function registry handles execution
  • SQL tests added covering standard angle values, NULL, and NaN

Test plan

  • SQL tests added: degrees.sql, radians.sql
  • CometSqlFileTestSuite verified — all 4 tests pass (2 per function × dictionary=false/true)

Related to #2084

Add angle conversion functions by delegating to DataFusion's built-in
degrees and radians implementations via CometScalarFunction.
Includes SQL tests for column and literal arguments with edge cases.
@rafafrdz rafafrdz force-pushed the feat/add-degrees-radians-functions branch from ee3865c to 53e30ef Compare March 25, 2026 06:25
@rafafrdz rafafrdz marked this pull request as ready for review March 25, 2026 06:45
-- specific language governing permissions and limitations
-- under the License.

-- ConfigMatrix: parquet.enable.dictionary=false,true
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.

No need to test dictionary encoding since the data being inserted does not have duplicates. Also dictionaries are now unpacked before expressions get evaluated.

Suggested change
-- ConfigMatrix: parquet.enable.dictionary=false,true

-- specific language governing permissions and limitations
-- under the License.

-- ConfigMatrix: parquet.enable.dictionary=false,true
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.

Same comment as above - we can remove this

Suggested change
-- ConfigMatrix: parquet.enable.dictionary=false,true

Copy link
Copy Markdown
Member

@andygrove andygrove 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 CI. Thanks @rafafrdz. Sorry for the very late review.

@andygrove andygrove merged commit e5351f4 into apache:main May 5, 2026
160 of 161 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