Skip to content

Commit 795d1ed

Browse files
jobarr-amznjobarr-amzn
authored
Standardize number handling in ion jq (#198)
1 parent d8d7dd4 commit 795d1ed

File tree

1 file changed

+51
-88
lines changed
  • src/bin/ion/commands

1 file changed

+51
-88
lines changed

‎src/bin/ion/commands/jq.rs

Lines changed: 51 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,7 @@ impl Add for JaqElement {
255255
// Sequences and strings concatenate
256256
(List(a), List(b)) => ion_rs::List::from_iter(a.into_iter().chain(b)).into(),
257257
(SExp(a), SExp(b)) => ion_rs::SExp::from_iter(a.into_iter().chain(b)).into(),
258+
//TODO: Does it make sense to concatenate a String and a Symbol? What type results?
258259
(String(a), String(b)) => format!("{}{}", a.text(), b.text()).into(),
259260
(Symbol(a), Symbol(b)) => match (a.text(), b.text()) {
260261
(Some(ta), Some(tb)) => format!("{}{}", ta, tb),
@@ -264,22 +265,18 @@ impl Add for JaqElement {
264265
.into(),
265266

266267
// Structs merge
267-
//TODO: Recursively remove duplicate fields, see doc comment for method
268+
//TODO: Recursively remove duplicate fields, see doc comment for rules
268269
(Struct(a), Struct(b)) => a.clone_builder().with_fields(b.fields()).build().into(),
269270

270271
// Number types, only lossless operations
271272
(Int(a), Int(b)) => (a + b).into(),
272-
(Float(a), Float(b)) => (a + b).into(),
273+
(Float(a), Float(b)) => a.add(b).into(),
273274
(Decimal(a), Decimal(b)) => a.add(b).into(),
274275
(Decimal(a), Int(b)) | (Int(b), Decimal(a)) => a.add(b).into(),
275276

276277
// Only try potentially lossy Float conversions when we've run out of the other options
277-
(a @ Int(_) | a @ Decimal(_), Float(b)) | (Float(b), a @ Int(_) | a @ Decimal(_)) => {
278-
let Some(f) = a.clone().to_f64() else {
279-
return jaq_unary_error(a, "cannot be an f64");
280-
};
281-
(f + b).into()
282-
}
278+
(a @ Int(_) | a @ Decimal(_), Float(b)) => (a.to_f64().unwrap() + b).into(),
279+
(Float(a), b @ Int(_) | b @ Decimal(_)) => (a + b.to_f64().unwrap()).into(),
283280

284281
(a, b) => return jaq_binary_error(a, b, "cannot be added"),
285282
};
@@ -301,40 +298,28 @@ impl Sub for JaqElement {
301298
use ion_math::{DecimalMath, ToFloat};
302299
use Value::*;
303300

301+
// b.iter.contains() will make these implementations O(N^2).
302+
// Neither Element nor Value implement Hash or Ord, so faster lookup isn't available
303+
// Perhaps someday we can do something more clever with ionhash or IonOrd?
304+
fn remove_elements(a: Sequence, b: &Sequence) -> impl Iterator<Item = Element> + '_ {
305+
a.into_iter().filter(|i| !b.iter().contains(i))
306+
}
307+
304308
let elt: Element = match (lhv, rhv) {
305309
// Sequences and strings do set subtraction with RHS
306-
// b.iter.contains() will make these implementations O(N^2).
307-
// Neither Element nor Value implement Hash or Ord, so faster lookup isn't available
308-
// Perhaps someday we can do something more clever with ion-hash or IonOrd?
309-
(List(a), List(b)) => {
310-
let a_less_b = a.into_iter().filter(|i| !b.iter().contains(i));
311-
ion_rs::List::from_iter(a_less_b).into()
312-
}
313-
(SExp(a), SExp(b)) => {
314-
let a_less_b = a.into_iter().filter(|i| !b.iter().contains(i));
315-
ion_rs::SExp::from_iter(a_less_b).into()
316-
}
310+
(List(a), List(b)) => ion_rs::List::from_iter(remove_elements(a, &b)).into(),
311+
(SExp(a), SExp(b)) => ion_rs::SExp::from_iter(remove_elements(a, &b)).into(),
317312

318313
// Number types, only lossless operations
319314
(Int(a), Int(b)) => (a + -b).into(), //TODO: use bare - with ion-rs > rc.11
320-
(Float(a), Float(b)) => (a - b).into(),
315+
(Float(a), Float(b)) => a.sub(b).into(),
321316
(Decimal(a), Decimal(b)) => a.sub(b).into(),
322317
(Decimal(a), Int(b)) => a.sub(b).into(),
323318
(Int(a), Decimal(b)) => a.sub(b).into(),
324319

325320
// Only try potentially lossy Float conversions when we've run out of the other options
326-
(a @ Int(_) | a @ Decimal(_), Float(b)) => {
327-
let Some(f) = a.clone().to_f64() else {
328-
return jaq_unary_error(a, "cannot be an f64");
329-
};
330-
(f - b).into()
331-
}
332-
(Float(a), b @ Int(_) | b @ Decimal(_)) => {
333-
let Some(f) = b.clone().to_f64() else {
334-
return jaq_unary_error(b, "cannot be an f64");
335-
};
336-
(a - f).into()
337-
}
321+
(a @ Int(_) | a @ Decimal(_), Float(b)) => (a.to_f64().unwrap() - b).into(),
322+
(Float(a), b @ Int(_) | b @ Decimal(_)) => (a - b.to_f64().unwrap()).into(),
338323

339324
(a, b) => return jaq_binary_error(a, b, "cannot be subtracted"),
340325
};
@@ -368,12 +353,11 @@ impl Mul for JaqElement {
368353

369354
(Symbol(a), Int(b)) | (Int(b), Symbol(a)) => match (b.as_usize(), a.text()) {
370355
(Some(n), Some(t)) => t.repeat(n).into(),
371-
//TODO: Handle symbols with unknown text?
372-
_ => Null(IonType::Null).into(),
356+
_ => Null(IonType::Null).into(), //TODO: Handle symbols with unknown text? How?
373357
},
374358

375359
// Structs merge recursively
376-
//TODO: Recursively remove duplicate fields, recursively merge if struct fields collide
360+
//TODO: Recursively remove duplicate fields, see doc comments for rules
377361
(Struct(a), Struct(b)) => a.clone_builder().with_fields(b.fields()).build().into(),
378362

379363
// Number types, only lossless operations
@@ -384,12 +368,8 @@ impl Mul for JaqElement {
384368
(Decimal(a), Int(b)) | (Int(b), Decimal(a)) => a.mul(b).into(),
385369

386370
// Only try potentially lossy Float conversions when we've run out of the other options
387-
(a @ Int(_) | a @ Decimal(_), Float(b)) | (Float(b), a @ Int(_) | a @ Decimal(_)) => {
388-
let Some(f) = a.clone().to_f64() else {
389-
return jaq_unary_error(a, "cannot be an f64");
390-
};
391-
(f * b).into()
392-
}
371+
(a @ Int(_) | a @ Decimal(_), Float(b)) => (a.to_f64().unwrap() * b).into(),
372+
(Float(a), b @ Int(_) | b @ Decimal(_)) => (a * b.to_f64().unwrap()).into(),
393373

394374
(a, b) => return jaq_binary_error(a, b, "cannot be multiplied"),
395375
};
@@ -413,13 +393,13 @@ impl Div for JaqElement {
413393
let elt: Element = match (lhv, rhv) {
414394
// Dividing a string by another splits the first using the second as separators.
415395
(String(a), String(b)) => {
416-
let split = a.text().split(b.text());
417-
let iter = split.map(|s| String(s.into())).map(Element::from);
396+
let (ta, tb) = (a.text(), b.text());
397+
let iter = ta.split(tb).map(ion_rs::Str::from).map(Element::from);
418398
ion_rs::List::from_iter(iter).into()
419399
}
420400
(Symbol(a), Symbol(b)) => match (a.text(), b.text()) {
421401
(Some(ta), Some(tb)) => {
422-
let iter = ta.split(tb).map(|s| Symbol(s.into())).map(Element::from);
402+
let iter = ta.split(tb).map(ion_rs::Symbol::from).map(Element::from);
423403
ion_rs::List::from_iter(iter)
424404
}
425405
//TODO: Handle symbols with unknown text?
@@ -435,18 +415,8 @@ impl Div for JaqElement {
435415
(Int(a), Decimal(b)) => a.div(b).into(),
436416

437417
// Only try potentially lossy Float conversions when we've run out of the other options
438-
(a @ Int(_) | a @ Decimal(_), Float(b)) => {
439-
let Some(f) = a.clone().to_f64() else {
440-
return jaq_unary_error(a, "cannot be an f64");
441-
};
442-
(f / b).into()
443-
}
444-
(Float(a), b @ Int(_) | b @ Decimal(_)) => {
445-
let Some(f) = b.clone().to_f64() else {
446-
return jaq_unary_error(b, "cannot be an f64");
447-
};
448-
(a / f).into()
449-
}
418+
(a @ Int(_) | a @ Decimal(_), Float(b)) => (a.to_f64().unwrap() / b).into(),
419+
(Float(a), b @ Int(_) | b @ Decimal(_)) => (a / b.to_f64().unwrap()).into(),
450420

451421
(a, b) => return jaq_binary_error(a, b, "cannot be divided"),
452422
};
@@ -473,18 +443,8 @@ impl Rem for JaqElement {
473443
(Int(a), Decimal(b)) => a.rem(b).into(),
474444

475445
// Only try potentially lossy Float conversions when we've run out of the other options
476-
(a @ Int(_) | a @ Decimal(_), Float(b)) => {
477-
let Some(f) = a.clone().to_f64() else {
478-
return jaq_unary_error(a, "cannot be an f64");
479-
};
480-
(f % b).into()
481-
}
482-
(Float(a), b @ Int(_) | b @ Decimal(_)) => {
483-
let Some(f) = b.clone().to_f64() else {
484-
return jaq_unary_error(b, "cannot be an f64");
485-
};
486-
(a % f).into()
487-
}
446+
(a @ Int(_) | a @ Decimal(_), Float(b)) => (a.to_f64().unwrap() % b).into(),
447+
(Float(a), b @ Int(_) | b @ Decimal(_)) => (a % b.to_f64().unwrap()).into(),
488448

489449
(a, b) => return jaq_binary_error(a, b, "cannot be divided (remainder)"),
490450
};
@@ -506,7 +466,7 @@ impl Neg for JaqElement {
506466
// Only number types can be negated
507467
Int(a) => (-a).into(),
508468
Float(a) => (-a).into(),
509-
Decimal(a) => (-a.to_big_decimal()).to_decimal().into(),
469+
Decimal(a) => (-a.into_big_decimal()).into_decimal().into(),
510470

511471
other => return jaq_unary_error(other, "cannot be negated"),
512472
};
@@ -643,7 +603,7 @@ impl jaq_std::ValT for JaqElement {
643603
fn as_isize(&self) -> Option<isize> {
644604
match self.0.value() {
645605
Value::Int(i) => i.expect_i64().unwrap().to_isize(),
646-
Value::Decimal(d) => d.to_big_decimal().to_isize(),
606+
Value::Decimal(d) => d.into_big_decimal().to_isize(),
647607
_ => None,
648608
}
649609
}
@@ -672,32 +632,32 @@ pub(crate) mod ion_math {
672632

673633
/// We can't provide math traits for Decimal directly, so we have a helper trait
674634
pub(crate) trait DecimalMath: Sized {
675-
fn to_big_decimal(self) -> BigDecimal;
676-
fn to_decimal(self) -> Decimal;
635+
fn into_big_decimal(self) -> BigDecimal;
636+
fn into_decimal(self) -> Decimal;
677637

678638
fn add(self, v2: impl DecimalMath) -> Decimal {
679-
(self.to_big_decimal() + v2.to_big_decimal()).to_decimal()
639+
(self.into_big_decimal() + v2.into_big_decimal()).into_decimal()
680640
}
681641

682642
fn sub(self, v2: impl DecimalMath) -> Decimal {
683-
(self.to_big_decimal() - v2.to_big_decimal()).to_decimal()
643+
(self.into_big_decimal() - v2.into_big_decimal()).into_decimal()
684644
}
685645

686646
fn mul(self, v2: impl DecimalMath) -> Decimal {
687-
(self.to_big_decimal() * v2.to_big_decimal()).to_decimal()
647+
(self.into_big_decimal() * v2.into_big_decimal()).into_decimal()
688648
}
689649

690650
fn div(self, v2: impl DecimalMath) -> Decimal {
691-
(self.to_big_decimal() / v2.to_big_decimal()).to_decimal()
651+
(self.into_big_decimal() / v2.into_big_decimal()).into_decimal()
692652
}
693653

694654
fn rem(self, v2: impl DecimalMath) -> Decimal {
695-
(self.to_big_decimal() % v2.to_big_decimal()).to_decimal()
655+
(self.into_big_decimal() % v2.into_big_decimal()).into_decimal()
696656
}
697657
}
698658

699659
impl DecimalMath for Decimal {
700-
fn to_big_decimal(self) -> BigDecimal {
660+
fn into_big_decimal(self) -> BigDecimal {
701661
let magnitude = self.coefficient().magnitude().as_u128().unwrap();
702662
let bigint = match self.coefficient().sign() {
703663
Sign::Negative => -BigInt::from(magnitude),
@@ -706,35 +666,41 @@ pub(crate) mod ion_math {
706666
BigDecimal::new(bigint, self.scale())
707667
}
708668

709-
fn to_decimal(self) -> Decimal {
669+
fn into_decimal(self) -> Decimal {
710670
self
711671
}
712672
}
713673

714674
impl DecimalMath for Int {
715-
fn to_big_decimal(self) -> BigDecimal {
675+
fn into_big_decimal(self) -> BigDecimal {
716676
let data = self.expect_i128().unwrap(); // error case is unreachable with current ion-rs
717677
BigDecimal::from(data)
718678
}
719679

720-
fn to_decimal(self) -> Decimal {
680+
fn into_decimal(self) -> Decimal {
721681
let data = self.expect_i128().unwrap(); // error case is unreachable with current ion-rs
722682
Decimal::new(data, 0)
723683
}
724684
}
725685

726686
impl DecimalMath for BigDecimal {
727-
fn to_big_decimal(self) -> BigDecimal {
687+
fn into_big_decimal(self) -> BigDecimal {
728688
self
729689
}
730690

731-
fn to_decimal(self) -> Decimal {
691+
fn into_decimal(self) -> Decimal {
732692
let (coeff, exponent) = self.into_bigint_and_exponent();
733693
let data = coeff.to_i128().unwrap();
734694
Decimal::new(data, -exponent)
735695
}
736696
}
737697

698+
/// A helper trait to allow conversion of various Ion value types into f64. This is inherently a
699+
/// lossy conversion for most possible expressible Decimal and Integer values even inside f64's
700+
/// range of expression, so we accept that and move on. The only `None` case for any of these
701+
/// conversions is when converting a non-numeric `Value` type. A large enough `Int` or `Decimal`
702+
/// may convert to `Inf` as a float, but that's just the cost of doing business with floating
703+
/// point math.
738704
pub(crate) trait ToFloat {
739705
fn to_f64(self) -> Option<f64>;
740706
}
@@ -747,16 +713,13 @@ pub(crate) mod ion_math {
747713

748714
impl ToFloat for Int {
749715
fn to_f64(self) -> Option<f64> {
750-
self.as_i128().and_then(|data| {
751-
let float = data as f64;
752-
(float as i128 == data).then_some(float)
753-
})
716+
self.as_i128().map(|data| data as f64)
754717
}
755718
}
756719

757720
impl ToFloat for Decimal {
758721
fn to_f64(self) -> Option<f64> {
759-
self.to_big_decimal().to_f64()
722+
self.into_big_decimal().to_f64()
760723
}
761724
}
762725

0 commit comments

Comments
 (0)