Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HUDI-6567] ExpressionEvaluators numeric types conversion support #9230

Conversation

Alowator
Copy link
Contributor

@Alowator Alowator commented Jul 19, 2023

Change Logs

Add numeric types conversion in ExpressionEvaluators::compare method

Impact

For table:
table tbl (a: bigint)

data skipping work only if both types are the same. If we use
select * from tbl where a > 10
data skipping will not work, we have to use explicit cast:
select * from tbl where a > cast(10 as bigint)
this PR fixes it.
See more in HUDI-6567

Risk level (write none, low medium or high below)

none: all conversions are being completed without information lose

Documentation Update

none

Contributor's checklist

  • Read through contributor's guide
  • Change Logs and Impact were stated clearly
  • Adequate tests were added if applicable
  • CI passed

@Alowator Alowator force-pushed the hudi-6567-ExpressionEvaluators-numeric-types-conversion-support branch from 928ee90 to fe77d23 Compare July 19, 2023 10:36
@Alowator
Copy link
Contributor Author

BigDecimal - the most common type of Byte, Short, Integer, Long, Float, Double and BigDecimal
So the most simple way to compare all this numeric types - cast them to BigDecimal (of course without information lose).
Also BigDecimal stores Integer types as primitive long inside.

@danny0405
Copy link
Contributor

Can we write some test cases for it.

@Alowator Alowator force-pushed the hudi-6567-ExpressionEvaluators-numeric-types-conversion-support branch 2 times, most recently from 7849eb3 to 18dfbfc Compare July 20, 2023 08:48
@Alowator
Copy link
Contributor Author

@danny0405 tests are written, pr could be reviewed

@Alowator
Copy link
Contributor Author

@danny0405 if there are no questions, it could be merged

void testNotEqualTo() {
@ParameterizedTest
@MethodSource("twelveObjects")
void testNotEqualTo(Object twelve) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we write a separate test for all the data types instead of modifying existing test. We just need to trigger the comparison of different data types. For each of the data type, test the comprison of it to all the other data types.

Just a simple Great Than(GT) comparision should be enough ~

@Alowator Alowator force-pushed the hudi-6567-ExpressionEvaluators-numeric-types-conversion-support branch from 18dfbfc to 187d528 Compare July 31, 2023 08:48
@Alowator
Copy link
Contributor Author

@hudi-bot run azure

@hudi-bot
Copy link

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

@danny0405 danny0405 merged commit 34afa67 into apache:master Aug 1, 2023
27 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.

3 participants