Skip to content

feat(Datum): allow to cast from Double to Float#11

Merged
askalt merged 1 commit intodf-52.3.0-release-bindingfrom
aonovikov/feat/datum_long_to_float_converting
Apr 21, 2026
Merged

feat(Datum): allow to cast from Double to Float#11
askalt merged 1 commit intodf-52.3.0-release-bindingfrom
aonovikov/feat/datum_long_to_float_converting

Conversation

@novartole
Copy link
Copy Markdown

What changes are included in this PR?

Extend Datum::to with Double to Float cast.

Are these changes tested?

A unit-test is added.

@novartole novartole requested review from LLDay, kryvashek and psergee and removed request for psergee April 13, 2026 22:09
Comment thread crates/iceberg/src/spec/values/datum.rs Outdated
@novartole novartole force-pushed the aonovikov/feat/datum_long_to_float_converting branch from c63a159 to 428d453 Compare April 16, 2026 09:55
@novartole novartole force-pushed the aonovikov/feat/datum_long_to_float_converting branch from 428d453 to f2d83f9 Compare April 16, 2026 11:58
Copy link
Copy Markdown

@LLDay LLDay left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Member

@kryvashek kryvashek left a comment

Choose a reason for hiding this comment

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

My only comment isn't critical, hence approved.

Comment on lines +51 to +52
pub(crate) const FLOAT_MAX: f32 = 3.4028235e38;
pub(crate) const FLOAT_MIN: f32 = -3.4028235e38;
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.

Why not

Suggested change
pub(crate) const FLOAT_MAX: f32 = 3.4028235e38;
pub(crate) const FLOAT_MIN: f32 = -3.4028235e38;
pub(crate) const FLOAT_MAX: f64 = f32::MAX as f64;
pub(crate) const FLOAT_MIN: f64 = f32::MIN as f64;

?
Less chance of error + already casted to required type (may remove in-code casts in that case).

Copy link
Copy Markdown
Author

@novartole novartole Apr 20, 2026

Choose a reason for hiding this comment

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

That absolutely makes sense. IDK why in the original code source this approach is used for other constants. I decided to keep this weird style for this part too.

@askalt askalt merged commit 50969b2 into df-52.3.0-release-binding Apr 21, 2026
12 of 17 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.

4 participants