Skip to content

Add numeric integer to decimal coercion for hive tables#20024

Merged
losipiuk merged 1 commit intotrinodb:masterfrom
findinpath:findinpath/integer-number-to-decimal-coercion
Dec 7, 2023
Merged

Add numeric integer to decimal coercion for hive tables#20024
losipiuk merged 1 commit intotrinodb:masterfrom
findinpath:findinpath/integer-number-to-decimal-coercion

Conversation

@findinpath
Copy link
Contributor

@findinpath findinpath commented Dec 5, 2023

Description

Add tinyint/smallint/integer/bigint to decimal coercion for hive tables.

Enable the Hive users to retrieve the information from their tables after performing queries which change numeric integer columns to decimal.

The numeric integer values which overflow the target decimal type (either because they exceed the declared precision of the type to coerce to or because along with the scale of the target decimal type they get too big) will be considered (same as in Hive) as NULL values.

Additional context and related issues

Fixes #19931

PoC for dealing with writing decimal values

public static void writeDecimal(String value, DecimalType decimalType, BlockBuilder builder)
{
BigDecimal bigDecimal = parseDecimal(value, decimalType);
if (overflows(bigDecimal, decimalType.getPrecision())) {
throw new NumberFormatException(format("Cannot convert '%s' to %s. Value too large.", value, decimalType));
}
if (decimalType.isShort()) {
decimalType.writeLong(builder, bigDecimal.unscaledValue().longValueExact());
}
else {
decimalType.writeObject(builder, Int128.valueOf(bigDecimal.unscaledValue()));
}
}

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Hive
* Add numeric integer to decimal coercion for hive tables. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Dec 5, 2023
@findinpath findinpath self-assigned this Dec 5, 2023
@findinpath findinpath added the hive Hive connector label Dec 5, 2023
@findinpath findinpath force-pushed the findinpath/integer-number-to-decimal-coercion branch from 55c7aaa to eecd2d6 Compare December 5, 2023 20:05
@findinpath findinpath force-pushed the findinpath/integer-number-to-decimal-coercion branch from eecd2d6 to ecf5108 Compare December 5, 2023 20:11
@github-actions github-actions bot added the docs label Dec 5, 2023
@findinpath findinpath marked this pull request as ready for review December 5, 2023 20:13
@findinpath findinpath force-pushed the findinpath/integer-number-to-decimal-coercion branch from ecf5108 to ecb5bbf Compare December 6, 2023 16:38
@findinpath findinpath marked this pull request as draft December 6, 2023 16:39
Copy link
Member

@dain dain left a comment

Choose a reason for hiding this comment

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

Just one comment, but otherwise looks good

@findinpath findinpath force-pushed the findinpath/integer-number-to-decimal-coercion branch 2 times, most recently from c902ba9 to 8046516 Compare December 6, 2023 21:18
@findinpath
Copy link
Contributor Author

Just one comment, but otherwise looks good

@dain I got a distracted while doing the first version of the PR and haven't considered the edge cases. The PTs went 🟢 on my machine, but this happened due to the fact that apparently I was running an older version of the code :(.
In any case, I have polished up the PR and am confident that now it is in a good shape for review.

@findinpath findinpath marked this pull request as ready for review December 6, 2023 21:21
@findinpath findinpath force-pushed the findinpath/integer-number-to-decimal-coercion branch from 8046516 to 9f9b097 Compare December 6, 2023 21:29
@findinpath
Copy link
Contributor Author

CI failure TestHiveTransactionalTable > testReadFullAcid
#3635

@findinpath findinpath requested review from dain and pajaks December 7, 2023 08:04
Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

LGTM

@losipiuk losipiuk merged commit a0f8178 into trinodb:master Dec 7, 2023
@github-actions github-actions bot added this to the 435 milestone Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

Add tinyint/smallint/integer/bigint to decimal coercion for hive tables

3 participants