Skip to content

Conversation

@zratkai
Copy link
Contributor

@zratkai zratkai commented Apr 25, 2023

What changes were proposed in this pull request?

This PR fixes issue : "ORC doesn't handle primitive category TIMESTAMPLOCALTZ".

Why are the changes needed?

Fix TIMESTAMPLOCALTZ type handling.

Does this PR introduce any user-facing change?

Yes, if user has a table containing timestamp column with default defined using TIMESTAMPLOCALTZ it fixes the ORC doesn't handle primitive category TIMESTAMPLOCALTZ" exception-

How was this patch tested?

Jenkins pipeline.

…with default defined using TIMESTAMPLOCALTZ fails with error " ORC doesn't handle primitive category TIMESTAMPLOCALTZ"
@InvisibleProgrammer
Copy link
Contributor

InvisibleProgrammer commented May 10, 2023

I have some extra thoughts about the tests:

There are two changes in the code: I think one for generic execution and one for vectorised. But I think the test only tests one of them.

And the test tests a lot of column types. But only the local timestamp column is affected on the change.
I don't remember to the exact location but I think I saw some test that involves all column types.

I would suggest creating (or finding an existing) vectorised test as well and also check if is there an existing checking all type of columns like test to extend.

Or, if the test is necessary to reproduce the issue, I would love seeing the ticket number on the top of the qtest file, as a comment.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@zratkai
Copy link
Contributor Author

zratkai commented May 10, 2023

he code: I think one for generic execution and one for vectoris

This is not the case. During the test both code changes are executed /tested. Ticket number is visible in git history for this file. It is very rare that a q test file contains the ticket number, I don't see any benefit.

@abstractdog abstractdog self-requested a review May 11, 2023 08:55
Copy link
Member

@deniskuzZ deniskuzZ left a comment

Choose a reason for hiding this comment

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

LGTM +1

@deniskuzZ deniskuzZ merged commit ea16342 into apache:master May 11, 2023
yeahyung pushed a commit to yeahyung/hive that referenced this pull request Jul 20, 2023
…h TIMESTAMPLOCALTZ type default (Zoltan Ratkai, reviewed by Attila Turoczy, Laszlo Bodor, Denys Kuzmenko)

Closes apache#4265
tarak271 pushed a commit to tarak271/hive-1 that referenced this pull request Dec 19, 2023
…h TIMESTAMPLOCALTZ type default (Zoltan Ratkai, reviewed by Attila Turoczy, Laszlo Bodor, Denys Kuzmenko)

Closes apache#4265
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants