Port aggregate test to sqllogictest #10384

Activity

1. jayzhan211

   added

   enhancementNew feature or requestNew feature or request

   good first issueGood for newcomersGood for newcomers

   on May 5, 2024
2. xinlifoobar commented on May 15, 2024

   

   xinlifoobar

   on May 15, 2024

   Last edited by xinlifoobar

   Contributor

   More actions

   Hello @jayzhan211 , I am trying to ramp up the repo via porting the min_max tests to sqllogictests.

   During the migrations, however, I found there are inconsistent behavours. An example is error handling inside rust class versus through the SQL engine as such:

   • Before:

     let left = ScalarValue::Decimal128(Some(123), 10, 2);
     let right = ScalarValue::Decimal128(Some(124), 10, 2);
     let result = max(&left, &right)?;
     assert_eq!(result, right);
   
     let right = ScalarValue::Decimal128(Some(124), 10, 3);
     let result = max(&left, &right);
     let err_msg = format!(
         "MIN/MAX is not expected to receive scalars of incompatible types {:?}",
         (Decimal128(Some(123), 10, 2), Decimal128(Some(124), 10, 3))
     );
     let expect = DataFusionError::Internal(err_msg);
     assert!(expect
         .strip_backtrace()
         .starts_with(&result.unwrap_err().strip_backtrace()));

   • After:

   statement ok
   DROP TABLE decimals;
   
   statement ok
   CREATE TABLE decimals_error (value DECIMAL(10, 2));
   
   statement ok
   INSERT INTO decimals_error VALUES (123.00), (arrow_cast(124.00, 'Decimal128(10, 3)'));
   
   query R
   SELECT MAX(value) FROM decimals_error
   ----
   124
   

   As far as I could tell, the error will be lost permanently.

   What's your opinions on such migrations? Shall we keep the tests that are different still in the rs files?
3. jayzhan211 commented on May 15, 2024

   

   jayzhan211

   on May 15, 2024

   ContributorAuthor

   More actions

   I think we don't need to keep the test.
   The reason that you didn't get the error is that we have coercion already in building values. It is the expected result.

   👍React with 👍1Reacted by Xin Li
4. jayzhan211

   closed this as completedin #10750on Jun 2, 2024