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.

Sign up for GitHub

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

Jump to bottom

Add support for double to varchar coercion in hive tables #18930

Merged
merged 2 commits into from Sep 7, 2023
Merged

Add support for double to varchar coercion in hive tables #18930

merged 2 commits into from Sep 7, 2023

Conversation

Praveen2112
Copy link
Member

Description

Allows coerce a double type to varchar in hive tables.

Additional context and related issues

Overrides #18832

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 support for double to varchar coercion in hive tables

@cla-bot cla-bot bot added the cla-signed label Sep 5, 2023
@github-actions github-actions bot added tests:hive hive Hive connector labels Sep 5, 2023
atanasenko
atanasenko approved these changes Sep 5, 2023
View reviewed changes
...in/trino-hive/src/test/java/io/trino/plugin/hive/coercions/TestDoubleToVarcharCoercions.java Outdated Show resolved Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/coercions/CoercionUtils.java Outdated
@@ -395,7 +398,7 @@ protected void applyCoercedValue(BlockBuilder blockBuilder, Block block, int pos
}
}

public record CoercionOption(HiveTimestampPrecision timestampPrecision)
public record CoercionOption(HiveTimestampPrecision timestampPrecision, boolean treatNaNAsNull)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe CoercionOption should actually be CoercionOptions since it will contain multiple options?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have renamed it to CoercionContext WDYT ?

@Praveen2112 Praveen2112 force-pushed the praveen/double_to_varchar_coercer branch from 026b920 to bf8732f Compare September 5, 2023 13:41
@Praveen2112
Copy link
Member Author

@atanasenko AC and added some additional tests for infinity.

This was referenced Sep 5, 2023
Merged
Closed
@Praveen2112 Praveen2112 force-pushed the praveen/double_to_varchar_coercer branch from bf8732f to 5c4b7d4 Compare September 5, 2023 14:53
@Praveen2112
Copy link
Member Author

@kokosing AC

@Praveen2112 Praveen2112 force-pushed the praveen/double_to_varchar_coercer branch from 5c4b7d4 to 8bee23b Compare September 6, 2023 02:26
@Praveen2112
Introduce CoercionContext
44f012c 
This abstraction captures all additional information related to coercing column
@Praveen2112 Praveen2112 force-pushed the praveen/double_to_varchar_coercer branch from 8bee23b to 29faa27 Compare September 6, 2023 07:12
raunaqmorarka
raunaqmorarka reviewed Sep 6, 2023
View reviewed changes
plugin/trino-hive/src/main/java/io/trino/plugin/hive/coercions/DoubleToVarcharCoercer.java

Slice converted = Slices.utf8Slice(Double.toString(doubleValue));
if (!toType.isUnbounded() && countCodePoints(converted) > toType.getBoundedLength()) {
throw new TrinoException(INVALID_ARGUMENTS, format("Varchar representation of %s exceeds %s bounds", doubleValue, toType));
Copy link
Member

Choose a reason for hiding this comment

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

If we call truncateToLength(converted, toType) without calling countCodePoints(converted) > toType.getBoundedLength() does that not throw any error ?
It would be nicer to avoid the extra bounds check and have the error come out of truncateToLength directly

Copy link
Member Author

Choose a reason for hiding this comment

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

If we call truncateToLength(converted, toType) without calling countCodePoints(converted) > toType.getBoundedLength() does that not throw any error

Yes it doesn't throw any error - scope of truncateToLength is to trim the length of the Varchar representation - to a narrower precision - so we don't have to fail it explicitly - but here it is an redundant operation as we don't have to truncate it out. Thanks for pointing it out

...in/trino-hive/src/test/java/io/trino/plugin/hive/coercions/TestDoubleToVarcharCoercions.java Outdated
Double.MIN_VALUE,
Double.MAX_VALUE,
Double.POSITIVE_INFINITY,
Double.parseDouble("123456789.12345678")).collect(toDataProvider()),
Copy link
Member

Choose a reason for hiding this comment

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

nit: collect on next line ?

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HivePageSourceProvider.java Outdated
@@ -191,7 +192,9 @@ public static Optional<ConnectorPageSource> createHivePageSource(
Optional<BucketAdaptation> bucketAdaptation = createBucketAdaptation(bucketConversion, tableBucketNumber, regularAndInterimColumnMappings);
Optional<BucketValidator> bucketValidator = createBucketValidator(path, bucketValidation, tableBucketNumber, regularAndInterimColumnMappings);

CoercionContext coercionContext = new CoercionContext(getTimestampPrecision(session));
// ORC files treats Double.NaN as null
Copy link
Member

Choose a reason for hiding this comment

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

nit: is this an ORC spec behaviour or a Apache Hive behaviour, might want to reword this to clarify

@Praveen2112 Praveen2112 force-pushed the praveen/double_to_varchar_coercer branch from 29faa27 to 08c10d9 Compare September 6, 2023 14:32
@Praveen2112 Praveen2112 force-pushed the praveen/double_to_varchar_coercer branch from 08c10d9 to 142e6c9 Compare September 6, 2023 14:33
@Praveen2112
Copy link
Member Author

Thanks for the review @raunaqmorarka . AC

@Praveen2112 Praveen2112 merged commit 781f87b into trinodb:master Sep 7, 2023
66 checks passed
@github-actions github-actions bot added this to the 427 milestone Sep 7, 2023
@colebow colebow mentioned this pull request Sep 12, 2023
Merged
This was referenced Nov 6, 2023
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atanasenko atanasenko atanasenko approved these changes

@raunaqmorarka raunaqmorarka raunaqmorarka approved these changes

@kokosing kokosing kokosing approved these changes

@marcinsbd marcinsbd marcinsbd approved these changes

@ebyhr ebyhr Awaiting requested review from ebyhr

@krvikash krvikash Awaiting requested review from krvikash

@vlad-lyutenko vlad-lyutenko Awaiting requested review from vlad-lyutenko

Assignees
No one assigned
Labels
cla-signed hive Hive connector
Milestone
427
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Praveen2112 @atanasenko @raunaqmorarka @kokosing @marcinsbd