Skip to content

Add coercion test for unpartitioned tables in hive#16119

Merged
Praveen2112 merged 5 commits intotrinodb:masterfrom
Praveen2112:praveen/coercion_test_un_partitioned_table
Mar 10, 2023
Merged

Add coercion test for unpartitioned tables in hive#16119
Praveen2112 merged 5 commits intotrinodb:masterfrom
Praveen2112:praveen/coercion_test_un_partitioned_table

Conversation

@Praveen2112
Copy link
Copy Markdown
Member

@Praveen2112 Praveen2112 commented Feb 15, 2023 •
edited
Loading

Description

Adds additional test coverage for coercion on un-partitioned tables in hive.

Release notes

(x) This is not user-visible or docs only and no release notes are required.

@cla-bot cla-bot bot added the cla-signed label Feb 15, 2023
@Praveen2112 Praveen2112 force-pushed the praveen/coercion_test_un_partitioned_table branch 2 times, most recently from 7724041 to 2ff3167 Compare February 23, 2023 13:00
@Praveen2112 Praveen2112 force-pushed the praveen/coercion_test_un_partitioned_table branch from 2ff3167 to f23b8bc Compare March 6, 2023 12:59
@Praveen2112 Praveen2112 marked this pull request as ready for review March 6, 2023 13:57
@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 6, 2023

@ebyhr does this relate to yours #15938 ?

@Praveen2112
Copy link
Copy Markdown
Member Author

Praveen2112 commented Mar 6, 2023

It kind of add partial test coverage to #15938 - only for ORC as of now.

@Praveen2112 Praveen2112 force-pushed the praveen/coercion_test_un_partitioned_table branch 2 times, most recently from c6048db to 35b38d9 Compare March 7, 2023 07:39
@Praveen2112 Praveen2112 mentioned this pull request Mar 7, 2023
Merged
findepi
findepi reviewed Mar 7, 2023
View reviewed changes
testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/BaseTestHiveCoercion.java Outdated
Comment on lines 118 to 120
Copy link
Copy Markdown
Member

@findepi findepi Mar 7, 2023

Choose a reason for hiding this comment

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

why assertion became conditional?

Copy link
Copy Markdown
Member Author

@Praveen2112 Praveen2112 Mar 7, 2023

Choose a reason for hiding this comment

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

In case of unpartitioned tables we might not support all the column coercion as of now so we avoid the assertion when Trino throws some sort of exception.

Copy link
Copy Markdown
Member

@findepi findepi Mar 9, 2023

Choose a reason for hiding this comment

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

add a code comment then

...ct-tests/src/main/java/io/trino/tests/product/hive/TestHiveCoercionOnUnPartitionedTable.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Mar 7, 2023

Choose a reason for hiding this comment

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

UnPartitioned is one word

...ct-tests/src/main/java/io/trino/tests/product/hive/TestHiveCoercionOnUnPartitionedTable.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Mar 7, 2023

Choose a reason for hiding this comment

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

I don't think Requirements Providers gives us any benefit, especially for mutable tables which aren't reused.
Plain old direct create table in a test would be better.

Copy link
Copy Markdown
Member Author

@Praveen2112 Praveen2112 Mar 7, 2023

Choose a reason for hiding this comment

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

Maybe we can remove the Requirements Provider as a follow up as it would be common for both partitioned and unpartitioned tables.

testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/BaseTestHiveCoercion.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Mar 7, 2023

Choose a reason for hiding this comment

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

why removed?

Copy link
Copy Markdown
Member Author

@Praveen2112 Praveen2112 Mar 7, 2023

Choose a reason for hiding this comment

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

For Trino side failures we don't need check on hive version right ?

Copy link
Copy Markdown
Member

@findepi findepi Mar 9, 2023

Choose a reason for hiding this comment

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

unless the failure is related to what hive version did modify the version

anyway, why are you changing this line to first add .orElseThrow() and then to remove the condition if all you need is remove the condition?

Copy link
Copy Markdown
Member Author

@Praveen2112 Praveen2112 Mar 10, 2023

Choose a reason for hiding this comment

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

With the latest change by squashing the commits - this changeset got disappeared.

@Praveen2112
Minor fixes in comment
e1a8721 
- varchar is also a primitive
- tailing/trailing
@Praveen2112 Praveen2112 force-pushed the praveen/coercion_test_un_partitioned_table branch 2 times, most recently from aec09cc to fd20512 Compare March 7, 2023 12:48
@Praveen2112
Copy link
Copy Markdown
Member Author

Praveen2112 commented Mar 7, 2023

@findepi AC

@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 7, 2023

@Praveen2112 didn't re-review, but i see the build is failin

@Praveen2112 Praveen2112 force-pushed the praveen/coercion_test_un_partitioned_table branch from fd20512 to e08a8dc Compare March 9, 2023 07:19
skrzypo987
skrzypo987 reviewed Mar 9, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@skrzypo987 skrzypo987 left a comment

Choose a reason for hiding this comment

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

lgtm % findepi's comments

@Praveen2112 Praveen2112 force-pushed the praveen/coercion_test_un_partitioned_table branch from e08a8dc to 3c863f5 Compare March 9, 2023 09:04
findepi
findepi approved these changes Mar 9, 2023
View reviewed changes
testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveCoercion.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Mar 9, 2023

Choose a reason for hiding this comment

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

Make version optional in ColumnContext

can this commit include some description on why?
(sorry, i failed to infer this myself)

testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/BaseTestHiveCoercion.java Outdated
Comment on lines 118 to 120
Copy link
Copy Markdown
Member

@findepi findepi Mar 9, 2023

Choose a reason for hiding this comment

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

add a code comment then

testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/BaseTestHiveCoercion.java Outdated
Copy link
Copy Markdown
Member

@findepi findepi Mar 9, 2023

Choose a reason for hiding this comment

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

unless the failure is related to what hive version did modify the version

anyway, why are you changing this line to first add .orElseThrow() and then to remove the condition if all you need is remove the condition?

...ct-tests/src/main/java/io/trino/tests/product/hive/TestHiveCoercionOnUnpartitionedTable.java Outdated
Comment on lines 74 to 75
Copy link
Copy Markdown
Member

@findepi findepi Mar 9, 2023

Choose a reason for hiding this comment

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

redundant line break between STORED AS and ORC

@Praveen2112
Make version optional in ColumnContext
91845c2 
It allows us to re-use `ColumnContext` in scenarios which are independent
of hive version.
@Praveen2112
Use record for ColumnContext
cea2891
@Praveen2112 Praveen2112 force-pushed the praveen/coercion_test_un_partitioned_table branch from 3c863f5 to f454737 Compare March 10, 2023 06:59
@Praveen2112
Copy link
Copy Markdown
Member Author

Praveen2112 commented Mar 10, 2023

@findepi AC

@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 10, 2023

LGTM

@Praveen2112 Praveen2112 merged commit 4bfa7b6 into trinodb:master Mar 10, 2023
@github-actions github-actions bot added this to the 411 milestone Mar 10, 2023
@colebow colebow mentioned this pull request Mar 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@findepi findepi findepi approved these changes

@ebyhr ebyhr Awaiting requested review from ebyhr

@phd3 phd3 Awaiting requested review from phd3

@kokosing kokosing Awaiting requested review from kokosing

@huberty89 huberty89 Awaiting requested review from huberty89

@atanasenko atanasenko Awaiting requested review from atanasenko

+1 more reviewer

@skrzypo987 skrzypo987 skrzypo987 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Milestone

411

Development

Successfully merging this pull request may close these issues.

3 participants

@Praveen2112 @findepi @skrzypo987